Skip to content

Refactor for tree processor #6177

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.

Sign up for GitHub

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
Merged
mjesun merged 3 commits into from
May 14, 2018
Merged

Refactor for tree processor #6177

mjesun merged 3 commits into from
May 14, 2018

Conversation

@mjesun
Copy link
Contributor

@mjesun mjesun commented May 13, 2018

This PR is a revamp of #5885, #6006, and #6100, removing the need of the flag.

cc // @niieani

niieani added 2 commits May 13, 2018 11:07
@niieani @mjesun
Refactor: unify handling of topSuite and children suites + increase r…
d9ffeb2 
…eadability (#5885)
@niieani @mjesun
fix(treeProcessor): revert a small change from the #5585 refactoring (#…
182b4a6 
…6006)

fixes #5964
moves the `wrapChildren` into higher scope to restore undocumented execution order
@mjesun mjesun requested review from SimenB, cpojer and thymikee May 13, 2018 10:19
@mjesun
Copy link
Contributor Author

mjesun commented May 13, 2018

Don't merge until @niieani gives green light!

@SimenB
Copy link
Member

SimenB commented May 13, 2018

Should we add tests for the order?

@mjesun
Copy link
Contributor Author

mjesun commented May 13, 2018

@SimenB It was added in #6098 😎

@SimenB
Copy link
Member

SimenB commented May 13, 2018

Awesome!

@SimenB
Copy link
Member

SimenB commented May 13, 2018

Needs a snapshot update for a stack change

niieani
niieani approved these changes May 13, 2018
View reviewed changes
Copy link
Contributor

@niieani niieani left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, thanks @mjesun. I've run the webpack suite with the legacy order (as is here) and it passed. Hopefully it will work in the long run.
We should still think about the before/after order in the future.

@codecov-io
Copy link

codecov-io commented May 14, 2018

Codecov Report

Merging #6177 into master will decrease coverage by 0.06%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6177      +/-   ##
==========================================
- Coverage   64.22%   64.15%   -0.07%     
==========================================
  Files         219      219              
  Lines        8430     8428       -2     
  Branches        4        4              
==========================================
- Hits         5414     5407       -7     
- Misses       3015     3020       +5     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-jasmine2/src/tree_processor.js 0% <0%> (ø) ⬆️
packages/jest-jasmine2/src/jasmine/Env.js 0% <0%> (ø) ⬆️
.../pretty-format/src/plugins/react_test_component.js 100% <0%> (ø) ⬆️
...ackages/pretty-format/src/plugins/react_element.js 100% <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 19c5278...b10311c. Read the comment docs.

@mjesun mjesun merged commit b7636e4 into jestjs:master May 14, 2018
@mjesun mjesun deleted the tree-processor branch May 14, 2018 17:31
@SimenB SimenB mentioned this pull request May 21, 2018
Closed
@github-actions
Copy link

github-actions bot commented May 12, 2021

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.

Reviewers

@cpojer cpojer cpojer approved these changes

@SimenB SimenB Awaiting requested review from SimenB

@thymikee thymikee Awaiting requested review from thymikee

+1 more reviewer

@niieani niieani niieani approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mjesun @SimenB @codecov-io @cpojer @niieani @facebook-github-bot