-
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
[Canvas] Fix expression editor test #146466
[Canvas] Fix expression editor test #146466
Conversation
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Awesome to see this test reinstated! Approving this based on the flaky test runner result, but ideally, more than 10 runs is better for flaky test runner, because some flakiness can be pernicious and only fail once every 100ish runs.
Left one nit.
await mdBox.type(newMd); | ||
await find.clickByCssSelector('.canvasArg--controls .euiButton'); | ||
await monacoEditor.setCodeEditorValue(newMd, 0); | ||
await PageObjects.common.sleep(300); |
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 could introduce some flakiness here, how did you come to the 300 ms number here?
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 set 300ms as a sorta arbitrary number. If I'm understanding correctly, the line below sets a 250ms debounce on updates to the sidebar editor before calling the onChange
callback. That might be enough time for the expression editor to receive the updates, or I could set this to a larger sleep
.
template: templateFromReactComponent(withDebounceArg(EditorArg, 250)), |
I'm running a new flaky test job with 100 runs here: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1607. Do you think that is sufficient or maybe try a larger number?
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 think 100 runs is definitely sufficient, thank you for doing that! If they all pass, then I'd say that 300ms is long enough to sleep, especially now that I know where the 300ms number comes from.
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.
Instead of sleep, how about putting assert statement below in a retry? That way the functional test is not couple to a debounce value
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.
Instead of sleep, how about putting assert statement below in a retry? That way the functional test is not couple to a debounce value
Ooh, good idea!
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
Fixes a skipped flaky test for the expression editor.
The test was skipped before the expression editor was changed to use Monaco in #133318. I refactored the test to use the Monaco service which should also address #133318 (comment).
Flaky test runner