fix beforeAll block & tests marked with only & todo runs even if it i…

…s inside a describe.skip block (#10806)
jestjs · Nov 26, 2020 · 0e50f73 · 0e50f73
1 parent 890852b
commit 0e50f73
Show file tree

Hide file tree

Showing 10 changed files with 137 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,6 +11,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-reporter]` Handle empty files when reporting code coverage with V8 ([#10819](https://github.com/facebook/jest/pull/10819))

diff --git a/e2e/__tests__/__snapshots__/circusDeclarationErrors.test.ts.snap b/e2e/__tests__/__snapshots__/circusDeclarationErrors.test.ts.snap
@@ -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
@@ -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
@@ -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
@@ -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)
 `;
diff --git a/e2e/__tests__/skipBeforeAfterAll.test.ts b/e2e/__tests__/skipBeforeAfterAll.test.ts
@@ -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);
 });
diff --git a/e2e/before-all-skipped/__tests__/beforeAllFiltered.test.js b/e2e/before-all-skipped/__tests__/beforeAllFiltered.test.js
@@ -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');
   });
 });
 

diff --git a/packages/jest-circus/src/__tests__/__snapshots__/afterAll.test.ts.snap b/packages/jest-circus/src/__tests__/__snapshots__/afterAll.test.ts.snap
@@ -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
@@ -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

diff --git a/packages/jest-circus/src/__tests__/afterAll.test.ts b/packages/jest-circus/src/__tests__/afterAll.test.ts
@@ -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();
+});
diff --git a/packages/jest-circus/src/eventHandler.ts b/packages/jest-circus/src/eventHandler.ts
@@ -79,6 +79,7 @@ const eventHandler: Circus.EventHandler = (
       }
       if (
         !state.hasFocusedTests &&
+        currentDescribeBlock.mode !== 'skip' &&
         currentDescribeBlock.children.some(
           child => child.type === 'test' && child.mode === 'only',
         )
@@ -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);

diff --git a/packages/jest-circus/src/run.ts b/packages/jest-circus/src/run.ts
@@ -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
@@ -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 &&
@@ -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)));

diff --git a/packages/jest-jasmine2/src/jasmine/Env.ts b/packages/jest-jasmine2/src/jasmine/Env.ts
@@ -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;
       };
@@ -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;
       };

diff --git a/packages/jest-jasmine2/src/treeProcessor.ts b/packages/jest-jasmine2/src/treeProcessor.ts
@@ -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) {