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: write snapshots to disk even if FS is mocked #7080

Merged
merged 5 commits into from
Jan 7, 2019

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Sep 30, 2018

Summary

We have a general problem in that when a user messes with globals, or requires node core modules, they get the same copy as we get. That means we have to make sure to work, even if users do global.Promise = undefined or fs.writeFileSync = () => {}.

We really should solve it generally (stick all globals and core modules on some Symbol known only to Jest?), but this solves it for the case of snapshots and FS mocking.

Fixes #5270. Without the changes in this PR, the snapshot would be reported as written, but not actually written to disk.

Test plan

Added integration test

Copy link
Contributor

@mattphillips mattphillips left a comment

Choose a reason for hiding this comment

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

LGTM! It would be good to solve this more generally and across all packages

@SimenB
Copy link
Member Author

SimenB commented Sep 30, 2018

Flow is being dumb and not understanding bind. I thought that was TS's thing...

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

👍 but I agree with @mattphillips

@SimenB
Copy link
Member Author

SimenB commented Oct 1, 2018

Of course, we've had that issue for years, though. And I noted it in the OP. Ideas welcome! :D

@thymikee
Copy link
Collaborator

@SimenB can we re-implement this with babel-plugin-jest-native-globals.js?

@SimenB
Copy link
Member Author

SimenB commented Dec 24, 2018

Ah, good idea!

@SimenB
Copy link
Member Author

SimenB commented Dec 24, 2018

@mattphillips @thymikee updated

@@ -57,6 +66,47 @@ module.exports = ({template}) => {

path.parentPath.replaceWithSourceString('jestNow');
}
if (
Copy link
Member Author

Choose a reason for hiding this comment

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

lots of copypasta here, should probably refactor this at some point

@codecov-io
Copy link

Codecov Report

Merging #7080 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7080   +/-   ##
=======================================
  Coverage   67.91%   67.91%           
=======================================
  Files         248      248           
  Lines        9507     9507           
  Branches        6        5    -1     
=======================================
  Hits         6457     6457           
  Misses       3048     3048           
  Partials        2        2
Impacted Files Coverage Δ
packages/jest-util/src/installCommonGlobals.js 100% <ø> (ø) ⬆️

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 df56fca...a63d8fb. Read the comment docs.

@thymikee
Copy link
Collaborator

thymikee commented Jan 7, 2019

@SimenB mind fixing prettier and merge this? :)

@SimenB SimenB merged commit 938f146 into jestjs:master Jan 7, 2019
@SimenB SimenB deleted the snapshot-mock-fs branch January 7, 2019 10:53
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@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.

Snapshots are reported to be written but are not
5 participants