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

Jump to bottom

feat: make jest-circus default test runner #10686

Merged
merged 1 commit into from Nov 26, 2020
Merged

feat: make jest-circus default test runner #10686

merged 1 commit into from Nov 26, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 23, 2020

Summary

As announced in the blog post.

Fixes #6295

Test plan

I've changed the helper around to opt-in to jasmine instead of circus, so green CI should be enough

@SimenB SimenB added this to the Jest 27 milestone Oct 23, 2020
@SimenB SimenB force-pushed the ship-circus branch 3 times, most recently from 9da97c6 to ecffde4 Compare October 23, 2020 17:03
@jeysal
Copy link
Contributor

jeysal commented Oct 25, 2020

Nice 👌 may I ask, do we really need the JEST_JASMINE env variable? Always found it a little odd that there is this one environment variable when we use the environment for almost nothing. It seems JEST_JASMINE=1 jest vs jest --testRunner=jest-jasmine isn't much of a difference, or am I missing something?

@SimenB
Copy link
Member Author

SimenB commented Oct 25, 2020 •
edited

It's to ensure all of our tests are executed with the same runner, including e2e tests. It's not a public API sort of thing. Passing testRunner all over is painful I'd think. Maybe not?

(background: #6285)

@jeysal
Copy link
Contributor

jeysal commented Oct 25, 2020

I was thinking for those we have a centralized place in utils with runJest, but re-considering it there are probably lots of exceptions as well, plus most importantly I guess it's unsafe otherwise since you probably wouldn't notice if something doesn't use the intended runner.

cpojer
cpojer approved these changes Oct 26, 2020
View reviewed changes
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.

Big deal!

@SimenB
Copy link
Member Author

SimenB commented Nov 2, 2020

huh, weird it failed on windows... will look into it later this week

@SimenB SimenB force-pushed the ship-circus branch 3 times, most recently from 91de206 to 68dd53e Compare November 4, 2020 20:39
@SimenB SimenB mentioned this pull request Nov 6, 2020
Closed
@SimenB SimenB force-pushed the ship-circus branch 3 times, most recently from bb2c4e2 to 96b232d Compare November 15, 2020 21:51
@SimenB SimenB mentioned this pull request Nov 26, 2020
Merged
@SimenB SimenB merged commit 5f6f2ec into jestjs:master Nov 26, 2020
@SimenB SimenB deleted the ship-circus branch November 26, 2020 16:24
@Mouvedia Mouvedia mentioned this pull request Nov 26, 2020
Closed
6 tasks
@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

@cpojer cpojer cpojer approved these changes

@thymikee thymikee Awaiting requested review from thymikee

@jeysal jeysal Awaiting requested review from jeysal

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

Successfully merging this pull request may close these issues.

Ship Jest Circus
4 participants
@SimenB @jeysal @cpojer @facebook-github-bot