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

fix(treeProcessor): revert a small change from the #5585 refactoring #6006

Merged
merged 1 commit into from
Apr 17, 2018

Conversation

niieani
Copy link
Contributor

@niieani niieani commented Apr 16, 2018

Summary

Fixes #5964 by partly restoring behavior from before #5585.

Moves the wrapChildren into higher scope to enforce undocumented execution order of beforeEach added inside of tests.

…oring

fixes jestjs#5964
moves the `wrapChildren` into higher scope to restore undocumented execution order
@SimenB
Copy link
Member

SimenB commented Apr 16, 2018

Should we add a test for this? But not document it, as IMO this behavior is not something anyone should depend on

@codecov-io
Copy link

Codecov Report

Merging #6006 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6006      +/-   ##
==========================================
- Coverage   64.32%   64.32%   -0.01%     
==========================================
  Files         217      217              
  Lines        8312     8313       +1     
  Branches        3        4       +1     
==========================================
  Hits         5347     5347              
- Misses       2964     2965       +1     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/tree_processor.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70e7ac2...c682ca3. Read the comment docs.

@niieani
Copy link
Contributor Author

niieani commented Apr 16, 2018

I don't believe I have enough context to write a good test for this.

If somebody (@mjesun maybe?) with better knowledge of how Facebook depends on this behavior wants to write an integration test, go for it.

@mjesun
Copy link
Contributor

mjesun commented Apr 17, 2018

You can use the one I put up on the issue as an integration test, asserting console.log output. That'd be excellent!

@cpojer cpojer merged commit bf654e1 into jestjs:master Apr 17, 2018
@cpojer
Copy link
Member

cpojer commented Apr 17, 2018

I'll merge it, we can add a test in a separate PR.

@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.

#5585 breaks pre-established "beforeEach" order
6 participants