From ca147cd670f30ad03e495a2416ec39a680d43482 Mon Sep 17 00:00:00 2001 From: Jason Palmer Date: Thu, 26 Jul 2018 08:05:17 -0400 Subject: [PATCH 1/4] Fix retryTimes and add e2e regression test --- e2e/__tests__/test_retries.test.js | 15 ++++-- e2e/test-retries/__tests__/e2e.test.js | 29 +++++++++++ packages/jest-circus/src/run.js | 72 ++++++++++++++------------ 3 files changed, 78 insertions(+), 38 deletions(-) create mode 100644 e2e/test-retries/__tests__/e2e.test.js diff --git a/e2e/__tests__/test_retries.test.js b/e2e/__tests__/test_retries.test.js index 4d20afe36425..fa7f1159aceb 100644 --- a/e2e/__tests__/test_retries.test.js +++ b/e2e/__tests__/test_retries.test.js @@ -28,12 +28,19 @@ 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 = { reporters: [ - ['/reporters/RetryReporter.js', {output: outputFilePath}], + ['/reporters/RetryReporter.js', { output: outputFilePath }], ], }; @@ -59,12 +66,12 @@ 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 = { reporters: [ - ['/reporters/RetryReporter.js', {output: outputFilePath}], + ['/reporters/RetryReporter.js', { output: outputFilePath }], ], }; diff --git a/e2e/test-retries/__tests__/e2e.test.js b/e2e/test-retries/__tests__/e2e.test.js new file mode 100644 index 000000000000..e9d0e08131b7 --- /dev/null +++ b/e2e/test-retries/__tests__/e2e.test.js @@ -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); +}); + +afterAll(() => { + // cleanup + fs.unlinkSync(countPath); +}); diff --git a/packages/jest-circus/src/run.js b/packages/jest-circus/src/run.js index 0853cb31f67e..c3da55f0ada2 100644 --- a/packages/jest-circus/src/run.js +++ b/packages/jest-circus/src/run.js @@ -9,13 +9,13 @@ import type { RunResult, - TestEntry, - TestContext, - Hook, - DescribeBlock, + TestEntry, + TestContext, + Hook, + DescribeBlock, } from 'types/Circus'; -import {getState, dispatch} from './state'; +import { getState, dispatch } from './state'; import { callAsyncFn, getAllHooksForDescribe, @@ -29,10 +29,10 @@ import { const Promise = getOriginalPromise(); const run = async (): Promise => { - const {rootDescribeBlock} = getState(); - dispatch({name: 'run_start'}); + const { rootDescribeBlock } = getState(); + dispatch({ name: 'run_start' }); await _runTestsForDescribeBlock(rootDescribeBlock); - dispatch({name: 'run_finish'}); + dispatch({ name: 'run_finish' }); return makeRunResult( getState().rootDescribeBlock, getState().unhandledErrors, @@ -40,11 +40,11 @@ const run = async (): Promise => { }; const _runTestsForDescribeBlock = async (describeBlock: DescribeBlock) => { - dispatch({describeBlock, name: 'run_describe_start'}); - const {beforeAll, afterAll} = getAllHooksForDescribe(describeBlock); + dispatch({ describeBlock, name: 'run_describe_start' }); + const { beforeAll, afterAll } = getAllHooksForDescribe(describeBlock); for (const hook of beforeAll) { - await _callHook({describeBlock, hook}); + await _callHook({ describeBlock, hook }); } // Tests that fail and are retried we run after other tests @@ -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 = []; + await _runTest(test); numRetriesAvailable--; } @@ -74,15 +78,15 @@ const _runTestsForDescribeBlock = async (describeBlock: DescribeBlock) => { } for (const hook of afterAll) { - await _callHook({describeBlock, hook}); + await _callHook({ describeBlock, hook }); } - dispatch({describeBlock, name: 'run_describe_finish'}); + dispatch({ describeBlock, name: 'run_describe_finish' }); }; const _runTest = async (test: TestEntry): Promise => { - dispatch({name: 'test_start', test}); + dispatch({ name: 'test_start', test }); const testContext = Object.create(null); - const {hasFocusedTests, testNamePattern} = getState(); + const { hasFocusedTests, testNamePattern } = getState(); const isSkipped = test.mode === 'skip' || @@ -90,11 +94,11 @@ const _runTest = async (test: TestEntry): Promise => { (testNamePattern && !testNamePattern.test(getTestID(test))); if (isSkipped) { - dispatch({name: 'test_skip', test}); + dispatch({ name: 'test_skip', test }); return; } - const {afterEach, beforeEach} = getEachHooksForTest(test); + const { afterEach, beforeEach } = getEachHooksForTest(test); for (const hook of beforeEach) { if (test.errors.length) { @@ -102,19 +106,19 @@ const _runTest = async (test: TestEntry): Promise => { // hooks after that. break; } - await _callHook({hook, test, testContext}); + await _callHook({ hook, test, testContext }); } await _callTest(test, testContext); for (const hook of afterEach) { - await _callHook({hook, test, testContext}); + await _callHook({ hook, test, testContext }); } // `afterAll` hooks should not affect test status (pass or fail), because if // we had a global `afterAll` hook it would block all existing tests until // this hook is executed. So we dispatche `test_done` right away. - dispatch({name: 'test_done', test}); + dispatch({ name: 'test_done', test }); }; const _callHook = ({ @@ -123,25 +127,25 @@ const _callHook = ({ describeBlock, testContext, }: { - hook: Hook, - describeBlock?: DescribeBlock, - test?: TestEntry, - testContext?: TestContext, -}): Promise => { - dispatch({hook, name: 'hook_start'}); + hook: Hook, + describeBlock?: DescribeBlock, + test?: TestEntry, + testContext?: TestContext, + }): Promise => { + dispatch({ hook, name: 'hook_start' }); const timeout = hook.timeout || getState().testTimeout; - return callAsyncFn(hook.fn, testContext, {isHook: true, timeout}) - .then(() => dispatch({describeBlock, hook, name: 'hook_success', test})) + return callAsyncFn(hook.fn, testContext, { isHook: true, timeout }) + .then(() => dispatch({ describeBlock, hook, name: 'hook_success', test })) .catch(error => - dispatch({describeBlock, error, hook, name: 'hook_failure', test}), - ); + dispatch({ describeBlock, error, hook, name: 'hook_failure', test }), + ); }; const _callTest = async ( test: TestEntry, testContext: TestContext, ): Promise => { - dispatch({name: 'test_fn_start', test}); + dispatch({ name: 'test_fn_start', test }); const timeout = test.timeout || getState().testTimeout; invariant(test.fn, `Tests with no 'fn' should have 'mode' set to 'skipped'`); @@ -150,9 +154,9 @@ const _callTest = async ( return; } - await callAsyncFn(test.fn, testContext, {isHook: false, timeout}) - .then(() => dispatch({name: 'test_fn_success', test})) - .catch(error => dispatch({error, name: 'test_fn_failure', test})); + await callAsyncFn(test.fn, testContext, { isHook: false, timeout }) + .then(() => dispatch({ name: 'test_fn_success', test })) + .catch(error => dispatch({ error, name: 'test_fn_failure', test })); }; export default run; From c607e87762d42587f1b2972e1d06bea242e53627 Mon Sep 17 00:00:00 2001 From: Jason Palmer Date: Thu, 26 Jul 2018 10:11:12 -0400 Subject: [PATCH 2/4] Lint fixes --- e2e/__tests__/test_retries.test.js | 4 +- packages/jest-circus/src/run.js | 68 +++++++++++++++--------------- 2 files changed, 36 insertions(+), 36 deletions(-) diff --git a/e2e/__tests__/test_retries.test.js b/e2e/__tests__/test_retries.test.js index fa7f1159aceb..9c2a7347212c 100644 --- a/e2e/__tests__/test_retries.test.js +++ b/e2e/__tests__/test_retries.test.js @@ -40,7 +40,7 @@ describe('Test Retries', () => { const reporterConfig = { reporters: [ - ['/reporters/RetryReporter.js', { output: outputFilePath }], + ['/reporters/RetryReporter.js', {output: outputFilePath}], ], }; @@ -71,7 +71,7 @@ describe('Test Retries', () => { const reporterConfig = { reporters: [ - ['/reporters/RetryReporter.js', { output: outputFilePath }], + ['/reporters/RetryReporter.js', {output: outputFilePath}], ], }; diff --git a/packages/jest-circus/src/run.js b/packages/jest-circus/src/run.js index c3da55f0ada2..6599911835ea 100644 --- a/packages/jest-circus/src/run.js +++ b/packages/jest-circus/src/run.js @@ -9,13 +9,13 @@ import type { RunResult, - TestEntry, - TestContext, - Hook, - DescribeBlock, + TestEntry, + TestContext, + Hook, + DescribeBlock, } from 'types/Circus'; -import { getState, dispatch } from './state'; +import {getState, dispatch} from './state'; import { callAsyncFn, getAllHooksForDescribe, @@ -29,10 +29,10 @@ import { const Promise = getOriginalPromise(); const run = async (): Promise => { - const { rootDescribeBlock } = getState(); - dispatch({ name: 'run_start' }); + const {rootDescribeBlock} = getState(); + dispatch({name: 'run_start'}); await _runTestsForDescribeBlock(rootDescribeBlock); - dispatch({ name: 'run_finish' }); + dispatch({name: 'run_finish'}); return makeRunResult( getState().rootDescribeBlock, getState().unhandledErrors, @@ -40,11 +40,11 @@ const run = async (): Promise => { }; const _runTestsForDescribeBlock = async (describeBlock: DescribeBlock) => { - dispatch({ describeBlock, name: 'run_describe_start' }); - const { beforeAll, afterAll } = getAllHooksForDescribe(describeBlock); + dispatch({describeBlock, name: 'run_describe_start'}); + const {beforeAll, afterAll} = getAllHooksForDescribe(describeBlock); for (const hook of beforeAll) { - await _callHook({ describeBlock, hook }); + await _callHook({describeBlock, hook}); } // Tests that fail and are retried we run after other tests @@ -78,15 +78,15 @@ const _runTestsForDescribeBlock = async (describeBlock: DescribeBlock) => { } for (const hook of afterAll) { - await _callHook({ describeBlock, hook }); + await _callHook({describeBlock, hook}); } - dispatch({ describeBlock, name: 'run_describe_finish' }); + dispatch({describeBlock, name: 'run_describe_finish'}); }; const _runTest = async (test: TestEntry): Promise => { - dispatch({ name: 'test_start', test }); + dispatch({name: 'test_start', test}); const testContext = Object.create(null); - const { hasFocusedTests, testNamePattern } = getState(); + const {hasFocusedTests, testNamePattern} = getState(); const isSkipped = test.mode === 'skip' || @@ -94,11 +94,11 @@ const _runTest = async (test: TestEntry): Promise => { (testNamePattern && !testNamePattern.test(getTestID(test))); if (isSkipped) { - dispatch({ name: 'test_skip', test }); + dispatch({name: 'test_skip', test}); return; } - const { afterEach, beforeEach } = getEachHooksForTest(test); + const {afterEach, beforeEach} = getEachHooksForTest(test); for (const hook of beforeEach) { if (test.errors.length) { @@ -106,19 +106,19 @@ const _runTest = async (test: TestEntry): Promise => { // hooks after that. break; } - await _callHook({ hook, test, testContext }); + await _callHook({hook, test, testContext}); } await _callTest(test, testContext); for (const hook of afterEach) { - await _callHook({ hook, test, testContext }); + await _callHook({hook, test, testContext}); } // `afterAll` hooks should not affect test status (pass or fail), because if // we had a global `afterAll` hook it would block all existing tests until // this hook is executed. So we dispatche `test_done` right away. - dispatch({ name: 'test_done', test }); + dispatch({name: 'test_done', test}); }; const _callHook = ({ @@ -127,25 +127,25 @@ const _callHook = ({ describeBlock, testContext, }: { - hook: Hook, - describeBlock?: DescribeBlock, - test?: TestEntry, - testContext?: TestContext, - }): Promise => { - dispatch({ hook, name: 'hook_start' }); + hook: Hook, + describeBlock?: DescribeBlock, + test?: TestEntry, + testContext?: TestContext, +}): Promise => { + dispatch({hook, name: 'hook_start'}); const timeout = hook.timeout || getState().testTimeout; - return callAsyncFn(hook.fn, testContext, { isHook: true, timeout }) - .then(() => dispatch({ describeBlock, hook, name: 'hook_success', test })) + return callAsyncFn(hook.fn, testContext, {isHook: true, timeout}) + .then(() => dispatch({describeBlock, hook, name: 'hook_success', test})) .catch(error => - dispatch({ describeBlock, error, hook, name: 'hook_failure', test }), - ); + dispatch({describeBlock, error, hook, name: 'hook_failure', test}), + ); }; const _callTest = async ( test: TestEntry, testContext: TestContext, ): Promise => { - dispatch({ name: 'test_fn_start', test }); + dispatch({name: 'test_fn_start', test}); const timeout = test.timeout || getState().testTimeout; invariant(test.fn, `Tests with no 'fn' should have 'mode' set to 'skipped'`); @@ -154,9 +154,9 @@ const _callTest = async ( return; } - await callAsyncFn(test.fn, testContext, { isHook: false, timeout }) - .then(() => dispatch({ name: 'test_fn_success', test })) - .catch(error => dispatch({ error, name: 'test_fn_failure', test })); + await callAsyncFn(test.fn, testContext, {isHook: false, timeout}) + .then(() => dispatch({name: 'test_fn_success', test})) + .catch(error => dispatch({error, name: 'test_fn_failure', test})); }; export default run; From f834dcc20bb2e8aeada8969bd4ccc21c233f54a5 Mon Sep 17 00:00:00 2001 From: Jason Palmer Date: Thu, 26 Jul 2018 21:44:45 -0400 Subject: [PATCH 3/4] Use dispatch for resetting test errors on retry --- e2e/test-retries/__tests__/e2e.test.js | 2 +- packages/jest-circus/src/__mocks__/test_event_handler.js | 1 + packages/jest-circus/src/event_handler.js | 4 ++++ packages/jest-circus/src/run.js | 3 +-- types/Circus.js | 4 ++++ 5 files changed, 11 insertions(+), 3 deletions(-) diff --git a/e2e/test-retries/__tests__/e2e.test.js b/e2e/test-retries/__tests__/e2e.test.js index e9d0e08131b7..ba34c2616d67 100644 --- a/e2e/test-retries/__tests__/e2e.test.js +++ b/e2e/test-retries/__tests__/e2e.test.js @@ -20,7 +20,7 @@ jest.retryTimes(3); it('retries', () => { const tries = parseInt(fs.readFileSync(countPath, 'utf8'), 10); fs.writeFileSync(countPath, `${tries + 1}`, 'utf8'); - expect(tries).toBeGreaterThanOrEqual(2); + expect(tries).toEqual(3); }); afterAll(() => { diff --git a/packages/jest-circus/src/__mocks__/test_event_handler.js b/packages/jest-circus/src/__mocks__/test_event_handler.js index 137327513d5b..ae61a8a9c3bb 100644 --- a/packages/jest-circus/src/__mocks__/test_event_handler.js +++ b/packages/jest-circus/src/__mocks__/test_event_handler.js @@ -25,6 +25,7 @@ const testEventHandler: EventHandler = (event, state) => { break; } case 'test_start': + case 'test_retry': case 'test_done': { console.log(event.name + ':', event.test.name); break; diff --git a/packages/jest-circus/src/event_handler.js b/packages/jest-circus/src/event_handler.js index c30537c14e77..e50e1f56548d 100644 --- a/packages/jest-circus/src/event_handler.js +++ b/packages/jest-circus/src/event_handler.js @@ -125,6 +125,10 @@ const handler: EventHandler = (event, state): void => { event.test.errors.push([error, asyncError]); break; } + case 'test_retry': { + event.test.errors = []; + break; + } case 'run_start': { global[TEST_TIMEOUT_SYMBOL] && (state.testTimeout = global[TEST_TIMEOUT_SYMBOL]); diff --git a/packages/jest-circus/src/run.js b/packages/jest-circus/src/run.js index 6599911835ea..1a05d7e45834 100644 --- a/packages/jest-circus/src/run.js +++ b/packages/jest-circus/src/run.js @@ -65,8 +65,7 @@ const _runTestsForDescribeBlock = async (describeBlock: DescribeBlock) => { while (numRetriesAvailable > 0 && test.errors.length > 0) { // Clear errors so retries occur - // TODO: consider creating test.hookErrors and test.errors - test.errors = []; + dispatch({name: 'test_retry', test}); await _runTest(test); numRetriesAvailable--; diff --git a/types/Circus.js b/types/Circus.js index 6490cf24932f..484afbde9656 100644 --- a/types/Circus.js +++ b/types/Circus.js @@ -91,6 +91,10 @@ export type Event = error: Exception, test: TestEntry, |} + | {| + name: 'test_retry', + test: TestEntry, + |} | {| // the `test` in this case is all hooks + it/test function, not just the // function passed to `it/test` From c569c3da6f282220e666c4ee6a2e5bdc54e2fe31 Mon Sep 17 00:00:00 2001 From: Jason Palmer Date: Thu, 26 Jul 2018 21:47:08 -0400 Subject: [PATCH 4/4] Update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f7b768baabc..464b490ce620 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ ### Fixes +- `[jest-circus]` Fix retryTimes so errors are reset before re-running ([#6762](https://github.com/facebook/jest/pull/6762)) - `[docs]` Update `expect.objectContaining()` description ([#6754](https://github.com/facebook/jest/pull/6754)) - `[babel-jest]` Make `getCacheKey()` take into account `createTransformer` options ([#6699](https://github.com/facebook/jest/pull/6699)) - `[docs]` Fix contributors link ([#6711](https://github.com/facebook/jest/pull/6711))