Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ML] Functional tests - stabilize slider value selection #94313

Merged
merged 1 commit into from
Mar 11, 2021
Merged

[ML] Functional tests - stabilize slider value selection #94313

merged 1 commit into from
Mar 11, 2021

Conversation

pheyos
Copy link
Member

@pheyos pheyos commented Mar 10, 2021 •
edited
Loading

Summary

This PR stabilizes the slider value selection during ML functional tests.

Details

Current situation

The logic of the setSliderValue method is not perfectly accurate and with bad timing can lead to test failures, see e.g. #94084.
In the final step, one can see a message like retry.tryForTime error: slider value should be '20' (got '20'), which looks wrong, but is actually not failing the test. In fact, the final throw of the else is needed to trigger the outer retry. But if the inner retry times out in a bad moment, currentValue might not be set correctly when the outer retry is entered (which relies on the calculation of the inner retry).

Changes with this PR

  • Move the currentValue fetching and currentDiff calculation to the start of the outer retry, so it's always accurate and doesn't rely on the inner retry calculation anymore.
  • Move the final assertSliderValue into the else, so it returns with a success if the last slider modification hit the target value or throws an error, which will trigger the outer retry. No assertion outside needed anymore, because either currentDiff === 0 (in if) or assertSliderValue was successful (in else), which both mean that the value has been set correctly.

Closes #94084

@pheyos pheyos requested a review from a team as a code owner March 10, 2021 15:39
@pheyos pheyos self-assigned this Mar 10, 2021
@pheyos pheyos added :ml release_note:skip Skip the PR/issue when compiling release notes test_ui_functional v7.13.0 v8.0.0 labels Mar 10, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@pheyos
Copy link
Member Author

pheyos commented Mar 10, 2021 •
edited
Loading

Checking test stability in a flaky test runner job ... no failure in 50 runs ✔️

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @pheyos

peteharverson
peteharverson approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

alvarezmelissa87
alvarezmelissa87 approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ⚡

darnautov
darnautov approved these changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pheyos pheyos added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 11, 2021
@pheyos pheyos merged commit 92307bf into elastic:master Mar 11, 2021
@pheyos pheyos deleted the stabilize_slider_value_selection branch March 11, 2021 09:11
@kibanamachine kibanamachine mentioned this pull request Mar 11, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 11, 2021
@pheyos @kibanamachine
[ML] Functional tests - stabilize slider value selection (elastic#94313)
178dcd3 
This PR stabilizes the slider value selection during ML functional tests.
@kibanamachine
Copy link
Contributor

💚 Backport successful

✅ 7.x / #94399

Successful backport PRs will be merged automatically after passing CI.

jloleysens added a commit that referenced this pull request Mar 11, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/rollup-v2…
bc79c12 
…-action

* 'master' of github.com:elastic/kibana: (43 commits)
  [Console] Update copy when showing warnings in response headers (#94270)
  [TSVB] Type public code. Step 1 (#93231)
  [ML] Functional tests - stabilize slider value selection (#94313)
  skip another suite blocking es promotion (#94367)
  [Security Solution] Eliminates a redundant external link icon (#94194)
  skip another suite blocking es promotion (#94367)
  [App Search] Role mappings migration part 1 (#94346)
  [Security Solution][Detections] Fix flaky indicator enrichment tests (#94241)
  [Workplace Search] Deduplicate icons (#94359)
  [ML] Add latest transform to intro text (#94039)
  skip test failing es promotion (#94367)
  [Maps] convert elasticsearch_utils to TS (#93984)
  [Security_Solution][Telemetry] - Update endpoint usage to use agentService (#93829)
  [Security Solution][Exceptions] Fixes OS adding method for exception enrichment (#94343)
  [ILM] Add support for frozen phase (#93068)
  [App Search] Fixed 2 relevance tuning bugs (#94312)
  remove `try` auth mode (#94287)
  Removing resolver functional tests (#94331)
  migrate warning mixin to core (#94273)
  [App Search] Add routes for Role Mappings (#94221)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/cold_phase/cold_phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/phase/phase.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/searchable_snapshot_field/searchable_snapshot_field.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/form/configuration_issues_context.tsx
kibanamachine added a commit that referenced this pull request Mar 11, 2021
@kibanamachine @pheyos
[ML] Functional tests - stabilize slider value selection (#94313) (#9…
5ea70ea 
…4399)

This PR stabilizes the slider value selection during ML functional tests.

Co-authored-by: Robert Oskamp <robert.oskamp@elastic.co>
@pheyos pheyos added the v7.12.2 label May 10, 2021
@pheyos pheyos mentioned this pull request May 10, 2021
Merged
pheyos added a commit to pheyos/kibana that referenced this pull request May 10, 2021
@pheyos
[ML] Functional tests - stabilize slider value selection (elastic#94313)
86f66f3 
This PR stabilizes the slider value selection during ML functional tests.
pheyos added a commit that referenced this pull request May 10, 2021
@pheyos
[ML] Functional tests - stabilize slider value selection (#94313) (#9…
cc68970 
…9600)

This PR stabilizes the slider value selection during ML functional tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson approved these changes

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

@darnautov darnautov darnautov approved these changes

Assignees

@pheyos pheyos

Labels
auto-backport Deprecated - use backport:version if exact versions are needed :ml release_note:skip Skip the PR/issue when compiling release notes test_ui_functional v7.12.2 v7.13.0 v8.0.0
Projects
None yet
Milestone
No milestone
6 participants
@pheyos @elasticmachine @kibanamachine @darnautov @alvarezmelissa87 @peteharverson