[Dashboard] Fix flaky sync_colors.tsx test
#172633
Merged
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
Heenawter
added
Team:Presentation
Heenawter
force-pushed
the
fix-flaky-sync-colors-148557_2023-12-05
branch
from
6499b07 to
03e3499
Compare
Heenawter
added
backport:prev-minor
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @Heenawter |
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Dec 6, 2023
## Summary It is possible for the `noDataPopover` to take more than the `100ms` timeout from `ensureHiddenNoDataPopover` (pictured below) to appear - in these cases, any tests that make use of the Lens page object's `goToTimeRange` method will fail due to clicking the wrong element (the tour step rather than the time picker).  While my initial thought was to simply increase the timeout for the `noDataPopover` check, this isn't ideal - **a lot** of tests use the `goToTimeRange` method and, by doing this, we would be slowing down **every single one of them** even when the test wouldn't have failed to begin with! Instead, I've chosen to surround the important parts of the code in `goToTimeRange` with a `retry` - that way, if the `noDataPopover` never shows up or it shows up comfortably within the `100ms` timeout, the impact to the speed of a given test will be **minimal**; however, if the no data popover shows up **outside** of this timeout, the retry will save the test from outright failure. ### [Flaky Test Runner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4192)  ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit d9e2d06)
💔 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 |
kibanamachine
added a commit
that referenced
this pull request
Dec 6, 2023
# Backport This will backport the following commits from `main` to `8.11`: - [[Dashboard] Fix flaky `sync_colors.tsx` test (#172633)](#172633) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Hannah Mudge","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-12-06T14:55:13Z","message":"[Dashboard] Fix flaky `sync_colors.tsx` test (#172633)\n\n## Summary\r\n\r\nIt is possible for the `noDataPopover` to take more than the `100ms`\r\ntimeout from `ensureHiddenNoDataPopover` (pictured below) to appear - in\r\nthese cases, any tests that make use of the Lens page object's\r\n`goToTimeRange` method will fail due to clicking the wrong element (the\r\ntour step rather than the time picker).\r\n\r\n\r\n\r\n\r\n\r\nWhile my initial thought was to simply increase the timeout for the\r\n`noDataPopover` check, this isn't ideal - **a lot** of tests use the\r\n`goToTimeRange` method and, by doing this, we would be slowing down\r\n**every single one of them** even when the test wouldn't have failed to\r\nbegin with! Instead, I've chosen to surround the important parts of the\r\ncode in `goToTimeRange` with a `retry` - that way, if the\r\n`noDataPopover` never shows up or it shows up comfortably within the\r\n`100ms` timeout, the impact to the speed of a given test will be\r\n**minimal**; however, if the no data popover shows up **outside** of\r\nthis timeout, the retry will save the test from outright failure.\r\n\r\n### [Flaky Test\r\nRunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4192)\r\n\r\n\r\n\r\n\r\n\r\n### Checklist\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"d9e2d06512196ff6800a8441fc4605c258f206dd","branchLabelMapping":{"^v8.12.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Presentation","loe:small","release_note:skip","impact:critical","backport:prev-minor","v8.12.0","v8.13.0"],"number":172633,"url":"https://github.com/elastic/kibana/pull/172633","mergeCommit":{"message":"[Dashboard] Fix flaky `sync_colors.tsx` test (#172633)\n\n## Summary\r\n\r\nIt is possible for the `noDataPopover` to take more than the `100ms`\r\ntimeout from `ensureHiddenNoDataPopover` (pictured below) to appear - in\r\nthese cases, any tests that make use of the Lens page object's\r\n`goToTimeRange` method will fail due to clicking the wrong element (the\r\ntour step rather than the time picker).\r\n\r\n\r\n\r\n\r\n\r\nWhile my initial thought was to simply increase the timeout for the\r\n`noDataPopover` check, this isn't ideal - **a lot** of tests use the\r\n`goToTimeRange` method and, by doing this, we would be slowing down\r\n**every single one of them** even when the test wouldn't have failed to\r\nbegin with! Instead, I've chosen to surround the important parts of the\r\ncode in `goToTimeRange` with a `retry` - that way, if the\r\n`noDataPopover` never shows up or it shows up comfortably within the\r\n`100ms` timeout, the impact to the speed of a given test will be\r\n**minimal**; however, if the no data popover shows up **outside** of\r\nthis timeout, the retry will save the test from outright failure.\r\n\r\n### [Flaky Test\r\nRunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4192)\r\n\r\n\r\n\r\n\r\n\r\n### Checklist\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"d9e2d06512196ff6800a8441fc4605c258f206dd"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"main","label":"v8.12.0","labelRegex":"^v8.12.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/172633","number":172633,"mergeCommit":{"message":"[Dashboard] Fix flaky `sync_colors.tsx` test (#172633)\n\n## Summary\r\n\r\nIt is possible for the `noDataPopover` to take more than the `100ms`\r\ntimeout from `ensureHiddenNoDataPopover` (pictured below) to appear - in\r\nthese cases, any tests that make use of the Lens page object's\r\n`goToTimeRange` method will fail due to clicking the wrong element (the\r\ntour step rather than the time picker).\r\n\r\n\r\n\r\n\r\n\r\nWhile my initial thought was to simply increase the timeout for the\r\n`noDataPopover` check, this isn't ideal - **a lot** of tests use the\r\n`goToTimeRange` method and, by doing this, we would be slowing down\r\n**every single one of them** even when the test wouldn't have failed to\r\nbegin with! Instead, I've chosen to surround the important parts of the\r\ncode in `goToTimeRange` with a `retry` - that way, if the\r\n`noDataPopover` never shows up or it shows up comfortably within the\r\n`100ms` timeout, the impact to the speed of a given test will be\r\n**minimal**; however, if the no data popover shows up **outside** of\r\nthis timeout, the retry will save the test from outright failure.\r\n\r\n### [Flaky Test\r\nRunner](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4192)\r\n\r\n\r\n\r\n\r\n\r\n### Checklist\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n### For maintainers\r\n\r\n- [ ] This was checked for breaking API changes and was [labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)","sha":"d9e2d06512196ff6800a8441fc4605c258f206dd"}},{"branch":"8.13","label":"v8.13.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Hannah Mudge <[email protected]>
|
This PR didn't make it on time for the latest 8.11.2 BC. Updating the labels. |
2 tasks
Heenawter
added a commit
that referenced
this pull request
Dec 20, 2023
Closes #148557 ## Summary [Ran the FTR](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4636) on the skipped test suite and it passed 100 times... Really unclear on why it keeps failing, especially after #172633. For now, since I can't get it to fail locally or with the FTR, I'm going to unskip. If it starts to fail on `main` again, we could loop in the Vis team to help (since it's the timepicker in the Lens app that is misbehaving) - but hopefully the previous failure was a fluke and that won't be necessary🤞 ### Checklist - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed ([Link](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/4636))  ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
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.
Closes #148557
Summary
It is possible for the
noDataPopoverto take more than the100mstimeout fromensureHiddenNoDataPopover(pictured below) to appear - in these cases, any tests that make use of the Lens page object'sgoToTimeRangemethod will fail due to clicking the wrong element (the tour step rather than the time picker).While my initial thought was to simply increase the timeout for the
noDataPopovercheck, this isn't ideal - a lot of tests use thegoToTimeRangemethod and, by doing this, we would be slowing down every single one of them even when the test wouldn't have failed to begin with! Instead, I've chosen to surround the important parts of the code ingoToTimeRangewith aretry- that way, if thenoDataPopovernever shows up or it shows up comfortably within the100mstimeout, the impact to the speed of a given test will be minimal; however, if the no data popover shows up outside of this timeout, the retry will save the test from outright failure.Flaky Test Runner
Checklist
For maintainers