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

Jump to bottom

#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
Merged

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

merged 8 commits into from
Nov 26, 2020

Conversation

sarathps93
Copy link
Contributor

@sarathps93 sarathps93 commented Nov 10, 2020 •
edited
Loading

Summary

Fixes #10451
Fixes the below issues in packages jest-circus & jest-jasmine2.

  1. beforeAll block runs even if it is inside a describe.skip block when the it block contains tests marked as todo or only
  2. Tests run even if it is inside a describe.skip when the tests are marked as todo or only

Test plan

e2e/__tests__/skipBeforeAfterAll.test.ts
packages/jest-circus/src/__tests__/afterAll.test.ts

@sarathps93 sarathps93 changed the title Fix/issue 10451 #10451 - beforeAll block & tests marked with only & todo runs even if it is inside a describe.skip block Nov 10, 2020
@sarathps93 sarathps93 force-pushed the fix/issue-10451 branch 2 times, most recently from 724e76f to 3c45ca1 Compare November 11, 2020 04:42
@sarathps93
Copy link
Contributor Author

@SimenB Could you please help me out with the failing CI build check? There seems to be some problem while running with Node 14.x version in MacOS latest

SimenB
SimenB approved these changes Nov 11, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

thanks!

you can ignore the one CI failure, it's flake

CHANGELOG.md Outdated
Comment on lines 5 to 7
- `[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 when it is inside a parent `describe` block with skipped status [#10451](https://github.com/facebook/jest/issues/10451)
Copy link
Member

Choose a reason for hiding this comment

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

these should be under the existent Fixes heading

Copy link
Contributor Author

@sarathps93 sarathps93 Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sure. I have made the changes

packages/jest-jasmine2/src/treeProcessor.ts Outdated
return (
node.disabled ||
node.markedPending ||
(!!node.children && node.children.every(hasNoEnabledTest))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
(!!node.children && node.children.every(hasNoEnabledTest))
node.children?.every(hasNoEnabledTest)

right?

Copy link
Contributor Author

@sarathps93 sarathps93 Nov 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. Made the change. However since ts is prompting for a boolean return value, without double negation it is throwing an error owing to a possible undefined return value. So it would be !!node.children?.every(hasNoEnabledTest). Is it fine?

Copy link
Member

Choose a reason for hiding this comment

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

node.children?.every(hasNoEnabledTest) ?? false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I have done the changes

@sarathps93 sarathps93 force-pushed the fix/issue-10451 branch 3 times, most recently from 49534d8 to 6687dcc Compare November 14, 2020 15:45
SimenB
SimenB approved these changes Nov 15, 2020
View reviewed changes
packages/jest-jasmine2/src/jasmine/Env.ts Outdated Show resolved Hide resolved
sarathps93 and others added 2 commits November 15, 2020 19:55
@sarathps93 @SimenB
Update packages/jest-jasmine2/src/jasmine/Env.ts
a485155 
Changing as per review comment suggestion

Co-authored-by: Simen Bekkhus <[email protected]>
@sarathps93
changes as per review comments
18715a0
@SimenB SimenB merged commit 0e50f73 into jestjs:master Nov 26, 2020
@sarathps93 sarathps93 deleted the fix/issue-10451 branch November 27, 2020 12:20
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thymikee thymikee thymikee approved these changes

@SimenB SimenB SimenB approved these changes

@jeysal jeysal Awaiting requested review from jeysal

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

beforeAll block runs even if it is inside a describe.skip block
4 participants
@sarathps93 @SimenB @thymikee @facebook-github-bot