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

[Migrations] Fix integration tests that use ZIP archives that are not compatible with 9.0.0 #193856

Merged
merged 8 commits into from
Sep 26, 2024
Merged

[Migrations] Fix integration tests that use ZIP archives that are not compatible with 9.0.0 #193856

merged 8 commits into from
Sep 26, 2024

Conversation

gsoldevila
Copy link
Contributor

@gsoldevila gsoldevila commented Sep 24, 2024 •
edited
Loading

@gsoldevila gsoldevila added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes Feature:Migrations labels Sep 24, 2024
@gsoldevila gsoldevila requested a review from a team as a code owner September 24, 2024 12:09
@elasticmachine
Copy link
Contributor

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

@gsoldevila gsoldevila added the backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development label Sep 24, 2024
afharo
afharo approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM. I just added a few nits for your consideration.

src/core/server/integration_tests/saved_objects/migrations/group2/multi_node_split.test.ts Outdated
@@ -0,0 +1,292 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

nit: this one might need its own group, as starting multiple Kibanas, with delays increase the execution time, and the chances of failing due to timeouts (and retrying the whole CI job).

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 already has its own group, there aren't any other tests in group2 anymore 😛 .
I will increase jest timeout though, thanks for raising the poing

src/core/server/integration_tests/saved_objects/migrations/group2/multi_node_split.test.ts Outdated
Comment on lines 77 to 88
afterEach(async () => {
checkMigratorsResults();
await checkIndicesInfo();
await checkSavedObjectDocuments();
await checkMigratorsSteps();
await checkUpToDateOnRestart();
});

afterEach(async () => {
await esServer?.stop();
await delay(5); // give it a few seconds... cause we always do ¯\_(ツ)_/¯
});
Copy link
Member

Choose a reason for hiding this comment

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

2 afterEach where esServer can be stopped before completing the checks in the other afterEach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that's a good catch, will put them together + finally block.

src/core/server/integration_tests/saved_objects/migrations/group2/multi_node_split.test.ts Outdated
Comment on lines 77 to 83
afterEach(async () => {
checkMigratorsResults();
await checkIndicesInfo();
await checkSavedObjectDocuments();
await checkMigratorsSteps();
await checkUpToDateOnRestart();
});
Copy link
Member

Choose a reason for hiding this comment

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

I love the shared piece of code, but it feels weird to run the validations in the after phase.

I wonder if an it.each with a table format (description and delay) that runs startWithDelay + these checks is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea, will update it

src/core/server/integration_tests/saved_objects/migrations/group2/multi_node_split.test.ts Outdated
@@ -0,0 +1,292 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
Copy link
Member

Choose a reason for hiding this comment

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

nit: there's another file multiple_es_nodes. Should we specify this one to be multi_kb_node_split?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Accepted proposal, will rename to to multiple_kb_nodes

@gsoldevila gsoldevila added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:prev-major Backport to (8.x, 8.17, 8.16) the previous major branch and other branches in development labels Sep 24, 2024
@gsoldevila gsoldevila changed the title [Migrations] Create multi_node_split test [Migrations] Create multiple_kb_nodes test Sep 24, 2024
@gsoldevila gsoldevila changed the title [Migrations] Create multiple_kb_nodes test [Migrations] Fix integration tests that use ZIP archives that are not compatible with 9.0.0 Sep 25, 2024
@gsoldevila gsoldevila added backport:skip This commit does not require backporting and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Sep 26, 2024
@gsoldevila gsoldevila enabled auto-merge (squash) September 26, 2024 08:09
@gsoldevila
Copy link
Contributor Author

Already backported with #194013

@afharo
Copy link
Member

afharo commented Sep 26, 2024 •
edited
Loading

Already backported with #194013

I think you meant to post that comment in #193328, right?

@gsoldevila gsoldevila mentioned this pull request Sep 26, 2024
Merged
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Integration Tests #2 / migration actions reindex & waitForReindexTask resolves left wait_for_task_completion_timeout when the task does not finish within the timeout

Metrics [docs]

✅ unchanged

History

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

@gsoldevila gsoldevila merged commit ac96a4c into elastic:main Sep 26, 2024
25 checks passed
@gsoldevila
Copy link
Contributor Author

I think you meant to post that comment in #193328, right?

In fact #194013 will backport multiple of the tests' updates:

There are dependencies between them, and on top of that I need to generate the 8.16.0 baseline zip archives (wrote a test for that). If I backport them one by one it'll take ages.

@gsoldevila gsoldevila mentioned this pull request Sep 26, 2024
Closed
This was referenced Sep 26, 2024
Closed
Closed
Closed
Open
Closed
Closed
Closed
Closed
gsoldevila added a commit that referenced this pull request Sep 27, 2024
@gsoldevila
[8.x] [Migrations] Backport ALL updates to integration tests (#194013)
c7b29d1 
# Backport

This will backport the following PRs from `main` to `8.x`:
 - #193328
 - #193856
 - #193696
 - #194151
@gsoldevila gsoldevila mentioned this pull request Oct 1, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@afharo afharo afharo approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:Migrations 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 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gsoldevila @elasticmachine @afharo @kibana-ci @kibanamachine