-
Notifications
You must be signed in to change notification settings - Fork 40
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
Safeguard tests from using .env #1277
Conversation
Makefile
Outdated
NODE_ENV=test $(BIN)/mocha $(shell find api -name '*.test.coffee') | ||
NODE_ENV=test $(BIN)/mocha $(shell find api -name '*.test.js') | ||
NODE_ENV=test $(BIN)/mocha $(shell find client -name '*.test.coffee') | ||
NODE_ENV=test $(BIN)/jest $(shell find client -name '*.test.js') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is necessary since mocha will pick up on test/mocha.opts
but JIC..
While we're looking at the env file, maybe it'd be better to use dotenv to preload the separate files via |
@craigspaeth you're right, that's a much cleaner solution 👍 |
4c24163
to
e5a23fd
Compare
Typos for README.md
Got false positives?Make changes to the global settings spellcheck.json in /artsy/artsy-danger. New dependencies added: dotenv. dotenvAuthor: scottmotte Description: Loads environment variables from .env file Homepage: https://github.com/motdotla/dotenv#readme
|
e5a23fd
to
a8488ab
Compare
Nice! Thanks for embarking on this! |
This PR safeguards against a bug where importing/requiring this test helper after importing/requiring the app will cause the helper to use your .env file instead of .env.test.
Problem
To replicate the issue, require the
app
before the helper. (Don't try this now unless you're pointing to a local database in your .env)This causes
app
to be loaded with noprocess.env.NODE_ENV
set which means this line gets executed and all subapps get loaded with your local.env
settings. If you swap the order of the require, it behaves correctly. This is why:We've been miraculously getting by without issues until now, when I accidentally wiped out staging yesterday when running a test locally with the problematic order.
Solution
We need to ensure that our
NODE_ENV
in a test setting is working properly. By setting theNODE_ENV
on themocha.opts
level, we can ensure that every test that runs with the right environment. Thejest
settings don't need to be updated since this only affects API work.NODE_ENV=test
on all mocha testsapi/index.coffee
to switch environments based onNODE_ENV
.cc @joeyAghion @eessex @damassi