[Maps] Update map report image test threshold for new EMS styles #167162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
jsanz
added
Team:Presentation
|
OK so finally 🙌 tests passed so I can remove the hardcoded link to staging EMS Styles and the only threshold to raise is at So next, I will remove the hardoced URL to let the tests run again against just that threshold change so we can merge this safely. Later, once the styles have been deployed to production we can update the report image if it feels necessary to revert to the original value |
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]
History
To update your PR or re-run it, just comment with: |
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
jsanz
changed the title
nreese
reviewed
Sep 28, 2023
| @@ -226,7 +226,7 @@ export default function ({ | |||
| updateBaselines | |||
| ); | |||
|
|
|||
| expect(percentDiff).to.be.lessThan(0.03); | |||
| expect(percentDiff).to.be.lessThan(0.035); | |||
There was a problem hiding this comment.
Instead of changing percentage, how about updating snapshots?
There was a problem hiding this comment.
The problem of updating the snapshot is that there will be an unknown interval between this PR is merged and the styles go to production. During that time tests would fail.
That's why I would only update the snapshot and revert this change after the styles are in production.
nreese
approved these changes
Sep 28, 2023
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Sep 28, 2023
…stic#167162) Updated EMS Styles are waiting to be put into production. They are already available in Elastic staging environment ([preview](maps-staging.elastic.co/?manifest=testing)). This PR is a safe measure to ensure that this change do not break our CI tests. The process has been as follows: 1. Momentarily replaces the EMS Tile Service `tileApiUrl` by our staging server to force the use of the new styles and check which tests break with the slightly different basemaps at [12481c6](elastic@12481c6) 2. Look for related [broken tests](https://buildkite.com/elastic/kibana-pull-request/builds/161870) ``` Error: expected 0.030813687704837327 to be below 0.03 ``` 4. Adjust the threshold for the dashboard report, since the new value was slightly over the limit [e655b84](elastic@e655b84) 5. Wait for a green CI (this took a few days because of unrelated issues with Kibana CI) 6. Revert the `tileApiUrl` change to its original value [c0030bc](elastic@c0030bc) --------- Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 274fe1e)
💔 Some backports could not be created
Note: Successful backport PRs will be merged automatically after passing CI. Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
|
@nreese backport to 7.17 failed because the change is in a test that does not exist in 7.17 screenshots.ts. Just to be sure, I will open a new draft PR against 7.17 with the same check I did to ensure no new tests fail. |
kibanamachine
added a commit
that referenced
this pull request
Oct 2, 2023
…es (#167162) (#167534) # Backport This will backport the following commits from `main` to `8.10`: - [[Maps] Update map report image test threshold for new EMS styles (#167162)](#167162) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Jorge Sanz","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-09-28T15:34:15Z","message":"[Maps] Update map report image test threshold for new EMS styles (#167162)\n\nUpdated EMS Styles are waiting to be put into production. They are\r\nalready available in Elastic staging environment\r\n([preview](maps-staging.elastic.co/?manifest=testing)). This PR is a\r\nsafe measure to ensure that this change do not break our CI tests.\r\n\r\nThe process has been as follows:\r\n\r\n1. Momentarily replaces the EMS Tile Service `tileApiUrl` by our staging\r\nserver to force the use of the new styles and check which tests break\r\nwith the slightly different basemaps at\r\n[12481c6](https://github.com/elastic/kibana/pull/167162/commits/12481c6ada986cda258f37108f5f0dd6c85f7689)\r\n2. Look for related [broken\r\ntests](https://buildkite.com/elastic/kibana-pull-request/builds/161870)\r\n```\r\nError: expected 0.030813687704837327 to be below 0.03\r\n```\r\n4. Adjust the threshold for the dashboard report, since the new value\r\nwas slightly over the limit\r\n[e655b84](https://github.com/elastic/kibana/pull/167162/commits/e655b845695788c67356de115ab6e4627a42b07b)\r\n5. Wait for a green CI (this took a few days because of unrelated issues\r\nwith Kibana CI)\r\n6. Revert the `tileApiUrl` change to its original value\r\n[c0030bc](https://github.com/elastic/kibana/pull/167162/commits/c0030bcff1a1de14860a05c27f26af45fda5e240)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"274fe1e9d04241aa99b2ad2c44e97f39e2a3f891","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Presentation","release_note:skip","backport:prev-MAJOR","Feature:Maps","v8.11.0"],"number":167162,"url":"https://github.com/elastic/kibana/pull/167162","mergeCommit":{"message":"[Maps] Update map report image test threshold for new EMS styles (#167162)\n\nUpdated EMS Styles are waiting to be put into production. They are\r\nalready available in Elastic staging environment\r\n([preview](maps-staging.elastic.co/?manifest=testing)). This PR is a\r\nsafe measure to ensure that this change do not break our CI tests.\r\n\r\nThe process has been as follows:\r\n\r\n1. Momentarily replaces the EMS Tile Service `tileApiUrl` by our staging\r\nserver to force the use of the new styles and check which tests break\r\nwith the slightly different basemaps at\r\n[12481c6](https://github.com/elastic/kibana/pull/167162/commits/12481c6ada986cda258f37108f5f0dd6c85f7689)\r\n2. Look for related [broken\r\ntests](https://buildkite.com/elastic/kibana-pull-request/builds/161870)\r\n```\r\nError: expected 0.030813687704837327 to be below 0.03\r\n```\r\n4. Adjust the threshold for the dashboard report, since the new value\r\nwas slightly over the limit\r\n[e655b84](https://github.com/elastic/kibana/pull/167162/commits/e655b845695788c67356de115ab6e4627a42b07b)\r\n5. Wait for a green CI (this took a few days because of unrelated issues\r\nwith Kibana CI)\r\n6. Revert the `tileApiUrl` change to its original value\r\n[c0030bc](https://github.com/elastic/kibana/pull/167162/commits/c0030bcff1a1de14860a05c27f26af45fda5e240)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"274fe1e9d04241aa99b2ad2c44e97f39e2a3f891"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/167162","number":167162,"mergeCommit":{"message":"[Maps] Update map report image test threshold for new EMS styles (#167162)\n\nUpdated EMS Styles are waiting to be put into production. They are\r\nalready available in Elastic staging environment\r\n([preview](maps-staging.elastic.co/?manifest=testing)). This PR is a\r\nsafe measure to ensure that this change do not break our CI tests.\r\n\r\nThe process has been as follows:\r\n\r\n1. Momentarily replaces the EMS Tile Service `tileApiUrl` by our staging\r\nserver to force the use of the new styles and check which tests break\r\nwith the slightly different basemaps at\r\n[12481c6](https://github.com/elastic/kibana/pull/167162/commits/12481c6ada986cda258f37108f5f0dd6c85f7689)\r\n2. Look for related [broken\r\ntests](https://buildkite.com/elastic/kibana-pull-request/builds/161870)\r\n```\r\nError: expected 0.030813687704837327 to be below 0.03\r\n```\r\n4. Adjust the threshold for the dashboard report, since the new value\r\nwas slightly over the limit\r\n[e655b84](https://github.com/elastic/kibana/pull/167162/commits/e655b845695788c67356de115ab6e4627a42b07b)\r\n5. Wait for a green CI (this took a few days because of unrelated issues\r\nwith Kibana CI)\r\n6. Revert the `tileApiUrl` change to its original value\r\n[c0030bc](https://github.com/elastic/kibana/pull/167162/commits/c0030bcff1a1de14860a05c27f26af45fda5e240)\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"274fe1e9d04241aa99b2ad2c44e97f39e2a3f891"}}]}] BACKPORT--> Co-authored-by: Jorge Sanz <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updated EMS Styles are waiting to be put into production. They are already available in Elastic staging environment (preview). This PR is a safe measure to ensure that this change do not break our CI tests.
The process has been as follows:
tileApiUrlby our staging server to force the use of the new styles and check which tests break with the slightly different basemaps at 12481c6tileApiUrlchange to its original value c0030bc