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.

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

[Usage] Fix flaky UI Counters test #100979

Merged
merged 11 commits into from
Jun 3, 2021

Conversation

Bamieh
Copy link
Member

@Bamieh Bamieh commented May 31, 2021

Increase the duration to wait for ES indices to refresh after SO increments from 5 to 8 seconds. 8 seconds is long enough to remove flakiness. Already fixed in other similar telemetry test cases.

Ran this test 1000 times locally with no flakiness.
Ran the test 1000 times on the kibana flaky test runner (https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/1599/)

closes #93159 and #98240

@Bamieh Bamieh added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Telemetry Team:KibanaTelemetry labels May 31, 2021
@Bamieh Bamieh requested a review from a team May 31, 2021 07:48
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-telemetry (Team:KibanaTelemetry)

@Bamieh Bamieh added v7.14.0 v8.0.0 auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes labels May 31, 2021
@pgayvallet
Copy link
Contributor

Ran this test 1000 times locally with no flakiness.

Could you run it against the flaky test runner? Such flakiness is often not reproducible locally as much as on CI

@mshustov
Copy link
Contributor

Could you run it against the flaky test runner?

@Bamieh just in case, it's here

@Bamieh
Copy link
Member Author

Bamieh commented May 31, 2021

@mshustov thanks! I was wondering if that is actually a thing 😅

@Bamieh
Copy link
Member Author

Bamieh commented Jun 1, 2021

@Bamieh
Copy link
Member Author

Bamieh commented Jun 1, 2021

@elasticmachine merge upstream

@@ -32,7 +32,7 @@ export default function ({ getService }: FtrProviderContext) {
.expect(200);

// wait for SO to index data into ES
await new Promise((res) => setTimeout(res, 5 * 1000));
await new Promise((res) => setTimeout(res, 8 * 1000));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we retry requesting /api/saved_objects/_find?type=usage-counters until it's succeeded instead of relying on a manual delay?

Copy link
Member Author

Choose a reason for hiding this comment

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

That is doable although the delay pattern is all across kibana tests (200+ places)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, but it doesn't mean it's a good thing.

  • it leads to false positives due to changes in Kibana internals (what you are fixing right now)
  • it introduces unnecessary delays in the execution flow. Maybe the condition is true after 2sec?
  • it's not clear how to pick a correct value, devs tend to copy-paste them

Copy link
Member Author

Choose a reason for hiding this comment

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

I completely agree. What i meant is that we might need to resolve it across all tests rather than just this one test. I've created a helper util (with tests) to retry assertions. We can move it outside the tests/.../telemetry folder in a separate PR later on. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can move it outside the tests/.../telemetry folder in a separate PR later on.

ok then. Let's create an issue for Core-related tests.

@Bamieh Bamieh requested a review from mshustov June 3, 2021 12:07
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@Bamieh Bamieh merged commit de85036 into elastic:master Jun 3, 2021
@Bamieh Bamieh deleted the usage/fix_ui_counters_test branch June 3, 2021 14:47
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 3, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 3, 2021
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Ahmad Bamieh <ahmadbamieh@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.14.0 v8.0.0
Projects
None yet
6 participants