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

Fix flaky jest test #58402

Merged
merged 7 commits into from
Feb 25, 2020
Merged

Fix flaky jest test #58402

merged 7 commits into from
Feb 25, 2020

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Feb 24, 2020

We disabled some very flaky unit tests which seemed to be poisoning the modules with a bad mock.

The Jest docs say:

By default, each test file gets its own independent module registry

But it's possible these docs should have an asterisk saying that built-in modules are not scoped to individual files...

In order to avoid the problem all together, I'm just not mutating the fs module and instead returning a new object from the mock provider with the actual fs module merged into it.

@spalger spalger changed the title Fix/flaky jest test Fix flaky jest test Feb 24, 2020
Comment on lines +45 to +49
return {
...realFs,
readFile: jest.fn(realFs.readFile),
};
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately the jest tests aren't very well isolated so when trying to run them in the flaky test suite we get all sorts of errors about file system issues, http ports already bound, etc. Because of this I think we can just go off our hunch that fs isn't reset like the other modules between test files and see how it goes over time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like an alternative would be to call resetModules as a heavy handed solution. Thoughts on calling that in src/dev/jest/setup/mocks.js as defined by setupFilesAfterEnv

jestjs/jest#3577 (comment)

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'm not confident in the ramifications of a change like that; I'm not sure what problems that would cause...

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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

@spalger spalger marked this pull request as ready for review February 24, 2020 23:59
@spalger spalger requested a review from a team as a code owner February 24, 2020 23:59
@spalger spalger added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.7.0 v8.0.0 labels Feb 24, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@tylersmalley
Copy link
Contributor

diff:

45,46c45,48
<   jest.spyOn(realFs, 'readFile');
<   return realFs;
---
>   return {
>     ...realFs,
>     readFile: jest.fn(realFs.readFile),
>   };

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

LGTM

@spalger spalger merged commit 0e23636 into elastic:master Feb 25, 2020
@spalger spalger deleted the fix/flaky-jest-test branch February 25, 2020 20:59
spalger pushed a commit to spalger/kibana that referenced this pull request Feb 25, 2020
* Revert "Temporarily removes kbn-optimizer cache key tests (elastic#58318)"

This reverts commit e64eff0.

* [kbn-optmizer] avoid mocking fs exports

* overwrite ciGroup script to support jest in flaky testing job

* limit jest workers to 3 so that concurrent runners have space to operate

* Revert "limit jest workers to 3 so that concurrent runners have space to operate"

This reverts commit 1a2f882.

* Revert "overwrite ciGroup script to support jest in flaky testing job"

This reverts commit 548db61.
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 26, 2020
spalger pushed a commit that referenced this pull request Feb 27, 2020
* Revert "Temporarily removes kbn-optimizer cache key tests (#58318)"

This reverts commit e64eff0.

* [kbn-optmizer] avoid mocking fs exports

* overwrite ciGroup script to support jest in flaky testing job

* limit jest workers to 3 so that concurrent runners have space to operate

* Revert "limit jest workers to 3 so that concurrent runners have space to operate"

This reverts commit 1a2f882.

* Revert "overwrite ciGroup script to support jest in flaky testing job"

This reverts commit 548db61.
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 27, 2020
@spalger
Copy link
Contributor Author

spalger commented Feb 27, 2020

7.x/7.7: a2a0358

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.7.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants