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

feat(jest-circus): add onTestCaseStart hook Reporter #14174

Conversation

DmitryMakhnev
Copy link
Contributor

@DmitryMakhnev DmitryMakhnev commented May 31, 2023

Summary

Test plan

Unit tests added

Detailed design

At start, I tried using the current jest-circus events to detect when a test case starts by listening to the test_start event and calling the onTestCaseStart hook of the Reporter after it. However, when a test is marked as todo or skipped for any reason, jest-circus still dispatches the test_start event, followed by the test_skip or test_todo events, which makes it incorrect to call the onTestCaseStart hook on the test_start event, because the test suite hasn't actually started yet. This behavior forces tools that will handle the onTestCaseStart hook to filter out the tests that have actually started. Unfortunately, also on test_start event we don't have full information about test skipping, because most reasons for skipping a test resolve after the event dispatching.

Moving the test_start event after the test_skip and test_todo events could be a solution, but it could also break things, because jest has an internal handler that listens to the test_start event and modifies global state. Therefore, I decided not to move the test_start event.

I investigated other events that are dispatched after the test_skip and test_todo events and found the hook_start event dispatching and the test_fn_start event dispatching. Calling the onTestCaseStart hook on the test_fn_start event would skip the before* hooks calling phase, which is not suitable for the feature request this PR is solving, because catching output from the before* hooks calling phase is important.

Calling the onTestCaseStart hook on the hook_start event also looks incorrect, because the hook_start event will be avoided for most tests and can be called many times for one test. This requires listening to a combination of hook_start and test_fn_start events, which makes the solution stateful and less stable.

So after all these investigations and discussing with colleagues, I decided to add a new event called test_started to the jest-circus test's run flow, which is dispatched after the test_skip and test_todo events and before the hook_start and test_fn_start events. This is the best place in the test's running flow to understand when a test case really starts, and connecting the onTestCaseStart hook of the Reporter to this event is simple and transparent.

I found this solution. If you have any other suggestions, you are welcome.

…nd transparent approach

- clean previous approach
- add `test_started` event to Circus runner
- fix tests for `test_started` event
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 31, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify
Copy link

netlify bot commented May 31, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 26566c9
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/64922072c5b6a0000810f974
😎 Deploy Preview https://deploy-preview-14174--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 settings.

@DmitryMakhnev DmitryMakhnev changed the title Issue 13112 on test case start reporter hook correct feat(jest-circus): add on testCaseStart Reporter hook May 31, 2023
@DmitryMakhnev DmitryMakhnev changed the title feat(jest-circus): add on testCaseStart Reporter hook feat(jest-circus): add onTestCaseStart hook Reporter May 31, 2023
@segrey
Copy link

segrey commented Jun 14, 2023

@SimenB Would you please take a look at this review?

@Ariane-B
Copy link

Is someone even assigned to this pull request? What's happening? It's Jest's problem, I guess, but at the moment it means jest-circus is unusable in my IDE, and so I'm not using it.

@DmitryMakhnev
Copy link
Contributor Author

Is someone even assigned to this pull request? What's happening? It's Jest's problem, I guess, but at the moment it means jest-circus is unusable in my IDE, and so I'm not using it.

Hey @Ariane-B,

@segrey and I have asked @SimenB for a review and are waiting for reply from @SimenB.

@Ariane-B
Copy link

Is someone even assigned to this pull request? What's happening? It's Jest's problem, I guess, but at the moment it means jest-circus is unusable in my IDE, and so I'm not using it.

Hey @Ariane-B,

@segrey and I have asked @SimenB for a review and are waiting for reply from @SimenB.

Oh yeah I know. The two of you are really stepping up here. That was my way of putting pressure on the Jest people. You've been talking to them about the issue for a long time, and they're a pretty significant bottleneck.

@SimenB
Copy link
Member

SimenB commented Jun 20, 2023

Hey folks! Sorry about the slow answer. I'm currently preparing for a new job, so open source doesn't get much attention atm. Thanks for pinging - anything that helps IDE integration (especially intellij which I use myself 😀) should be prioritized.

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.

👍

@SimenB SimenB merged commit 45daa05 into jestjs:main Jun 20, 2023
@SimenB
Copy link
Member

SimenB commented Jun 20, 2023

(are you on open js slack? That forum would work better to ping me on than GitHub tags or emails. (If there's some other forum you like prefer, I'd be happy to join. The open ones doesn't really work))

@DmitryMakhnev
Copy link
Contributor Author

Hey @SimenB,

Thank you so much 🙏

I'm currently preparing for a new job

Have awesome time with new job ✨

anything that helps IDE integration (especially intellij which I use myself 😀) should be prioritized

Thanks. Nice to hear.

are you on open js slack? That forum would work better to ping me on than GitHub tags or emails.

I joined after you mentioned it and I'm interesting to try it.

If there's some other forum you like prefer, I'd be happy to join

I don't know of any suitable forum, however I'll think about it.

@SimenB
Copy link
Member

SimenB commented Jun 21, 2023

I'll release this after landing #14062 (unless it takes a long time). That might be useful for your cases as well?

@SimenB
Copy link
Member

SimenB commented Jul 4, 2023

https://github.com/jestjs/jest/releases/tag/v29.6.0

@github-actions
Copy link

github-actions bot commented Aug 4, 2023

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 Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: report beginning of every test or it via reporter API
4 participants