Fix retryTimes and add e2e regression test #6762

palmerj3 · 2018-07-26T12:09:23Z

Summary

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.

codecov-io · 2018-07-26T14:33:27Z

Codecov Report

Merging #6762 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            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

Impacted Files	Coverage Δ
packages/jest-circus/src/run.js	`0% <0%> (ø)`	⬆️
packages/jest-circus/src/event_handler.js	`12.32% <0%> (-0.35%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6ebdd3d...c569c3d. Read the comment docs.

SimenB

Missing changelog entry

SimenB · 2018-07-26T16:23:49Z

packages/jest-circus/src/run.js

@@ -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 = [];


It would be nice to keep all errors and not just the last (in case it still errors after retrying). Thoughts?

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.

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.

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 :)

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.

If you have an architectural suggestion I'm all for it :) But right now the feature doesn't work as advertised.

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)

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

Ahhhh ok - let me try that!

laat · 2018-07-26T23:24:30Z

e2e/test-retries/__tests__/e2e.test.js

+it('retries', () => {
+  const tries = parseInt(fs.readFileSync(countPath, 'utf8'), 10);
+  fs.writeFileSync(countPath, `${tries + 1}`, 'utf8');
+  expect(tries).toBeGreaterThanOrEqual(2);


thinking about it, is it better to be more explicit about the number of previous tries on the final retry?

expect(tries).toEqual(3);

Yeah good call

palmerj3 · 2018-07-27T21:33:04Z

@aaronabramov if you could take another look that would be awesome :) I applied your feedback regarding dispatch.

aaronabramov · 2018-07-28T01:57:44Z

looks awesome!!

laat · 2018-07-30T10:29:22Z

Can we make a patch release of jest-circus with this change?

This change was not included in the latest release
https://unpkg.com/jest-circus@23.4.2/build/event_handler.js (no case 'test_retry' in the file)

I assume the intent was to include this change.

aaronabramov · 2018-07-30T23:49:33Z

@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) ...

laat · 2018-08-09T08:42:27Z

@mjesun 👋 😄

mjesun · 2018-08-09T09:52:22Z

Let's wait for merging #6815 then we'll do a minor :)

thymikee · 2018-08-09T09:53:59Z

@mjesun can we also squeeze this one #6802? Metro will benefit 😃

github-actions · 2021-05-12T10:07:31Z

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

Fix retryTimes and add e2e regression test

ca147cd

facebook-github-bot added the cla signed label Jul 26, 2018

Lint fixes

c607e87

palmerj3 force-pushed the fixRetryTimes branch from c69b17c to c607e87 Compare July 26, 2018 14:55

SimenB requested a review from aaronabramov July 26, 2018 16:22

SimenB requested changes Jul 26, 2018

View reviewed changes

laat reviewed Jul 26, 2018

View reviewed changes

palmerj3 added 2 commits July 26, 2018 21:44

Use dispatch for resetting test errors on retry

f834dcc

Update CHANGELOG

c569c3d

SimenB approved these changes Jul 27, 2018

View reviewed changes

thymikee approved these changes Jul 27, 2018

View reviewed changes

aaronabramov merged commit 0f525c5 into jestjs:master Jul 28, 2018

palmerj3 deleted the fixRetryTimes branch July 28, 2018 02:10

This was referenced Jul 30, 2018

Update jest in / from 23.4.1 to 23.4.2 opencomponents/oc-template-react#265

Closed

Update babel-jest in / from 23.0.1 to 23.4.2 severest/retrobot#181

Closed

Update jest in / from 23.1.0 to 23.4.2 severest/retrobot#182

Closed

This was referenced Aug 1, 2018

Update jest in /modules/api from 22.4.3 to 23.4.2 project-accelerate/accelerate#207

Open

Update jest in /modules/frontend from 22.4.3 to 23.4.2 project-accelerate/accelerate#235

Open

dependencies bot mentioned this pull request Aug 1, 2018

Update babel-jest in / from 22.1.0 to 23.4.2 leonardofaria/leozera-ui#209

Closed

dependencies bot mentioned this pull request Nov 1, 2018

Update jest in / from 22.1.4 to 23.6.0 leonardofaria/leozera-ui#250

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix retryTimes and add e2e regression test #6762

Fix retryTimes and add e2e regression test #6762

palmerj3 commented Jul 26, 2018

codecov-io commented Jul 26, 2018 •

edited

Loading

SimenB left a comment

SimenB Jul 26, 2018

palmerj3 Jul 26, 2018

palmerj3 Jul 26, 2018

aaronabramov Jul 26, 2018

palmerj3 Jul 26, 2018

palmerj3 Jul 26, 2018

aaronabramov Jul 26, 2018

aaronabramov Jul 26, 2018

palmerj3 Jul 26, 2018

laat Jul 26, 2018

palmerj3 Jul 27, 2018

palmerj3 commented Jul 27, 2018

aaronabramov commented Jul 28, 2018

laat commented Jul 30, 2018

aaronabramov commented Jul 30, 2018

laat commented Aug 9, 2018

mjesun commented Aug 9, 2018

thymikee commented Aug 9, 2018

github-actions bot commented May 12, 2021

Fix retryTimes and add e2e regression test #6762

Fix retryTimes and add e2e regression test #6762

Conversation

palmerj3 commented Jul 26, 2018

Summary

Test plan

codecov-io commented Jul 26, 2018 • edited Loading

Codecov Report

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

palmerj3 commented Jul 27, 2018

aaronabramov commented Jul 28, 2018

laat commented Jul 30, 2018

aaronabramov commented Jul 30, 2018

laat commented Aug 9, 2018

mjesun commented Aug 9, 2018

thymikee commented Aug 9, 2018

github-actions bot commented May 12, 2021

codecov-io commented Jul 26, 2018 •

edited

Loading