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 jest.isolateModules for scoped module initialization #6701

Merged
merged 14 commits into from
Dec 18, 2018

Conversation

rogeliog
Copy link
Contributor

@rogeliog rogeliog commented Jul 16, 2018

Summary

Fixes: #6174

This PR provides an interface(jest.isolateModules) for scoped module initialization/cleanup.

This was originally proposed by @gaearon, and a similar proposal was done by @benmj #6638

~I initially built this with the idea that you could have multiple nested withResetModules, but I'm not sure if that would be a valid use case

I made withResetModules async, so that a user can wait for some async code to be executed inside of the function before moving forward with the rest.

  • Add documentation

@gaearon
Copy link
Contributor

gaearon commented Jul 16, 2018

I made withResetModules async, so that a user can wait for some async code to be executed inside of the function before moving forward with the rest.

Hmm. How would this work in beforeEach? Here's what we're doing today:

https://github.com/facebook/react/blob/43ffae2d1750ee32409e140e41ea562ff46d2516/packages/react-art/src/__tests__/ReactART-test.js#L18-L33

or

https://github.com/facebook/react/blob/43ffae2d1750ee32409e140e41ea562ff46d2516/packages/react-dom/src/__tests__/ReactServerRenderingBrowser-test.js#L19-L24

@codecov-io
Copy link

codecov-io commented Jul 16, 2018

Codecov Report

Merging #6701 into master will increase coverage by 0.03%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6701      +/-   ##
==========================================
+ Coverage   66.54%   66.58%   +0.03%     
==========================================
  Files         241      241              
  Lines        9316     9336      +20     
  Branches        5        6       +1     
==========================================
+ Hits         6199     6216      +17     
- Misses       3114     3117       +3     
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-runtime/src/index.js 77.47% <88.88%> (+0.48%) ⬆️

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 c22d9f5...380e9c6. Read the comment docs.

@rogeliog
Copy link
Contributor Author

@gaearon That is a good, point I'll remove it.

Another question... could withResetModules be nested inside each other?

@gaearon
Copy link
Contributor

gaearon commented Jul 17, 2018

Another question... could withResetModules be nested inside each other?

In theory it might make sense to support it. Not sure I see practical use case.

@rogeliog
Copy link
Contributor Author

I agree with that. I think the API makes it seem as if it should be supported, but it does add a bit of complexity to the implementation given that it needs to keep a stack of sandbox registries, compared to just one.

We could throw an error if the user tries to nest them. withResetModules cannot be nested inside another withResetModules.

Also, I'll give this a try with the tests that you linked above and I'll let you know how it goes.

@gaearon
Copy link
Contributor

gaearon commented Jul 17, 2018

Yeah, if it's not supported then it should error. If somebody later asks for it we can look at a use case.

@rogeliog
Copy link
Contributor Author

rogeliog commented Jul 18, 2018

My implementation is still rough and I would need to change some things, but it seems to be working.
@gaearon I went ahead changed all the resetModules() in facebook/react to withResetModules(() => {}), and the full React test suite seems to be passing

facebook/react@master...rogeliog:with-reset-modules

NOTE: I'm not sure if replacing all the resetModules() with withResetModules(() => {}) is something that makes sense for the React codebase, I just did it to better test my withResetModules implementation

@rogeliog rogeliog force-pushed the with-reset-modules branch from 4af7d27 to 65e426e Compare July 23, 2018 00:57
@rogeliog
Copy link
Contributor Author

@gaearon do you know if the changes that I did on facebook/react to support withResetModules make sense?

@gaearon
Copy link
Contributor

gaearon commented Aug 19, 2018

I think they make sense.

Maybe isolateModules would be a better name?

@rogeliog rogeliog changed the title [PoC][WIP] scoped module initialization withResetModules Add jest.isolateModules for scoped module initialization Oct 5, 2018
@rogeliog
Copy link
Contributor Author

rogeliog commented Oct 11, 2018

@SimenB, @rickhanlonii, @cpojer any thoughts?

@SimenB
Copy link
Member

SimenB commented Oct 11, 2018

Disclaimer: It is still in a rough state, needs to be cleaned up and still has some issues.

What issues? Or do you think it's ready for review?

EDIT: Looking though quickly now nothing popped up, at least. Mind adding some docs?

@rogeliog
Copy link
Contributor Author

What issues? Or do you think it's ready for review?

