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

[Lens][FTR] Isolate logsDB and TSDB tests into separate suites #200007

Closed
wants to merge 9 commits into from
Closed

[Lens][FTR] Isolate logsDB and TSDB tests into separate suites #200007

wants to merge 9 commits into from

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Nov 13, 2024 •
edited
Loading

Summary

This PR contains few changes to improve the FTR CI speed:

  • moved TSDB and LogsDB into a new group7
  • Merged together few tests and tweaked some methods to avoid flaky behaviour and speed up the execution.
  • Created a new existsDataStream method for the dataStream service
  • On data stream creation added an error handler when the stream exists already (this should make it easier to debug failures without running the suite multiple times)

Checklist

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens backport:skip This commit does not require backporting Feature:Functional Testing v8.17.0 labels Nov 13, 2024
@dej611
Copy link
Contributor Author

dej611 commented Nov 14, 2024

/ci

mbondyra
mbondyra reviewed Nov 15, 2024
View reviewed changes
x-pack/test/functional/page_objects/lens_page.ts
@@ -1563,7 +1563,7 @@ export function LensPageProvider({ getService, getPageObjects }: FtrProviderCont
await retry.waitFor('rendering count to stabilize', async () => {
const firstCount = await getRenderingCount();

await common.sleep(1000);
await common.sleep(500);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you run flaky test runner to make sure it doesn't cause flakiness?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a broad flaky test runner with 33x run of most of the Lens suites: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/7409

@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7409

[✅] x-pack/test/functional/apps/lens/group2/config.ts: 33/33 tests passed.
[✅] x-pack/test/functional/apps/lens/group3/config.ts: 33/33 tests passed.
[✅] x-pack/test/functional/apps/lens/group4/config.ts: 33/33 tests passed.
[✅] x-pack/test/functional/apps/lens/group5/config.ts: 33/33 tests passed.
[✅] x-pack/test/functional/apps/lens/group6/config.ts: 33/33 tests passed.

see run history

@dej611 dej611 marked this pull request as ready for review November 27, 2024 11:38
@dej611 dej611 requested review from a team as code owners November 27, 2024 11:38
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@dej611 dej611 changed the title [Lens][FTR] Merged some tests together to speedup execution [Lens][FTR] Isolate logsDB and TSDB tests into separate suites Nov 27, 2024
@kibanamachine
Copy link
Contributor

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7503

[✅] x-pack/test/functional/apps/lens/group7/config.ts: 100/100 tests passed.

see run history

@dej611 dej611 added v8.18.0 and removed v8.17.0 labels Nov 27, 2024
@dej611 dej611 mentioned this pull request Nov 27, 2024
Merged
@dej611 dej611 requested a review from mbondyra November 28, 2024 09:28
@ElenaStoeva ElenaStoeva self-requested a review November 28, 2024 13:45
ElenaStoeva
ElenaStoeva reviewed Nov 28, 2024
View reviewed changes
x-pack/test/functional/services/data_stream.ts Outdated
@@ -23,6 +23,11 @@ export function DataStreamProvider({ getService, getPageObject }: FtrProviderCon
deleteOriginal: false,
};

async function existsDataStream(stream: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this function used? I don't see a test case that uses it. Also, wouldn't it be better to call it something like getDataStream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not used anywhere yet, but initially I thought to expose it to handle errors when creating a data stream with the same name of an existing one (i.e. manual stop of a failing FTR and subsequent restart), then I've addressed the issue with a bespoken code here: https://github.com/elastic/kibana/pull/200007/files#diff-460cf76ae380343d7b854f412b3a2daa5ccde74135d55c7b383d5d1321b15661R261-R266

But I've left this method exposed as I thought it may be useful to have a specific utility to check the existence of a datastream. If you want I can remove it, no strong opinion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I would say let's remove it for now as it's not needed yet. Also, it's technically a single line of code so not sure if we need a helper function for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed with 6c0cf8e

ElenaStoeva
ElenaStoeva reviewed Nov 28, 2024
View reviewed changes
x-pack/test/functional/services/data_stream.ts
log.info(`Found resource_already_exists_exception: delete and rety again...`);
await deleteDataStream(streamIndex);
await createDataStream(streamIndex, mappings, mode);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed to add one suggestion here: if the error is not resource_already_exists_exception I think we should still throw the error so that we know if the data stream has failed to create.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with 768dcfc

ElenaStoeva
ElenaStoeva approved these changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@ElenaStoeva ElenaStoeva left a comment

Choose a reason for hiding this comment

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

Thanks for addressing my comments! Data stream service changes lgtm.

@markov00 markov00 mentioned this pull request Nov 29, 2024
Merged
@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 29, 2024 •
edited
Loading

💔 Build Failed

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #33 / ESQL execution logic API @ess @serverless ES|QL rule type @skipInServerlessMKI manual rule run does not alert if the manual run overlaps with a previous scheduled rule execution
  • [job] [logs] FTR Configs #47 / lens app - group 4 lens tsdb downsampling for regular metric defaults to median for non-rolled up metric
  • [job] [logs] FTR Configs #47 / lens app - group 4 lens tsdb downsampling for regular metric defaults to median for non-rolled up metric

Metrics [docs]

✅ unchanged

History

@dej611 dej611 closed this Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElenaStoeva ElenaStoeva ElenaStoeva approved these changes

@mbondyra mbondyra mbondyra approved these changes

@jbudz jbudz jbudz approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Functional Testing Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.18.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dej611 @kibanamachine @elasticmachine @jbudz @mbondyra @ElenaStoeva