add noJestGlobals config, if false, no jest-circus globals #9306
Conversation
|
Hi agilgur5! 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 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 to sign the corporate CLA. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Codecov Report
@@ Coverage Diff @@
## master #9306 +/- ##
==========================================
+ Coverage 64.99% 65.04% +0.04%
==========================================
Files 278 278
Lines 11912 11914 +2
Branches 2934 2938 +4
==========================================
+ Hits 7742 7749 +7
+ Misses 3539 3535 -4
+ Partials 631 630 -1
Continue to review full report at Codecov.
|
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
- previously was set exclusively on the global, no way to import from module - now it is done both ways
- don't set them if config for either global or project is false - still initialize test.concurrent, but directly on the imported module instead of on its global alias - requires a type-cast though
- still adds the snapshot matchers, so they should still work if one were to directly import expect
- so all the test globals (except for 'jest') can be imported from
jest-circus, no need to import from 'expect' as well
- jest-circus sets the expect global, so would make sense that you
could import from it as well
Yes that declares the jest parameter of the wrapping function, and
I suppose this would just be a
I think this could be a setting only on project config since it is specific to how the project code uses Jest? It seems so far you haven't added @SimenB is this feature and approach in general something we want at this stage? |
|
Thanks for the PR @agilgur5, and sorry for never getting to it! I've landed this partially in #9801 (including support for |
|
Yea was waiting on your guidance before making any iterations 馃槄 Hopefully my code helped/is helpful in some way |
|
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
Summed up well by #7571, #4473, #5192 (and possibly some others). With this PR, one can
import { expect, describe, it, xtest, fit } from 'jest-circus'etc (all butjest), as well as limit pollution of the global namespace (such that if you don鈥檛 import, trying to use a global will error)test.concurrentandexpect's snapshot matchers are still set, just on their imported variables instead of their global aliases like beforexit,fdescribeetc aliases are now exported like normal, not exclusively set on the global as they were beforeexpectis re-exported fromjest-circusso that everything can be imported from one place.Some things I wasn't sure of how to change (first PR 馃憢 ):
jestisn't added to the global. I couldn't find a place whereglobal.jestis set, but from what I could tell, it seemed likejest-runtimesets it(?), possibly withconstructInjectedModuleParameters()? Was digging a bit of rabbit hole searching fromjest-mocktojest-environmenttojest-runtimesearching where it gets set 馃槄Not sure how to change this if that's the right place, but the
jestfunctions do rely on a bunch of globals, so this is in last order to import directly (and likely the most complex).jest/no-globalsentry from which one could import instead (would basically re-export fromjest-circus). not sure how entries are done in this repo.globalConfig.noJestGlobals = falsebutprojectConfig.noJestGlobals = true? should the project override it if it is set totruespecifically (vs. the default ofundefined).I didn't look too much into
jest-jasmine2's globals as it is being replaced and because it seems to rely on globals a lot more and isn't as easy to analyze/parse. Might check it out in the future.Test plan
I also wasn't sure how to test this -- specifically, I need to change the jest config to set
noJestGlobalstotruefor a single test suite for this, but I'm not sure where to actually change the config.Once I figure out how to do that, I would think the tests would not be much more than the suite I have for
jest-without-globals, possibly adding some tests fortest.concurrent(test.eachprobably?) and the expect snapshot matchers, since those are dynamically added byjest-circusI also was not totally sure how/if some tests should be added to
jest-configaround validation or something for the newnoJestGlobalsconfig.