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.

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

#10451 - beforeAll block & tests marked with only & todo runs even if it is inside a describe.skip block #10806

Merged
merged 8 commits into from
Nov 26, 2020
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@

### Fixes

- `[jest-circus]` Fixed the issue of beforeAll & afterAll hooks getting executed even if it is inside a skipped `describe` block [#10451](https://github.com/facebook/jest/issues/10451)
- `[jest-jasmine2]` Fixed the issue of beforeAll & afterAll hooks getting executed even if it is inside a skipped `describe` block when it has child `tests` marked as either `only` or `todo` [#10451](https://github.com/facebook/jest/issues/10451)
- `[jest-jasmine2]` Fixed the issues of child `tests` marked with `only` or `todo` getting executed even if it is inside a skipped parent `describe` block [#10451](https://github.com/facebook/jest/issues/10451)
- `[jest-console]` `console.dir` now respects the second argument correctly ([#10638](https://github.com/facebook/jest/pull/10638))
- `[expect]` [**BREAKING**] Revise `expect.not.objectContaining()` to be the inverse of `expect.objectContaining()`, as documented. ([#10708](https://github.com/facebook/jest/pull/10708))
- `[jest-resolve]` Replace read-pkg-up with escalade package ([#10781](https://github.com/facebook/jest/pull/10781))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ FAIL __tests__/asyncDefinition.test.js
14 | });
15 | });

at eventHandler (../../packages/jest-circus/build/eventHandler.js:143:11)
at eventHandler (../../packages/jest-circus/build/eventHandler.js:144:11)
at test (__tests__/asyncDefinition.test.js:12:5)

● Test suite failed to run
Expand All @@ -31,7 +31,7 @@ FAIL __tests__/asyncDefinition.test.js
15 | });
16 |

at eventHandler (../../packages/jest-circus/build/eventHandler.js:112:11)
at eventHandler (../../packages/jest-circus/build/eventHandler.js:113:11)
at afterAll (__tests__/asyncDefinition.test.js:13:5)

● Test suite failed to run
Expand All @@ -46,7 +46,7 @@ FAIL __tests__/asyncDefinition.test.js
20 | });
21 |

at eventHandler (../../packages/jest-circus/build/eventHandler.js:143:11)
at eventHandler (../../packages/jest-circus/build/eventHandler.js:144:11)
at test (__tests__/asyncDefinition.test.js:18:3)

● Test suite failed to run
Expand All @@ -60,6 +60,6 @@ FAIL __tests__/asyncDefinition.test.js
20 | });
21 |

at eventHandler (../../packages/jest-circus/build/eventHandler.js:112:11)
at eventHandler (../../packages/jest-circus/build/eventHandler.js:113:11)
at afterAll (__tests__/asyncDefinition.test.js:19:3)
`;
5 changes: 2 additions & 3 deletions e2e/__tests__/skipBeforeAfterAll.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ const DIR = path.resolve(__dirname, '../before-all-skipped');

test('correctly skip `beforeAll`s in skipped tests', () => {
const {json} = runWithJson(DIR);

expect(json.numTotalTests).toBe(6);
expect(json.numTotalTests).toBe(8);
expect(json.numPassedTests).toBe(4);
expect(json.numPendingTests).toBe(2);
expect(json.numPendingTests).toBe(4);
});
7 changes: 7 additions & 0 deletions e2e/before-all-skipped/__tests__/beforeAllFiltered.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,13 @@ describe.skip('in describe.skip', () => {
test('it should be skipped', () => {
throw new Error('This should never happen');
});

// eslint-disable-next-line jest/no-focused-tests
test.only('it should be skipped as well', () => {
throw new Error('This should never happen');
});

test.todo('it should also be skipped');
});
});

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,41 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`child tests marked with only should not run if describe block is skipped 1`] = `
start_describe_definition: describe
add_hook: afterAll
add_hook: beforeAll
add_test: my test
finish_describe_definition: describe
run_start
run_describe_start: ROOT_DESCRIBE_BLOCK
run_describe_start: describe
test_start: my test
test_skip
run_describe_finish: describe
run_describe_finish: ROOT_DESCRIBE_BLOCK
run_finish

unhandledErrors: 0
`;

exports[`child tests marked with todo should not run if describe block is skipped 1`] = `
start_describe_definition: describe
add_hook: afterAll
add_hook: beforeAll
add_test: my test
finish_describe_definition: describe
run_start
run_describe_start: ROOT_DESCRIBE_BLOCK
run_describe_start: describe
test_start: my test
test_skip
run_describe_finish: describe
run_describe_finish: ROOT_DESCRIBE_BLOCK
run_finish

unhandledErrors: 0
`;

exports[`describe block _can_ have hooks if a child describe block has tests 1`] = `
start_describe_definition: describe
add_hook: afterEach
Expand Down Expand Up @@ -56,6 +92,24 @@ run_finish
unhandledErrors: 4
`;

exports[`describe block hooks must not run if describe block is skipped 1`] = `
start_describe_definition: describe
add_hook: afterAll
add_hook: beforeAll
add_test: my test
finish_describe_definition: describe
run_start
run_describe_start: ROOT_DESCRIBE_BLOCK
run_describe_start: describe
test_start: my test
test_skip
run_describe_finish: describe
run_describe_finish: ROOT_DESCRIBE_BLOCK
run_finish

unhandledErrors: 0
`;

exports[`tests are not marked done until their parent afterAll runs 1`] = `
"start_describe_definition: describe
add_hook: afterAll
Expand Down
33 changes: 33 additions & 0 deletions packages/jest-circus/src/__tests__/afterAll.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,36 @@ test('describe block _can_ have hooks if a child describe block has tests', () =
`);
expect(wrap(result.stdout)).toMatchSnapshot();
});

test('describe block hooks must not run if describe block is skipped', () => {
const result = runTest(`
describe.skip('describe', () => {
afterAll(() => console.log('> afterAll'));
beforeAll(() => console.log('> beforeAll'));
test('my test', () => console.log('> my test'));
})
`);
expect(wrap(result.stdout)).toMatchSnapshot();
});

test('child tests marked with todo should not run if describe block is skipped', () => {
const result = runTest(`
describe.skip('describe', () => {
afterAll(() => console.log('> afterAll'));
beforeAll(() => console.log('> beforeAll'));
test.todo('my test');
})
`);
expect(wrap(result.stdout)).toMatchSnapshot();
});

test('child tests marked with only should not run if describe block is skipped', () => {
const result = runTest(`
describe.skip('describe', () => {
afterAll(() => console.log('> afterAll'));
beforeAll(() => console.log('> beforeAll'));
test.only('my test', () => console.log('> my test'));
})
`);
expect(wrap(result.stdout)).toMatchSnapshot();
});
3 changes: 2 additions & 1 deletion packages/jest-circus/src/eventHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ const eventHandler: Circus.EventHandler = (
}
if (
!state.hasFocusedTests &&
currentDescribeBlock.mode !== 'skip' &&
currentDescribeBlock.children.some(
child => child.type === 'test' && child.mode === 'only',
)
Expand Down Expand Up @@ -143,7 +144,7 @@ const eventHandler: Circus.EventHandler = (
timeout,
asyncError,
);
if (test.mode === 'only') {
if (currentDescribeBlock.mode !== 'skip' && test.mode === 'only') {
state.hasFocusedTests = true;
}
currentDescribeBlock.children.push(test);
Expand Down
24 changes: 17 additions & 7 deletions packages/jest-circus/src/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,12 @@ const _runTestsForDescribeBlock = async (
await dispatch({describeBlock, name: 'run_describe_start'});
const {beforeAll, afterAll} = getAllHooksForDescribe(describeBlock);

for (const hook of beforeAll) {
await _callCircusHook({describeBlock, hook});
const isSkipped = describeBlock.mode === 'skip';

if (!isSkipped) {
for (const hook of beforeAll) {
await _callCircusHook({describeBlock, hook});
}
}

// Tests that fail and are retried we run after other tests
Expand All @@ -50,7 +54,7 @@ const _runTestsForDescribeBlock = async (
}
case 'test': {
const hasErrorsBeforeTestRun = child.errors.length > 0;
await _runTest(child);
await _runTest(child, isSkipped);

if (
hasErrorsBeforeTestRun === false &&
Expand All @@ -72,24 +76,30 @@ const _runTestsForDescribeBlock = async (
// Clear errors so retries occur
await dispatch({name: 'test_retry', test});

await _runTest(test);
await _runTest(test, isSkipped);
numRetriesAvailable--;
}
}

for (const hook of afterAll) {
await _callCircusHook({describeBlock, hook});
if (!isSkipped) {
for (const hook of afterAll) {
await _callCircusHook({describeBlock, hook});
}
}

await dispatch({describeBlock, name: 'run_describe_finish'});
};

const _runTest = async (test: Circus.TestEntry): Promise<void> => {
const _runTest = async (
test: Circus.TestEntry,
parentSkipped: boolean,
): Promise<void> => {
await dispatch({name: 'test_start', test});
const testContext = Object.create(null);
const {hasFocusedTests, testNamePattern} = getState();

const isSkipped =
parentSkipped ||
test.mode === 'skip' ||
(hasFocusedTests && test.mode !== 'only') ||
(testNamePattern && !testNamePattern.test(getTestID(test)));
Expand Down
12 changes: 10 additions & 2 deletions packages/jest-jasmine2/src/jasmine/Env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,11 @@ export default function (j$: Jasmine) {
() => {},
currentDeclarationSuite,
);
spec.todo();
if (currentDeclarationSuite.markedPending) {
spec.pend();
} else {
spec.todo();
}
currentDeclarationSuite.addChild(spec);
return spec;
};
Expand All @@ -624,7 +628,11 @@ export default function (j$: Jasmine) {
timeout,
);
currentDeclarationSuite.addChild(spec);
focusedRunnables.push(spec.id);
if (currentDeclarationSuite.markedPending) {
spec.pend();
} else {
focusedRunnables.push(spec.id);
}
unfocusAncestor();
return spec;
};
Expand Down
9 changes: 5 additions & 4 deletions packages/jest-jasmine2/src/treeProcessor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,11 @@ export default function treeProcessor(options: Options): void {
}

function hasNoEnabledTest(node: TreeNode): boolean {
if (node.children) {
return node.children.every(hasNoEnabledTest);
}
return node.disabled || node.markedPending;
return (
node.disabled ||
node.markedPending ||
(node.children?.every(hasNoEnabledTest) ?? false)
);
}

function wrapChildren(node: TreeNode, enabled: boolean) {
Expand Down