-
-
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
Fix retryTimes and add e2e regression test #6762
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6762 +/- ##
==========================================
- Coverage 63.68% 63.66% -0.03%
==========================================
Files 235 235
Lines 9007 9010 +3
Branches 3 3
==========================================
Hits 5736 5736
- Misses 3270 3273 +3
Partials 1 1
Continue to review full report at Codecov.
|
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.
Missing changelog entry
packages/jest-circus/src/run.js
Outdated
@@ -64,6 +64,10 @@ const _runTestsForDescribeBlock = async (describeBlock: DescribeBlock) => { | |||
let numRetriesAvailable = retryTimes; | |||
|
|||
while (numRetriesAvailable > 0 && test.errors.length > 0) { | |||
// Clear errors so retries occur | |||
// TODO: consider creating test.hookErrors and test.errors | |||
test.errors = []; |
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.
It would be nice to keep all errors and not just the last (in case it still errors after retrying). Thoughts?
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 don't think it's useful to keep all errors, in this context. As a user you're explicitly saying you want certain tests to be retried on failure and you'd just be interested in the errors from the last attempt.
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.
Given that you will still see output for each test (if your test throws an error or something) I think the user will have enough information already in their console to be able to diagnose problems.
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 only thing i'm worried about is modifying the data structure directly instead of dispatching an action (cause FLUX!)
i sense the possibility of wtf did my errors disappear??
in future development :)
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.
Yeah it's ultimately necessary, though, due to the checks performed in jest-circus. We need to synchronously update those errors in this case otherwise the test isn't actually re-run.
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.
If you have an architectural suggestion I'm all for it :) But right now the feature doesn't work as advertised.
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.
are you talking about the line 104 in this file?
dispatch
actually works synchronously, so if you just dispatch an action and update test
from the event_handler
it should be fine (cause it's still the same reference)
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 kind of want to experiment with redux and immutablejs and see if it affects performance. just to make sure all data structures are safe and don't get mutated in utility functions
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.
Ahhhh ok - let me try that!
it('retries', () => { | ||
const tries = parseInt(fs.readFileSync(countPath, 'utf8'), 10); | ||
fs.writeFileSync(countPath, `${tries + 1}`, 'utf8'); | ||
expect(tries).toBeGreaterThanOrEqual(2); |
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.
thinking about it, is it better to be more explicit about the number of previous tries on the final retry?
expect(tries).toEqual(3);
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.
Yeah good call
@aaronabramov if you could take another look that would be awesome :) I applied your feedback regarding dispatch. |
looks awesome!! |
Can we make a patch release of jest-circus with this change? This change was not included in the latest release I assume the intent was to include this change. |
@mjesun can you make another one? :) |
* upstream/master: (122 commits) fix: don't report promises as open handles (jestjs#6716) support serializing `DocumentFragment` (jestjs#6705) Allow test titles to include array index (jestjs#6414) fix `toContain` suggest to contain equal message (jestjs#6810) fix: testMatch not working with negations (jestjs#6648) Add test cases for jestjs#6744 (jestjs#6772) print stack trace on calls to process.exit (jestjs#6714) Updates SnapshotTesting.md to provide more info. on snapshot scope (jestjs#6735) Mark snapshots as obsolete when moved to an inline snapshot (jestjs#6773) [Docs] Clarified the use of literal values as property matchers in toMatchSnapshot() (jestjs#6807) Update CHANGELOG.md (jestjs#6799) fix changelog entry that is not in 23.4.2 (jestjs#6796) Fix --coverage with --findRelatedTests overwriting collectCoverageFrom options (jestjs#6736) Update testURL default value from about:blank to localhost (jestjs#6792) fix: `matchInlineSnapshot` when prettier dependencies are mocked (jestjs#6776) Fix retryTimes and add e2e regression test (jestjs#6762) Release v23.4.2 Docs/ExpectAPI: Correct docs for `objectContaining` (jestjs#6754) chore(packages/babel-jest) readme (jestjs#6746) docs: noted --coverage aliased by --collectCoverage (jestjs#6741) ...
@mjesun 👋 😄 |
Let's wait for merging #6815 then we'll do a minor :) |
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
Fixes #6727
retryTimes initial implementation did not actually fully retry a test on failure due to logic in jest-circus which would not run a test if previous errors occurred. This PR updates that logic to handle test retries and adds a new e2e regression test that tests more than the reporter API (invocations).
Test plan
Adds an e2e test inspired heavily by the referenced issue above.