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

Ensure run with NODE_ENV set to test #34

Merged
merged 1 commit into from
Mar 8, 2019
Merged

Conversation

trevor-bliss
Copy link
Contributor

To mimic the environment when the jest executable is invoked directly, set process.env.NODE_ENV to 'test' if not already set.

@trevor-bliss trevor-bliss mentioned this pull request Mar 5, 2019
Copy link
Member

@pmdartus pmdartus left a comment

Choose a reason for hiding this comment

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

Why do we need branch the code depending if we run in debug mode or not? Instead of traying to match the same environment in both cases can't we use the same code path?

@@ -64,6 +64,10 @@ async function testRunner(argv) {

shell.runCommand(command, commandArgs);
} else {
// Jest will not set the env if not run from the bin executable
if (process.env.NODE_ENV == null) {
Copy link
Member

Choose a reason for hiding this comment

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

If an environment variable is not set its value is always undefined and not null.

Suggested change
if (process.env.NODE_ENV == null) {
if (process.env.NODE_ENV === undefined) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the exact code Jest uses. I want to keep parity between the two modes. It's very confusing to get a failure in a test and then try to debug and get different results.

@trevor-bliss
Copy link
Contributor Author

@pmdartus Yeah, in non-debug mode we can run Jest via the same node <path/to/jest> command without attaching the debugger or any debug specific flags. Feels like that shouldn't cause any issues.

I remember @micmmakarov did it this way for testability, but I'm sure we can find a way to have parity between commands and also make it testable. Any concerns Mikhail?

@trevor-bliss
Copy link
Contributor Author

@pmdartus I filed #36 to update the commands for next release. Let's stick with the manual NODE_ENV check for now. Mind giving this a re-review?

@pmdartus
Copy link
Member

pmdartus commented Mar 8, 2019

@trevor-bliss LGTM

@trevor-bliss trevor-bliss merged commit 1326a73 into master Mar 8, 2019
@trevor-bliss trevor-bliss deleted the tbliss/set-env-test branch March 8, 2019 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants