-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Conversation
Hmm. How would this work in or |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@gaearon That is a good, point I'll remove it. Another question... could |
In theory it might make sense to support it. Not sure I see practical use case. |
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. Also, I'll give this a try with the tests that you linked above and I'll let you know how it goes. |
Yeah, if it's not supported then it should error. If somebody later asks for it we can look at a use case. |
My implementation is still rough and I would need to change some things, but it seems to be working. facebook/react@master...rogeliog:with-reset-modules NOTE: I'm not sure if replacing all the |
4af7d27
to
65e426e
Compare
@gaearon do you know if the changes that I did on |
I think they make sense. Maybe |
withResetModules
jest.isolateModules
for scoped module initialization
@SimenB, @rickhanlonii, @cpojer any thoughts? |
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? |
I forgot to remove that, yes it is ready for review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
docs/Configuration.md
Outdated
|
||
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). |
There was a problem hiding this comment.
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 () => |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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
@gaearon have you been able to test this? Does it work for your use case? |
There was a problem hiding this 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 🚀
docs/Configuration.md
Outdated
let myModule; | ||
jest.isolateModules(() => { | ||
myModule = require('myModule'); | ||
}); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
packages/jest-runtime/src/index.js
Outdated
_moduleMocker: ModuleMocker; | ||
_sandboxModuleRegistry: ?ModuleRegistry; |
There was a problem hiding this comment.
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 👍.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
* 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) ...
* 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
hey! I know this is an old thread, but what do you think about an async version of isolateModules? I'm using await jest.isolateModules.async(async () => (await import('something')).default) |
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. |
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 caseI madewithResetModules
async, so that a user can wait for some async code to be executed inside of the function before moving forward with the rest.