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

Fix retryTimes and add e2e regression test #6762

Merged
merged 4 commits into from
Jul 28, 2018
Merged

Fix retryTimes and add e2e regression test #6762

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca147cd
Fix retryTimes and add e2e regression test
palmerj3 Jul 26, 2018
c607e87
Lint fixes
palmerj3 Jul 26, 2018
f834dcc
Use dispatch for resetting test errors on retry
palmerj3 Jul 27, 2018
c569c3d
Update CHANGELOG
palmerj3 Jul 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions e2e/__tests__/test_retries.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,14 @@ describe('Test Retries', () => {
fs.unlinkSync(outputFilePath);
});

it('retries failed tests if configured', () => {
it('retries failed tests', () => {
const result = runJest('test-retries', ['e2e.test.js']);

expect(result.code).toEqual(0);
expect(result.failed).toBe(false);
});

it('reporter shows more than 1 invocation if test is retried', () => {
let jsonResult;

const reporterConfig = {
Expand Down Expand Up @@ -59,7 +66,7 @@ describe('Test Retries', () => {
expect(jsonResult.testResults[0].testResults[0].invocations).toBe(4);
});

it('does not retry by default', () => {
it('reporter shows 1 invocation if tests are not retried', () => {
let jsonResult;

const reporterConfig = {
Expand Down
29 changes: 29 additions & 0 deletions e2e/test-retries/__tests__/e2e.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
'use strict';

const path = require('path');
const fs = require('fs');

const countPath = path.join(__dirname, '.tries');

beforeAll(() => {
fs.writeFileSync(countPath, '0', 'utf8');
});

jest.retryTimes(3);

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good call

});

afterAll(() => {
// cleanup
fs.unlinkSync(countPath);
});
4 changes: 4 additions & 0 deletions packages/jest-circus/src/run.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [];
Copy link
Member

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?

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

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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)

Copy link
Contributor

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

Copy link
Contributor Author

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!


await _runTest(test);
numRetriesAvailable--;
}
Expand Down