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

Issue 8547: side effect import deps extracted #8670

Merged
merged 1 commit into from
Jul 15, 2019
Merged

Conversation

astigefb
Copy link
Contributor

Summary

Jest previously wouldn't find changes to side-effect only imports when watching files.

#8547

Test plan

  1. Added a test to make it fail
  2. Made the test pass
  3. Tried to ensure other tests failing / passing status were the same running a full yarn jest, but there's a fair amount of noise is pre-existing failures. Also tried to use yarn jest --json --outputFile=[...] before and after but a lot of noise there in comparing. Settled for similar looking summaries before / after that looks something like the below when run close together:

Snapshot Summary
› 4 snapshots failed from 2 test suites. Inspect your code changes or run yarn run jest -u to update them.

Test Suites: 7 failed, 328 passed, 335 total
Tests: 12 failed, 9 skipped, 3465 passed, 3486 total
Snapshots: 4 failed, 1465 passed, 1469 total
Time: 154.846s, estimated 158s
Ran all test suites in 14 projects.

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@thymikee thymikee requested a review from scotthovestadt July 10, 2019 17:27
@jeysal
Copy link
Contributor

jeysal commented Jul 10, 2019

Hi @astigefb, thanks for the PR!
There is already #8548, which seems to do this in a more comprehensive way I think. Maybe we should see if @dy93 can follow up on the review comment there so that the effort is not duplicated :)

@astigefb astigefb force-pushed the master branch 3 times, most recently from eca3234 to f343f81 Compare July 10, 2019 17:48
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@astigefb
Copy link
Contributor Author

@jeysal - Whoops I didn't notice someone else was working on this, thanks for the pointer / link~ I've copied in the negative tests from #8548, and looks like I happened to do the single regex request here. If there's anything else I'm missing let me know :)

@dy93 - I've added you as a co-author since I copied in some of your code. Though I wasn't sure what e-mail address to use, so I just put in a no-reply. If you want anything changed here, notice anything in this diff, etc let me know~

@dy93
Copy link

dy93 commented Jul 11, 2019

@astigefb Thanks! My email address is dy93dy93@gmail.com.

ed4edd8 -> red
965a24c -> green
c4bbbba -> Refinement

Co-authored-by: dy93 <dy93dy93@gmail.com>
@astigefb
Copy link
Contributor Author

@dy93 np :) I've added your e-mail to the commit~

@codecov-io
Copy link

Codecov Report

Merging #8670 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8670      +/-   ##
==========================================
- Coverage   63.46%   63.42%   -0.05%     
==========================================
  Files         274      274              
  Lines       11385    11385              
  Branches     2771     2771              
==========================================
- Hits         7226     7221       -5     
- Misses       3546     3547       +1     
- Partials      613      617       +4
Impacted Files Coverage Δ
...ages/jest-haste-map/src/lib/dependencyExtractor.ts 95.45% <ø> (ø) ⬆️
packages/expect/src/spyMatchers.ts 89.02% <0%> (-2%) ⬇️
packages/jest-mock/src/index.ts 77.52% <0%> (-0.2%) ⬇️

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 b76f01b...9d9b320. Read the comment docs.

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

7 participants