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

Add test for "beforeEach" in "test" behavior #6098

Merged
merged 1 commit into from
May 2, 2018
Merged

Add test for "beforeEach" in "test" behavior #6098

merged 1 commit into from
May 2, 2018

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Apr 30, 2018

#6006 didn't fix the issue introduced in #5885, discussed in #5964. I'm adding a test (that will fail) until fixed.

@mjesun
Copy link
Contributor Author

mjesun commented Apr 30, 2018

cc / @niieani

@niieani
Copy link
Contributor

niieani commented Apr 30, 2018

@mjesun does this mean FB's internal test suite is still broken?

@mjesun
Copy link
Contributor Author

mjesun commented Apr 30, 2018

Yes :'( I thought I tested it, but can't see where I commented

@niieani niieani mentioned this pull request Apr 30, 2018
@niieani
Copy link
Contributor

niieani commented May 1, 2018

Fix in #6100

@mjesun mjesun merged commit 3ad83eb into jestjs:master May 2, 2018
@mjesun
Copy link
Contributor Author

mjesun commented May 2, 2018

@niieani Now that the test is in master, could you bring back #5885, #6006 and #6100 altogether, without the legacy flag? Having this would be perfect and then I'm happy to merge it again. Sorry for all the churn I've created, I'm not happy with it either 😔

@SimenB
Copy link
Member

SimenB commented May 2, 2018

@mjesun this test should either be fixed or skipped on windows, appveyor is broken

@mjesun
Copy link
Contributor Author

mjesun commented May 2, 2018

@SimenB looks like this PR passed on AppVeyor: https://ci.appveyor.com/project/Daniel15/jest/build/7913

@SimenB
Copy link
Member

SimenB commented May 2, 2018

That's odd, the merge failed: https://ci.appveyor.com/project/Daniel15/jest/build/7914

@niieani
Copy link
Contributor

niieani commented May 2, 2018

@mjesun revert the revert (lol) #6105 and merge #6100 on top and it should be golden.

@mjesun
Copy link
Contributor Author

mjesun commented May 2, 2018

@niieani can you remove the legacy flag on #6100?

@SimenB
Copy link
Member

SimenB commented May 2, 2018

It's not a flag though, it's config. Why is it an issue adding that to package.json in FB?

@niieani
Copy link
Contributor

niieani commented May 2, 2018

@mjesun Given @SimenB's comment on unintuitive ordering here #6105 (comment) can we confirm that we do not want this odd behavior behind a flag in config?

@mjesun
Copy link
Contributor Author

mjesun commented May 2, 2018

If we can figure out someone who will clean this internally, I'm happy having the flag // cc @cpojer

@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 12, 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