-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Test retries #6498
Test retries #6498
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6498 +/- ##
==========================================
- Coverage 63.83% 63.75% -0.09%
==========================================
Files 234 234
Lines 8797 8810 +13
Branches 3 3
==========================================
+ Hits 5616 5617 +1
- Misses 3180 3192 +12
Partials 1 1
Continue to review full report at Codecov.
|
packages/jest-cli/src/cli/args.js
Outdated
@@ -51,6 +51,17 @@ export const check = (argv: Argv) => { | |||
); | |||
} | |||
|
|||
const intRetries = parseInt(argv.testRetries, 10); | |||
if ( | |||
(argv.hasOwnProperty('testRetries') && isNaN(intRetries)) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Number.isNaN
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea! Making Jest more useful for integration tests would be awesome.
This is missing tests, docs and a changelog entry, but the code itself LGTM 🙂
packages/jest-circus/src/run.js
Outdated
@@ -28,30 +28,41 @@ import { | |||
|
|||
const Promise = getOriginalPromise(); | |||
|
|||
const run = async (): Promise<RunResult> => { | |||
const run = async (numRetries: number): Promise<RunResult> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we stay consistent with the naming and use testRetries
instead?
Great! I'll make these changes today and update the PR. |
Also I have code working locally which updates the reporter API adding a new field for TestResults called invocations. I'm not sure how useful it would be so I haven't pushed it up. But I like the idea of a reporter being able to track which tests are retried since it's a signal about potential flakiness. Let me know your thoughts on that. I'm happy to include that if you think it's worthwhile. |
Yeah seems useful. Can use it to inform about flakiness over time |
i was also thinking about
the reason is that sometimes when you run e2e integration tests there's can be a spike in CPU usage (another thread decided to spawn something?) and the test times out. If you retry it right away the spike might be still going on and it'll fail again, but if you first try to execute everything else, see what passes, and then re-execute tests that failed you might have enough time for the CPU spike to die down. |
also.. i noticed when executing fat integration tests, sometimes they timeout, jest moves on to the next test and the previous test eventually throws something. Jest treats it as a global error (because it does not know where it came from) and fails whatever current test is running, which results in a pretty confusing behavior. |
@aaronabramov awesome! Is test.retry something you want me to implement as part of this or entirely to replace the CLI/Config-based approach? Also regarding errors thrown after tests - not really sure the best way to make tests run later on. Push to the end of the test array? How would I do that? |
@palmerj3 those we just my observations from last time i tried it! we can definitely iterate on those ideas later! jest.retryTimes(3); or something like that this way we can still set it globally from a |
Awesome - you've given me a different perspective for approaching this problem. I'm going to try a few things out (jest obj, test.retry, etc) and see what comes of it. Thanks! |
@aaronabramov I guess the thing I'm confused about is how I would responsibly go about retrying tests via the jest object? I can easily set a global to know how many retries are required in a given context but the act of actually retrying would have to be implemented by the test runner unless I override the test/it global and orchestrate from jest-runtime. |
@palmerj3 yeah, i was thinking about exactly how |
Ok cool that's how I'll do it |
@aaronabramov @SimenB if you get a chance please review again. CI seems to be failing and I'm having a hard time debugging that... tests all pass locally on node 9 and node 10 so I'm not understanding why things fail on CI. That said: I updated the PR so test retries are configured via the Jest object and I updated the reporter API so "invocations" field is available in TestResult. Take a look when you can and offer ideas on how to fix CI :) |
packages/jest-runtime/src/index.js
Outdated
@@ -830,6 +830,11 @@ class Runtime { | |||
return jestObject; | |||
}; | |||
|
|||
const retryTimes = (numTestRetries: number) => { | |||
this._environment.global.retryTimes = numTestRetries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we put it on the jest
object instead of as a global?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if that will work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimenB so I don't think that's going to work. From the context of jest-circus/run.js I don't have access to the jest object. Even if I did I think I would have to add both a getter and a setter for that to work. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a clean (and private) way of speaking across that boundary would be awesome, but no need to tackle it at this moment 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last time i did it with a Symbol
that is attached to global
. which is pretty much the same thing, but you only know it's there if you know what you're looking for :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we do that? Polluting the global namespace in user code seems icky
EDIT: "that" being the symbol Aaron suggested, since this comment was not kept in context...
e2e/__tests__/test_retries.test.js
Outdated
runJest( | ||
'test-retries', | ||
['--config', JSON.stringify(reporterConfig), 'retry.test.js'], | ||
{useJestCircus: true}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can do skipOnJasmine
instead, then it will run just with circus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah perfect thanks!
@@ -312,6 +313,21 @@ test('works too', () => { | |||
|
|||
Returns the `jest` object for chaining. | |||
|
|||
### `jest.retryTimes()` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should mention it just works with circus
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mention in that section that it works with jest-circus. Should I be more explicit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is it mentioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah my mistake sorry. I mentioned it in the changelog not the docs. Will update.
e2e/runJest.js
Outdated
@@ -21,6 +21,7 @@ const JEST_PATH = path.resolve(__dirname, '../packages/jest-cli/bin/jest.js'); | |||
type RunJestOptions = { | |||
nodePath?: string, | |||
skipPkgJsonCheck?: boolean, // don't complain if can't find package.json | |||
useJestCircus?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need the changes in this file now, right?
@aaronabramov @SimenB I think the PR is ready. I've addressed all the feedback so far. But I'm getting a weird (unrelated?) error on some CI checks that I can't make sense of. All things pass locally on node 9 and node 10.
Something related to graceful-fs |
e2e/__tests__/test_retries.test.js
Outdated
], | ||
}; | ||
|
||
// Test retries only available via JEST_CIRCUS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
outdated comment, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Pushing a commit now to fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmm :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All set - still getting CI errors due to the graceful-fs dependency bump yesterday (I believe)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is still there :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failures are not because of graceful-fs
bump (there wasn't any afaik), but started to happen since Node 10.5
e2e/runJest.js
Outdated
@@ -49,12 +49,15 @@ function runJest( | |||
); | |||
} | |||
|
|||
const env = options.nodePath | |||
? Object.assign({}, process.env, { | |||
const nodePathOverrides = options.nodePath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can revert that back to the original
I really need to learn how to software :) Outdated comments removed. |
@@ -8,3 +8,4 @@ | |||
|
|||
// prettier-ignore | |||
test('escape strings', () => expect('one: \\\'').toMatchSnapshot()); | |||
test('escape strings two', () => expect('two: \'"').toMatchSnapshot()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this line is not supposed to be committed.
i believe it's coming from our old janky snapshot tests that modifies our fixtures (that are under git and not in /tmp) and if you interrupt the test before it's done this thing will stay there :( (hence all other modifications to snapshots you had to make)
check e2e/__tests__/snapshot.test.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok - should I remove this file or that line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw other PRs failing with the same error
packages/jest-runtime/src/index.js
Outdated
@@ -830,6 +830,11 @@ class Runtime { | |||
return jestObject; | |||
}; | |||
|
|||
const retryTimes = (numTestRetries: number) => { | |||
this._environment.global.retryTimes = numTestRetries; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last time i did it with a Symbol
that is attached to global
. which is pretty much the same thing, but you only know it's there if you know what you're looking for :)
@aaronabramov so I removed that one test case but still getting the same error. Happy to revert that commit if you like. But if you have other ideas for things I can check I can do that too. |
@SimenB all set |
Merge? :) |
FB employees typically merge the larger changes :) |
@aaronabramov any more thoughts on this? No rush on my end - would just rather not have to rebase again :) |
it looks good to me! |
This Pull Request updates dependency [jest](https://github.com/facebook/jest) from `v23.1.0` to `v23.2.0` <details> <summary>Release Notes</summary> ### [`v23.2.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#​2320) [Compare Source](jestjs/jest@v23.1.0...v23.2.0) ##### Features - `[jest-each]` Add support for keyPaths in test titles ([#​6457](`https://github.com/facebook/jest/pull/6457`)) - `[jest-cli]` Add `jest --init` option that generates a basic configuration file with a short description for each option ([#​6442](`https://github.com/facebook/jest/pull/6442`)) - `[jest.retryTimes]` Add `jest.retryTimes()` option that allows failed tests to be retried n-times when using jest-circus. ([#​6498](`https://github.com/facebook/jest/pull/6498`)) ##### Fixes - `[jest-cli]` Add check to make sure one or more tests have run before notifying when using `--notify` ([#​6495](`https://github.com/facebook/jest/pull/6495`)) - `[jest-cli]` Pass `globalConfig` as a parameter to `globalSetup` and `globalTeardown` functions ([#​6486](`https://github.com/facebook/jest/pull/6486`)) - `[jest-config]` Add missing options to the `defaults` object ([#​6428](`https://github.com/facebook/jest/pull/6428`)) - `[expect]` Using symbolic property names in arrays no longer causes the `toEqual` matcher to fail ([#​6391](`https://github.com/facebook/jest/pull/6391`)) - `[expect]` `toEqual` no longer tries to compare non-enumerable symbolic properties, to be consistent with non-symbolic properties. ([#​6398](`https://github.com/facebook/jest/pull/6398`)) - `[jest-util]` `console.timeEnd` now properly log elapsed time in milliseconds. ([#​6456](`https://github.com/facebook/jest/pull/6456`)) - `[jest-mock]` Fix `MockNativeMethods` access in react-native `jest.mock()` ([#​6505](`https://github.com/facebook/jest/pull/6505`)) ##### Chore & Maintenance - `[docs]` Add jest-each docs for 1 dimensional arrays ([#​6444](`https://github.com/facebook/jest/pull/6444`/files)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This PR introduces a new feature to Jest that allows you to re-run failed tests automatically n-times.
This is configurable via the Jest object.
Example:
jest.retryTimes(3);
By default Jest will not re-run any failed tests.
The rationale behind this feature is that many teams use Jest for Integration and E2E tests. Higher level tests are much more likely to be flaky, and flakiness is often not the fault of the test itself (e.g. network problems).
This option provides the user a much better experience in CI since tests can be retried a few times before failing. The current behavior requires entire CI jobs to be re-triggered when a single test fails.
This PR also introduces a new field in TestResults for reporters to make use of. It is called "invocations" and represents the number of times a given test was executed.
This functionality is only available if you use jest-circus!
Test plan
Added a e2e test to ensure that tests are only re-run when jest.retryTimes is set and jest-circus is used.