I forgot to remove that, yes it is ready for review

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Looks good!


Default: `false`

`isolateModules` goes a step further than `resetModules` and creates a sanbox registry for the modules that are loaded inside the callback function. This is useful to isolate modules for every test so that local module state doesn't conflict between tests. This can be done programmatically using [`jest.isolateModules()`](#jest-isolatemodules).
Copy link
Member

Choose a reason for hiding this comment

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

Typo: sanbox

expect(exports.getState()).toBe(1);
}));

it('cannot nest isolateModules blocks', async () =>
Copy link
Member

Choose a reason for hiding this comment

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

Async?

* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*
* @providesModule ModuleWithState
Copy link
Member

Choose a reason for hiding this comment

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

We've cleaned out providesModule from master

@SimenB
Copy link
Member

SimenB commented Oct 12, 2018

@gaearon have you been able to test this? Does it work for your use case?

@SimenB SimenB requested a review from thymikee October 12, 2018 22:57
@rogeliog
Copy link
Contributor Author

@gaearon @cpojer any thoughts?

@SimenB SimenB added this to the Jest 24 milestone Oct 17, 2018
@rogeliog rogeliog requested review from gaearon and cpojer October 18, 2018 23:24
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.

Small adjustments to the docs and we're good to land it 🚀

let myModule;
jest.isolateModules(() => {
myModule = require('myModule');
});
Copy link
Collaborator

@thymikee thymikee Oct 28, 2018

Choose a reason for hiding this comment

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

I think we should move the example to https://jestjs.io/docs/en/jest-object (and fix the link to jest.isolateModules() above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I think it should only live in jest-object. This was my bad.

_moduleMocker: ModuleMocker;
_sandboxModuleRegistry: ?ModuleRegistry;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think _isolatedModuleRegistry would play slightly better with the name isolateModules but I'm good with both names 👍.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I like it way better!

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.

Awesome!

@rogeliog
Copy link
Contributor Author

@gaearon @cpojer any thoughts?

@thymikee
Copy link
Collaborator

@SimenB let's merge it and iterate when needed. Thanks @rogeliog for taking a stab at it!

@thymikee thymikee merged commit 873c641 into jestjs:master Dec 18, 2018
thymikee added a commit to spion/jest that referenced this pull request Dec 18, 2018
* master: (24 commits)
  Add `jest.isolateModules` for scoped module initialization (jestjs#6701)
  Migrate to Babel 7 (jestjs#7016)
  docs: changed "Great Scott!" link (jestjs#7524)
  Use reduce instead of filter+map in dependency_resolver (jestjs#7522)
  Update Configuration.md (jestjs#7455)
  Support dashed args (jestjs#7497)
  Allow % based configuration of max workers (jestjs#7494)
  chore: Standardize filenames: jest-runner pkg (jestjs#7464)
  allow `bail` setting to control when to bail out of a failing test run (jestjs#7335)
  Add issue template labels (jestjs#7470)
  chore: standardize filenames in e2e/babel-plugin-jest-hoist (jestjs#7467)
  Add node worker-thread support to jest-worker (jestjs#7408)
  Add `testPathIgnorePatterns` to CLI documentation (jestjs#7440)
  pretty-format: Omit non-enumerable symbol properties (jestjs#7448)
  Add Jest Architecture overview to docs. (jestjs#7449)
  chore: run appveyor tests on node 10
  chore: fix failures e2e test for node 8 (jestjs#7446)
  chore: update docusaurus to v1.6.0 (jestjs#7445)
  Enhancement/expect-to-be-close-to-with-infinity (jestjs#7444)
  Update CHANGELOG formatting (jestjs#7429)
  ...
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
* First iteration of withResetModules

* Make withResetModules "not async"

* Reimplement withResetModules without the stack

* Rename withResetModules to isolateModules

* Add changelog entry

* Fix eslint

* Add docs for isolateModules

* Fix typo, remove provideModules and extra async stuff

* Update Configuration.md

* Rename implementation to isolated* and change docs to JestObjectAPI
@dreyks
Copy link

dreyks commented Jul 17, 2020

hey! I know this is an old thread, but what do you think about an async version of isolateModules? I'm using import() instead of require in my tests, so I'd love to be able to do something like

await jest.isolateModules.async(async () => (await import('something')).default)

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

Scoped module initialization / cleanup
7 participants