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

[Remote clusters] Reorganize test files #82362

Merged
merged 5 commits into from
Nov 3, 2020
Merged

[Remote clusters] Reorganize test files #82362

merged 5 commits into from
Nov 3, 2020

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Nov 2, 2020 •
edited
Loading

I reorganized the test files to improve readability in two ways:

  • I colocated the helper files with their test files to make them easier to find. This follows the pattern in Index Management.
  • I removed the default API responses from the HTTP response mocking functions to make their usage and expected behavior clearer at call sites.

@cjcenizal
Refactor Remote Clusters client integration tests for readability.
0502482 
- Colocate helpers with test files.
- Remove default API responses from HTTP response mocking functions to make behavior clearer at call sites.
@cjcenizal
Remove await nextTick, which was causing flakiness.
8ee1674
@cjcenizal cjcenizal added Feature:CCR and Remote Clusters v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more release_note:skip Skip the PR/issue when compiling release notes v7.11.0 labels Nov 2, 2020
@cjcenizal cjcenizal requested a review from a team as a code owner November 2, 2020 19:34
@elasticmachine
Copy link
Contributor

Pinging @elastic/es-ui (Team:Elasticsearch UI)

alisonelizabeth
alisonelizabeth reviewed Nov 3, 2020
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth 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 taking this on @cjcenizal!

I left a comment in the code regarding the use of waitFor - @sebelga determined that it is not good practice to use this anymore in tests. I opened #82393 to update the test_utils readme with some best practices that Seb shared awhile back.

I think the remote clusters component integration tests as a whole probably need to be refactored to fix the flakiness and remove the use of waitFor introduced in #58768. I'm happy to take this over if you like. I recently did similar work for the ingest node pipeline tests in #78838.

x-pack/plugins/remote_clusters/__jest__/client_integration/list/remote_clusters_list.test.js Outdated

beforeEach(async () => {
({ exists, component } = setup());
({ waitFor, exists } = setup());
Copy link
Contributor

Choose a reason for hiding this comment

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

We determined that it is not best practice to use the waitFor helper in tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, no more waitFor in our jest tests 🙏

@cjcenizal cjcenizal changed the title [Remote clusters] Fix flaky "empty prompt" jest test and reorganize test files [Remote clusters] Reorganize test files Nov 3, 2020
@cjcenizal
Copy link
Contributor Author

@elasticmachine merge upstream

@cjcenizal
Copy link
Contributor Author

cjcenizal commented Nov 3, 2020 •
edited
Loading

Thanks @alisonelizabeth! I reverted that commit, so this PR is now limited to reorganizing the test files only. Could you please take another look?

alisonelizabeth
alisonelizabeth approved these changes Nov 3, 2020
View reviewed changes
Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Thanks @cjcenizal! The reorganization LGTM. I'm approving to not block you, but I think there is one line (left a comment) that should be reverted until a better assessment is made as to what needs to be refactored in the tests.

x-pack/plugins/remote_clusters/__jest__/client_integration/list/remote_clusters_list.test.js Outdated
@@ -56,7 +49,7 @@ describe('<RemoteClusterList />', () => {
beforeEach(async () => {
({ exists, component } = setup());

await nextTick(100); // We need to wait next tick for the mock server response to kick in
// await nextTick(100); // We need to wait next tick for the mock server response to kick in
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you uncomment this line for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, that was a leftover!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@cjcenizal cjcenizal merged commit b770e82 into elastic:master Nov 3, 2020
@cjcenizal cjcenizal deleted the refactor/remote-clusters-tests branch November 3, 2020 18:57
@cjcenizal cjcenizal mentioned this pull request Nov 3, 2020
Merged
cjcenizal added a commit that referenced this pull request Nov 4, 2020
@cjcenizal
[Remote clusters] Reorganize test files (#82362) (#82520)
266893d 
* Refactor Remote Clusters client integration tests for readability.
- Colocate helpers with test files.
- Remove default API responses from HTTP response mocking functions to make behavior clearer at call sites.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebelga sebelga sebelga left review comments

@alisonelizabeth alisonelizabeth alisonelizabeth approved these changes

Assignees
No one assigned
Labels
Feature:CCR and Remote Clusters release_note:skip Skip the PR/issue when compiling release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cjcenizal @elasticmachine @kibanamachine @sebelga @alisonelizabeth