Dotenv and tests

[slifty]

Oh environment variables, how we love thee.

We use dotenv to automatically load the contents of .env for any environment variables that have not been explicitly provided to the node runtime.

The question is whether we want our tests to pull from .env or if we want to provide an engineered way to pass a potentially different set of environment variables when running tests.

OPTION 1: Pull from BOTH .env and .env.test when testing

Have globalSetup call dotenv twice to make the hierarchy explicit:

dotenv.config({ path: '.env.test' });
dotenv.config();

And then it's on the dev to set up .env.test if that is what they want, but it becomes optional if they are OK with just having .env

^ We might consider adding some kind of test safety valve that prevents integration tests from being run against the production database somehow....

OPTION 2: have tests ENSURE that a special .env.test is loaded (and NOT .env)

This would basically mean setting the DOTENV_CONFIG_PATH as part of the test globalSetup

OPTION 3: make it manual

Only ever pull from .env and require the local dev to update to point to a test database before running tests locally.

The downside of this is that we know this will be a needed manual step already.

---

I'm leaning to Option 1 -- load .env by default but provide a hook to have our tests optionally first pull from .env.test.

[bickelj]

For simplicity's sake (i.e. reliability, understanding, durability, robustness, etc.), I lean toward letting the software look for exactly .env and then trying to understand what problems that would cause. In a test environment, we use .env and point all the variables to test environment locations. In a production environment, we use .env and point all the variables to production environment locations. In a dev environment, we use .env and point all the variables to dev environment locations.

Some of the issues with the .env.test came from #60 where we would like to use a logger and also let the log level be set by an environment variable. The logger can be used in both test and production environments (and also test and production code). When we set a LOG_LEVEL, we need it to take effect in any environment. We want to integrate with the dotenv library nicely as well.

So then what about a development environment where we wish to try out production-like things and also run integration tests? Do we need to be able to have the software consider .env and .env.test and which one to load? Why do we need that? I suppose to have a separate database connection to a local "test" database rather than the database against which we npm run migrate. Isolation is a concern, for sure. In a production environment, unless someone goes and runs npm ci manually, there is no test code nor test libraries present, so npm run test should fail. That is one small safety valve already in place. But then we go back to the development environment and juggling one .env for purpose A and another .env for purpose B. Why not safely run the integration tests against the same database as the other one? The tests should be isolated by some other means regardless, right?

[slifty]

This all makes sense so I think it comes down to this:

Why not safely run the integration tests against the same database as the other one?

Truly the answer is because right now we just can't (I'm pretty sure this is why your tests are failing)

There is a bug in the migration library that causes migrations to fail in a new schema if a migrations table already exists in ANY schema in a given current database (which is the case if migrations have been run by a local dev). This means if you run your local tests against a database where you've previously run migrations (e.g. in order to serve the application) then the integration tests cannot run the migrations and will fail.

I think that until they merge my patch OR we decide to give up on them and roll our own build of it, we're in a bit of a rut where we either have to manually update .env before running integration tests or manually drop the database before running integration tests.

This is why I think the option of a .env.test would be helpful in the short term (we can do it as just one extra line in our globalSettings.ts -- though I appreciate the complexity downsides at a design level)

So we ditch any fiddling with DOTENV_CONFIG_PATH or whatever, we just have our globalSettings load both .env.test and .env, and we document why we have that option (and once the patch is made we can remove the option, since to your point the tests should be written in a way that is isolated, and indeed they are)

[bickelj]

If this works, I will go with it in #59/#60:

dotenv.config({ path: '.env.test' });
dotenv.config();

Reading it though, I would imagine it means that the variables will be production variables rather than test variables, right? And then what happens when logger.ts comes along and does dotenv.config() again? Or should we move that out and up?

Edit: no, the order of precedence is "earlier/outer if already set." So if I only set it in .env.test, that is what comes through. If I set it only in .env that is what comes through. If I set it in neither and not on command line, then the default is used. If I set it on command line, that is what comes through.

[slifty]

variables will be production variables rather than test variables

I may be misunderstanding the question but basically under the "load both" approach if .env.test is NOT populated it's as though it never existed and was never loaded. I think your arguments for only using .env are very good and completely agree with them. I see this .env.test thing as a temporary local-quality-of-life patch until the upstream bug is fixed

So basically I think that the intent here is that .env is treated the way you are intending it to be treated -- in our dev, test, and prod instances we ONLY use .env

The only time .env.test is used is for this obnoxious buggy local environment case where we the need for a convenient test override becomes a bigger deal.

what happens when logger.ts comes along

dotenv is OK with being called more than once (it just won't do anything later since the vars are already set)

That means the logger.ts won't cause trouble so long as those two lines are run before logger.ts is loaded.

---

I just saw your edit and yes that's it.

[slifty]

^ You may even want to add a note / TODO / issue link around the {.env.test} import just to call out how it is intended to have a short life in our repo.

[bickelj]

The most recent push to #60 has several such notes.

[bickelj]

The solution in #60 has been working fine for the last few months. When ThomWright/postgres-migrations#93 gets merged we have the option of re-evaluating the .env situation and re-open.