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

feat: add new injectGlobals config and CLI option to disable injecting global variables into the runtime #10484

Merged
merged 3 commits into from
Sep 7, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Sep 7, 2020

Summary

Fixes #10481

Test plan

E2E test added

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.

LGTM. Docs will need a cut for next minor version

@SimenB SimenB merged commit 521cd09 into jestjs:master Sep 7, 2020
@SimenB SimenB deleted the no-inject-globals branch September 7, 2020 12:32
@noomorph
Copy link
Contributor

noomorph commented Oct 5, 2020

@SimenB, looks like after this pull request the initialisation of the globals was moved to a later stage.

Speaking TestEnvironment.prototype.handleTestEvent(event, state) language, I can't see the usual this.global.(beforeAll|beforeEach|...|expect) globals when I get the first event, setup.

In the recent 26.5.1, the first event where I can see the globals available, is now add_hook (or add_test) event, which is a bit later than I would usually expect.

Was that an intended change, or an unplanned side effect?

You see, Detox custom environment has been overriding Jest's global.expect upon getting the setup event.

Could you recommend something in this case, i.e.:

  1. fixing the timing of injecting globals, so setup works...
  2. using a later event, which one, btw?..
  3. using injectGlobals: false + the environment manually injects the globals... not sure whether this is a good idea, tbh — the scenario for injectGlobals is the opposite, when your inject them from the user tests, not from the environment itself, so I am almost sure it is not a good option anyway.

Thanks in advance!

@SimenB
Copy link
Member Author

SimenB commented Oct 5, 2020

@noomorph sorry about that, not on purpose at all. Restoring original timing of injection sounds like the correct fix. Can do so tomorrow (happy to take a PR, if you're up for it)

@SimenB
Copy link
Member Author

SimenB commented Oct 6, 2020

@noomorph could you open up an issue for your use case of overriding global.expect? That won't work with the approach I made in this PR as you mention for people using @jest/globals import. I don't have a solution off the top of my head, but we should at least track it.

@noomorph
Copy link
Contributor

noomorph commented Oct 6, 2020

@SimenB, is your request still relevant? To me it looks like Object.assign(this.global, event.runtimeGlobals); is fully functional. Is there something I'm missing?

P. S. At the moment, if Detox is initialized with { initGlobals: false } behavior, then our expect function can be imported explicitly, e.g.: const { expect } = require('detox');. So, I assume implicit and explicit imports are fine for both Jest and Detox globals.

@SimenB
Copy link
Member Author

SimenB commented Oct 6, 2020

Main issue is if people do import {expect} from '@jest/globals' they'll get the wrong version. It might be fine to just address this in detox's docs, though?

@noomorph
Copy link
Contributor

noomorph commented Oct 6, 2020

@SimenB, okay, 🤔 I'll see what we can do documentation-wise.

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

Add configuration to configure Jest test methods in global environment
4 participants