-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[functional-tests] TimePicker optimizations #130200
[functional-tests] TimePicker optimizations #130200
Conversation
Pinging @elastic/kibana-operations (Team:Operations) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a few questions but I don't want to hold up any change that speeds up tests (and doesn't cause flakiness).
Something I've done in a few places to speed up checks is to get a parent element which we know will always be there, and then check either the innerHTML or the other classes of the element to see if it's open or not. That makes it fast in both cases where the element is or isn't there.
await this.retryCall(async function click(wrapper) { | ||
await wrapper.scrollIntoViewIfNecessary(topOffset); | ||
await wrapper._webElement.click(); | ||
}, retries); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what type of failures this will detect and retry for? A click can fail because of a stale element, but retrying won't fix that. There's no code to detect if the desired action from the click actually happened. Maybe if the element was initially disabled, a retry could click again when it's enabled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually wrote this a while ago, but I believe the main case was an element that was hidden or obscured by another element. For example, if a toast notification pops up in front of the element and you try to click it, I believe you get an error saying that another element would receive the click.
timeout: 50, | ||
}); | ||
if (isShowDatesButton) { | ||
await this.testSubjects.clickWithRetries('superDatePickerShowDatesButton', 0, 50); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like zero retries? I don't see retries actually being used but I might be missing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in the Lens test I ran, in the output I see the test looking for superDatePickerShowDatesButton
and not finding it. I've opened and closed the quick pick (superDatePickerToggleQuickMenuButton
) and the start date popover (superDatePickerstartDatePopoverButton
) and searched the dom elements and never find it. Maybe it's used on another page.
[15:12:37.302824000] │ debg TestSubjects.exists(superDatePickerShowDatesButton)
[15:12:37.307540000] │ debg Find.existsByDisplayedByCssSelector('[data-test-subj="superDatePickerShowDatesButton"]') with timeout=50
[15:12:37.350293000] │ debg --- retry.tryForTime error: [data-test-subj="superDatePickerShowDatesButton"] is not displayed
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
@LeeDr could you take a look again? I originally split this out from some past work, and didn't realize more of it could be taken out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Code and CI results review
(cherry picked from commit fb87699)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…disable-server-side * 'main' of github.com:elastic/kibana: (103 commits) [Osquery] Update eslint config (elastic#129637) [Uptime] Update monitor saved object mappings (elastic#130433) Add links to metricbeat module docs (elastic#130519) Add link to troubleshooting guide in confirm data copy (elastic#130420) [Step 3] Cleanup charts plugin (elastic#130132) [Visualize] Adds a deprecation warning to the pie app (elastic#130447) [Maps] fix vector tile load errors not displayed in legend (elastic#130395) [CI] Split alerting-api-integration tests into separate cigroups (elastic#130414) [CI] Use spot instances for default cigroups in PR CI (elastic#130476) [functional-tests] TimePicker optimizations (elastic#130200) [kbn/pm] use stable module ids in dist (elastic#130497) [8.2.1][Security Solution][Session view] fix full screen session view margin (elastic#130496) Fix wrong config in comments (elastic#130378) Add deprecated telemetry (elastic#130458) Add eslint rule to support breaking up packages (elastic#130483) [Security Solution][Endpoint] Fix test stability and un-skip flaky tests (elastic#130176) Update object types for SharePoint Online external connector (elastic#130478) [Workplace Search] Fix broken feedback link (elastic#130475) Rename the term "execution" in config to "run" (elastic#130172) [Cloud Posture] use index with keyword mapping (elastic#130456) ... # Conflicts: # docs/user/reporting/index.asciidoc # x-pack/plugins/reporting/public/types.ts # x-pack/plugins/screenshotting/server/screenshots/index.test.ts # x-pack/plugins/screenshotting/server/screenshots/index.ts
These optimizations speed up some of the interactions with the timepicker during functional tests. Previously, many common operations had a minimum execution time of 2.5s and are now instant.
These changes cut about 1hr of cumulative execution time off the build, divided up over various test suites. For example, the lens tests in CI Group 3 are now almost 10 minutes faster.