Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(jest-circus) correct concurrent event ordering #15381

Merged

Conversation

madcapnmckay
Copy link
Contributor

@madcapnmckay madcapnmckay commented Nov 15, 2024

Summary

Note this is a re-raise of an old PR that I previously let go stale.

I'm unsure if this is a major, minor, or patch change. It does not change the API, but it changes the order of events emitted from jest-circus. Please advise.

This fixes the issue documented here

When using test.concurrent events such as test_start, test_started, and test_fn_start are dispatched after the body of the test is executed. This is due to the _runTest function not being executed as part of the parallel execution runner; instead, it was executed synchronously like non-concurrent tests.

The solution was to use _runTest within startTestsConcurrently() and then wait for that process to complete instead of replacing the test.fn as was being done before.

I also added a bunch of e2e tests for concurrent, this change most likely closes some other issues relating to before/afterAll.

Test plan

  • Checkout commit 953bbed700
  • Run yarn test packages/jest-circus/src/__tests__/baseTest.test.ts to observe the broken snapshot content for the concurrent test.
hello one <-- test running before `test_start` has fired.
hello two
hello three
run_describe_start: describe
test_start: one
test_started: one
test_fn_start: one
test_fn_failure: one
  • Check out HEAD and rerun the test. Note that the output has been reordered to match the expectation that the circus actions occur before and after the test execution.
  • Execute the new concurrent e2e tests with yarn jest e2e/__tests__/circusConcurrent.test.ts and yarn jest e2e/__tests__/testRetries.test.ts

Note: at the time this is being raised main does not pass yarn test, I have confirmed that the same tests are failing before and after my change.

Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for jestjs ready!

Name Link
🔨 Latest commit fb06764
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/67878dde37cb410008ab919b
😎 Deploy Preview https://deploy-preview-15381--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@madcapnmckay madcapnmckay changed the title Jest circus concurrent event ordering2 fix(jest-circus) correct concurrent event ordering Nov 15, 2024
@madcapnmckay
Copy link
Contributor Author

@SimenB - the failures on the checks seem to be affecting other PRs.

@madcapnmckay madcapnmckay force-pushed the jest-circus-concurrent-event-ordering2 branch from 0578d54 to 99ada77 Compare January 6, 2025 17:43
@SimenB
Copy link
Member

SimenB commented Jan 15, 2025

Hey, sorry about the radio silence! CI should be fixed now, and I've merged in main here.

@SimenB
Copy link
Member

SimenB commented Jan 15, 2025

I think this makes sense, looking at the implementation changes. And the extensive tests (thank you!!!) also seems reasonable to me

@SimenB SimenB enabled auto-merge (squash) January 15, 2025 10:32
@SimenB SimenB merged commit fba7764 into jestjs:main Jan 15, 2025
83 of 84 checks passed
mohammednumaan pushed a commit to mohammednumaan/jest that referenced this pull request Jan 17, 2025
@SimenB
Copy link
Member

SimenB commented Jan 30, 2025

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants