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

jest-circus: throw if a test / hook is defined asynchronously #8096

Merged
merged 6 commits into from
May 2, 2020

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Mar 9, 2019

Summary

This fixes the behavior when defining tests / hooks asynchronously in jest-circus.
Previously outside of a describe block (in the root describe block) the tests would be accepted and run.
Previously inside a describe block this error would be thrown:
image
But it was also possible to time it so that the test declared asynchronously in the describe block would become a test of the root describe block.

With this PR async definitions anywhere will throw:
image

Test plan

circus-only e2e test added

@jeysal
Copy link
Contributor Author

jeysal commented Mar 9, 2019

cc @aaronabramov since this touches circus

@@ -183,6 +183,7 @@ export type State = {
currentlyRunningTest: TestEntry | undefined | null; // including when hooks are being executed
expand?: boolean; // expand error messages
Copy link
Member

Choose a reason for hiding this comment

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

not for this PR, but we should use tsdoc for these so they show up when using autocomplete

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.

This is great, thank you!

Needs to make the test non-flaky and add a changelog entry 🙂

@jeysal
Copy link
Contributor Author

jeysal commented Mar 9, 2019

Not sure if this is a fix or feature for the changelog 🤔

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

Not sure if this is a fix or feature for the changelog 🤔

ergh... fix I'd say. It might have been working by accident before, and probably flaky depending on how much resources your computer had. So it's just an explicit error in the case of user error

@jeysal
Copy link
Contributor Author

jeysal commented Mar 9, 2019

beforeEachQueue e2e test is failing

@jeysal
Copy link
Contributor Author

jeysal commented Mar 9, 2019

Apparently defining hooks in tests is supported? #6098

@SimenB
Copy link
Member

SimenB commented Mar 9, 2019

@mjesun @rubennorte did you ever manage to migrate away from those funky tests?

@jeysal
Copy link
Contributor Author

jeysal commented Mar 26, 2019

@scotthovestadt perhaps you could also answer this using fbcodesearch or whatever it's called?
TLDR does FB still do {before,after}{Each,All} inside of it/test somewhere?

@jeysal jeysal force-pushed the circus-add-test-while-running branch from 32d2e5d to c559c75 Compare April 14, 2019 10:46
@jeysal jeysal added this to the Jest 25 milestone Aug 11, 2019
@SimenB
Copy link
Member

SimenB commented Aug 22, 2019

Rebase this?

@jeysal
Copy link
Contributor Author

jeysal commented Aug 22, 2019

@SimenB done. But we still need @scotthovestadt to verify that FB no longer has these weird hook definitions that would break because of this. Or would you do that when releasing an alpha an this has already landed anyway, instead of from this branch while it's open?

@SimenB
Copy link
Member

SimenB commented Aug 22, 2019

Whatever's most convenient for @scotthovestadt 🙂

@jeysal
Copy link
Contributor Author

jeysal commented Aug 22, 2019

GitHub not realizing I've pushed the rebase ... https://www.githubstatus.com "Degraded Performance"

@jeysal
Copy link
Contributor Author

jeysal commented Aug 22, 2019

(And ofc would have to remove beforeEachQueue e2e test that fails on circus once we think that weird behavior no longer needs to be supported before merging ) ⚠️

@jeysal jeysal force-pushed the circus-add-test-while-running branch from c559c75 to 2a2fb98 Compare August 22, 2019 09:57
CHANGELOG.md Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Nov 9, 2019

@scotthovestadt could you test this?

@jeysal jeysal mentioned this pull request May 1, 2020
6 tasks
@SimenB
Copy link
Member

SimenB commented May 1, 2020

@jeysal I've completely forgotten about this one. I still think it makes sense 😀 @cpojer will this break FB, or can we land it?

@cpojer
Copy link
Member

cpojer commented May 1, 2020

No, this is a good change (y)

Copy link
Member

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

👍

@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants