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 noJestGlobals config, if false, no jest-circus globals #9306

Closed
wants to merge 4 commits into from

Conversation

agilgur5
Copy link

@agilgur5 agilgur5 commented Dec 15, 2019

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 but jest), as well as limit pollution of the global namespace (such that if you don’t import, trying to use a global will error)

  • test.concurrent and expect's snapshot matchers are still set, just on their imported variables instead of their global aliases like before
  • the xit, fdescribe etc aliases are now exported like normal, not exclusively set on the global as they were before
  • expect is re-exported from jest-circus so that everything can be imported from one place.

Some things I wasn't sure of how to change (first PR 👋 ):

  • how to make sure jest isn't added to the global. I couldn't find a place where global.jest is set, but from what I could tell, it seemed like jest-runtime sets it(?), possibly with constructInjectedModuleParameters()? Was digging a bit of rabbit hole searching from jest-mock to jest-environment to jest-runtime searching where it gets set 😅
    Not sure how to change this if that's the right place, but the jest functions do rely on a bunch of globals, so this is in last order to import directly (and likely the most complex).
  • creating a jest/no-globals entry from which one could import instead (would basically re-export from jest-circus). not sure how entries are done in this repo.
  • what to do if globalConfig.noJestGlobals = false but projectConfig.noJestGlobals = true? should the project override it if it is set to true specifically (vs. the default of undefined).

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 noJestGlobals to true for 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 for test.concurrent (test.each probably?) and the expect snapshot matchers, since those are dynamically added by jest-circus

I also was not totally sure how/if some tests should be added to jest-config around validation or something for the new noJestGlobals config.

@facebook-github-bot
Copy link
Contributor

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 [email protected]. Thanks!

@codecov-io
Copy link

codecov-io commented Dec 15, 2019

Codecov Report

Merging #9306 into master will increase coverage by 0.04%.
The diff coverage is 46.15%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
...us/src/legacy-code-todo-rewrite/jestAdapterInit.ts 0% <0%> (ø) ⬆️
...circus/src/legacy-code-todo-rewrite/jestAdapter.ts 0% <0%> (ø) ⬆️
...-circus/src/legacy-code-todo-rewrite/jestExpect.ts 0% <0%> (ø) ⬆️
packages/jest-circus/src/index.ts 70.12% <100%> (+2.07%) ⬆️
packages/expect/src/utils.ts 96.2% <0%> (+1.26%) ⬆️

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 446d174...f7d9db1. Read the comment docs.

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

- 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
@jeysal
Copy link
Contributor

jeysal commented Dec 15, 2019

  • constructInjectedModuleParameters

Yes that declares the jest parameter of the wrapping function, and _createJestObjectFor and its usage are where it is created. This is definitely a lot less trivial for the jest object, I think the runtime would need a way to specify when the jest global should not be passed into the script and the test framework could use that when calling requireModule with the test path. Seems like it requires a lot of changes to jest-runtime.

  • creating a jest/no-globals entry

I suppose this would just be a no-globals.js file in packages/jest? Could also be nice to avoid reexporting expect from jest-circus, where it does not really belong

  • globalConfig.noJestGlobals = false but projectConfig.noJestGlobals = true

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 jest-config properties at all


@SimenB is this feature and approach in general something we want at this stage?

@SimenB
Copy link
Member

SimenB commented Apr 12, 2020

Thanks for the PR @agilgur5, and sorry for never getting to it! I've landed this partially in #9801 (including support for jest). I have a branch locally which also adds a config option to not add it to global. As you note, getting that to work with jasmine is finicky, so I might just bother with circus for now.

@agilgur5
Copy link
Author

Yea was waiting on your guidance before making any iterations 😅 Hopefully my code helped/is helpful in some way

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

5 participants