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

Feature request: set NODE_ENV=test #1470

Closed
AJamesPhillips opened this issue Jul 26, 2017 · 14 comments
Closed

Feature request: set NODE_ENV=test #1470

AJamesPhillips opened this issue Jul 26, 2017 · 14 comments
Labels

Comments

@AJamesPhillips
Copy link

Description

To set process.env.NODE_ENV='test' if it is not already set.

@sindresorhus
Copy link
Member

Can you check if any other test runner like Mocha, Jest, QUnit, etc, sets this? Any prior art?

@AJamesPhillips
Copy link
Author

AJamesPhillips commented Jul 27, 2017

The prior art I can find is:

"Actively" not set for:

Could not find for:

  • Jasmine
  • Karma
  • QUnit
  • Enzyme
  • Cucumber
  • tape

Not a testing framework but a framework for building front end web apps:

So it's mixed. I'm going to close this for now but happy to have a discussion and consider this proposal if you'd like to take it forwards.

@sindresorhus
Copy link
Member

We can keep it open to see what the rest of the team and anyone else thinks.

@sindresorhus sindresorhus reopened this Jul 27, 2017
@maxgallo
Copy link

maxgallo commented Jul 28, 2017

We're using NODE_ENV=test for the tests in my team to use different .babelrc configurations. So it's a 👍

@novemberborn
Copy link
Member

On the one hand, I've only run into a situation where I needed NODE_ENV=test once. On the other, I can't think of any reason it would have broken my tests.

I'm less interested in using it to control how babel-register selects its configuration though (I'd prefer the explicitness of BABEL_ENV). It's useful for testing databases and environment variable loaders like envalid, where you may need to set default values for a test environment.

I'm leaning towards 👍 on this, but I wonder if the default value ('test') should be configurable in the package.json#ava object.

@sindresorhus what's your preference on this?

@sindresorhus
Copy link
Member

but I wonder if the default value ('test') should be configurable in the package.json#ava object.

Neh, not until someone asks for it and provides a good use-case I would say.

@sindresorhus what's your preference on this?

I'm 👍 as long as we have a warning in the docs about its dangers. You should not use it extensively in your code, as you're then no longer testing the actual code paths, but rather the ones made for the tests.

@novemberborn
Copy link
Member

Cool, let's do this 👍

@dashersw
Copy link

This is very heart-breaking. NODE_ENV is such a crucial piece that ava taking such a bold decision is simply unfathomable. There are so many use cases that change their behavior based on NODE_ENV and even on its absence, they all now break.

I think the worst part is we can't force ava to not set it, deliberately leaving NODE_ENV empty. This breaks log-suppress, and probably many others.

@AJamesPhillips
Copy link
Author

Hey @dashersw sorry to hear you've obviously encountered hassle with this change. We did look at prior art to see what others were doing and why. From looking at the implementation of #1523 it would be possible to set process.env.NODE_ENV = undefined before ava and this should / might result in the existing behaviour? Does that help?

@dashersw
Copy link

@AJamesPhillips how do you mean, before ava? The implementation uses Object.assign, which doesn't work the way you suggested. If you set NODE_ENV to undefined assign will overwrite it. 'NODE_ENV' in process.env would be a better check for the existence of the key, even if it's undefined, so NODE_ENV= ava would simply work, which is not the case now.

But apart from these, why the need for such a feature? Apparently ava itself doesn't make use of this feature. Why was this approach with many implications chosen over simply typing NODE_ENV=test ava instead of ava? Setting such an important environment variable without actually using it is a very interesting choice.

@AJamesPhillips
Copy link
Author

AJamesPhillips commented Nov 29, 2017

Object.assign, which doesn't work the way you suggested.

Object.assign({NODE_ENV: 'test'}, {NODE_ENV:undefined}) returns {NODE_ENV:undefined}, not sure what you're seeing?

so NODE_ENV= ava would simply work, which is not the case now.

Run yarn add ava, make a test.js file with:

import test from 'ava';

test(t => {
    console.log('NODE_ENV... "' + process.env.NODE_ENV + '"');
    t.deepEqual([1, 2], [1, 2]);
});

Running: NODE_ENV= yarn run ava gives:

yarn run v0.27.5
warning package.json: No license field
$ "/Users/ajp/projects/fluid_dynamics/node_modules/.bin/ava"
NODE_ENV... ""

  1 passed
Done in 3.28s.

So that should be fine. Shout if you have any problems with that @dashersw

@dashersw
Copy link

That unfortunately sets NODE_ENV to an empty string, such that process.env.NODE_ENV === '' is true. But this is different than NODE_ENV === undefined, so in this case, log-suppress still doesn't work.

@AJamesPhillips
Copy link
Author

AJamesPhillips commented Nov 30, 2017

Got it. You can use this:

process.env.NODE_ENV = undefined
import test from 'ava';

test(t => {
    console.log('NODE_ENV... "' + process.env.NODE_ENV + '"'); // logs out:  NODE_ENV... "undefined"
    t.deepEqual([1, 2], [1, 2]);
});

Does that let log-suppress work? Alternatively submit a fix for log-suppress. Actually the docs also show:

process.env.NODE_ENV = 'test'
require("log-suppress").init(console, 'test');

So it seems you should use: require("log-suppress").init(console, process.env.NODE_ENV); though you should then set NODE_ENV explicitly using NODE_ENV=whatever ava or make sure ava is imported and run before log-suppress.

@novemberborn
Copy link
Member

With NODE_ENV, I think the absence of a particular value (production, test) is relevant, not whether process.env.NODE_ENV === undefined. For edge cases where this is critical you can use AVA's require option to modify process.env before your test files are loaded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants