Simplify test code, reenable skipped suite #163158
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
gsoldevila
added
Team:Core
afharo
approved these changes
Aug 31, 2023
There was a problem hiding this comment.
I'm happy with this approach. We are proving that the underlying core feature is working with the running tests.
Re the skipped ones, IIRC, the Sample Dashboards were revisited, and some visualizations have changed ever since this test was skipped.
We may ask other teams help to adapt the tests accordingly (is it @elastic/appex-sharedux or @elastic/kibana-presentation 🤷 ?)
| await this.testSubjects.click('superDatePickerToggleQuickMenuButton'); | ||
| await this.testSubjects.exists('`superDatePickerCommonlyUsed_${option}`', { timeout: 5000 }); |
There was a problem hiding this comment.
Should this only use the backticks?
Suggested change
| await this.testSubjects.exists('`superDatePickerCommonlyUsed_${option}`', { timeout: 5000 }); | |
| await this.testSubjects.exists(`superDatePickerCommonlyUsed_${option}`, { timeout: 5000 }); |
There was a problem hiding this comment.
Wait! How on earth did that not fail? will review this
UPDATE: this caused the exists check to wait for 5 seconds unnecessarily.
Fixing the malformed identifier should speed up the process.
|
Pinging @elastic/kibana-core (Team:Core) |
💚 Build Succeeded
Metrics [docs]
History
To update your PR or re-run it, just comment with: cc @gsoldevila |
|
I created a separate issue to update + unskip the outdated tests: |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Aug 31, 2023
Attempt at fixing elastic#149611 I updated the test code as follows: * Removed the RxJS logic and simply factorised the reads to read only once. * Got rid of the "retry" service. There's already a mechanism in place to make sure the logs are up-to-date. * Updated the `setCommonlyUsedTime` method to make sure it awaits for the popup to be ready before clicking. * Skipped 4 tests that seem outdated, the logs don't have the related entries even after waiting for more than one minute and flushing (in fact, they all seem to systematically fail on `main` too): * lnsLegacyMetric * [Flights] Delays & Cancellations * [Flights] Destination Weather * [Flights] Delay Buckets Attached is the generated [kibana.log](https://github.com/elastic/kibana/files/12260144/kibana.log) (focussing only the `browser.ts` tests). So for the skipped tests, this does not look like flakiness anymore, but rather outdated / incorrect checks. I propose we review and update them on a separate issue / PR. 50 runs results [here](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3026). (cherry picked from commit a62d9a9)
kibanamachine
added a commit
that referenced
this pull request
Aug 31, 2023
# Backport This will backport the following commits from `main` to `8.10`: - [Simplify test code, reenable skipped suite (#163158)](#163158) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Gerard Soldevila","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-31T13:09:19Z","message":"Simplify test code, reenable skipped suite (#163158)\n\nAttempt at fixing https://github.com/elastic/kibana/issues/149611\r\n\r\nI updated the test code as follows:\r\n* Removed the RxJS logic and simply factorised the reads to read only\r\nonce.\r\n* Got rid of the \"retry\" service. There's already a mechanism in place\r\nto make sure the logs are up-to-date.\r\n* Updated the `setCommonlyUsedTime` method to make sure it awaits for\r\nthe popup to be ready before clicking.\r\n* Skipped 4 tests that seem outdated, the logs don't have the related\r\nentries even after waiting for more than one minute and flushing (in\r\nfact, they all seem to systematically fail on `main` too):\r\n * lnsLegacyMetric\r\n * [Flights] Delays & Cancellations\r\n * [Flights] Destination Weather\r\n * [Flights] Delay Buckets\r\n\r\nAttached is the generated\r\n[kibana.log](https://github.com/elastic/kibana/files/12260144/kibana.log)\r\n(focussing only the `browser.ts` tests).\r\n\r\nSo for the skipped tests, this does not look like flakiness anymore, but\r\nrather outdated / incorrect checks. I propose we review and update them\r\non a separate issue / PR.\r\n\r\n50 runs results\r\n[here](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3026).","sha":"a62d9a90f53c6ebec48d4982393c637cc73c26ac","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Team:Core","release_note:skip","test-failure-flaky","backport:prev-minor","v8.11.0"],"number":163158,"url":"https://github.com/elastic/kibana/pull/163158","mergeCommit":{"message":"Simplify test code, reenable skipped suite (#163158)\n\nAttempt at fixing https://github.com/elastic/kibana/issues/149611\r\n\r\nI updated the test code as follows:\r\n* Removed the RxJS logic and simply factorised the reads to read only\r\nonce.\r\n* Got rid of the \"retry\" service. There's already a mechanism in place\r\nto make sure the logs are up-to-date.\r\n* Updated the `setCommonlyUsedTime` method to make sure it awaits for\r\nthe popup to be ready before clicking.\r\n* Skipped 4 tests that seem outdated, the logs don't have the related\r\nentries even after waiting for more than one minute and flushing (in\r\nfact, they all seem to systematically fail on `main` too):\r\n * lnsLegacyMetric\r\n * [Flights] Delays & Cancellations\r\n * [Flights] Destination Weather\r\n * [Flights] Delay Buckets\r\n\r\nAttached is the generated\r\n[kibana.log](https://github.com/elastic/kibana/files/12260144/kibana.log)\r\n(focussing only the `browser.ts` tests).\r\n\r\nSo for the skipped tests, this does not look like flakiness anymore, but\r\nrather outdated / incorrect checks. I propose we review and update them\r\non a separate issue / PR.\r\n\r\n50 runs results\r\n[here](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3026).","sha":"a62d9a90f53c6ebec48d4982393c637cc73c26ac"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/163158","number":163158,"mergeCommit":{"message":"Simplify test code, reenable skipped suite (#163158)\n\nAttempt at fixing https://github.com/elastic/kibana/issues/149611\r\n\r\nI updated the test code as follows:\r\n* Removed the RxJS logic and simply factorised the reads to read only\r\nonce.\r\n* Got rid of the \"retry\" service. There's already a mechanism in place\r\nto make sure the logs are up-to-date.\r\n* Updated the `setCommonlyUsedTime` method to make sure it awaits for\r\nthe popup to be ready before clicking.\r\n* Skipped 4 tests that seem outdated, the logs don't have the related\r\nentries even after waiting for more than one minute and flushing (in\r\nfact, they all seem to systematically fail on `main` too):\r\n * lnsLegacyMetric\r\n * [Flights] Delays & Cancellations\r\n * [Flights] Destination Weather\r\n * [Flights] Delay Buckets\r\n\r\nAttached is the generated\r\n[kibana.log](https://github.com/elastic/kibana/files/12260144/kibana.log)\r\n(focussing only the `browser.ts` tests).\r\n\r\nSo for the skipped tests, this does not look like flakiness anymore, but\r\nrather outdated / incorrect checks. I propose we review and update them\r\non a separate issue / PR.\r\n\r\n50 runs results\r\n[here](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3026).","sha":"a62d9a90f53c6ebec48d4982393c637cc73c26ac"}}]}] BACKPORT--> Co-authored-by: Gerard Soldevila <[email protected]>
This was referenced Oct 1, 2023
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.
Attempt at fixing #149611
I updated the test code as follows:
setCommonlyUsedTimemethod to make sure it awaits for the popup to be ready before clicking.maintoo):Attached is the generated kibana.log (focussing only the
browser.tstests).So for the skipped tests, this does not look like flakiness anymore, but rather outdated / incorrect checks. I propose we review and update them on a separate issue / PR.
50 runs results here.