nathanjhood / esbuild-scripts

esbuild-flavoured 'react-scripts'.
Other
0 stars 0 forks source link

Why does this test pass? #23

Closed nathanjhood closed 2 months ago

nathanjhood commented 2 months ago
import type Test = require('node:test');
import test = require('node:test');
import path = require('node:path');

test.suite('parseEnv()', { timeout: 10000 }, (suiteContext) => {
  test.test(
    'loadEnvFile',
    { signal: suiteContext.signal },
    (testContext: Test.TestContext) => {
      const before = process.env;
      process.loadEnvFile(path.resolve(process.cwd(), '.env.example'));
      const after = process.env;
      testContext.assert.deepStrictEqual(before, after);
    }
  );
});
nathanjhood commented 2 months ago

Here's the fix (spot the difference):

import type Test = require('node:test');
import test = require('node:test');
import path = require('node:path');

test.suite('parseEnv()', { timeout: 10000 }, (suiteContext) => {
  test.test(
    'loadEnvFile',
    { signal: suiteContext.signal },
    (testContext: Test.TestContext) => {
      const before = process.env;
      console.log(before);
      process.loadEnvFile(path.resolve(process.cwd(), '.env.example'));
      const after = process.env;
      testContext.assert.deepStrictEqual(before, after);
      console.log(before);
    }
  );
});

Its' as if doing const before = process.env gives you only a reference instead of an instance... well, I never.

I will look into this further...!

nathanjhood commented 2 months ago

Confirmed: before only represents the state of process.env as it currently is whenever before gets called for the first time.

Broken:

import type Test = require('node:test');
import test = require('node:test');
import path = require('node:path');

test.suite('parseEnv()', { timeout: 10000 }, (suiteContext) => {
  test.test(
    'loadEnvFile',
    { signal: suiteContext.signal },
    (testContext: Test.TestContext) => {
      const before = process.env; // 'before' is not used until *after* 'loadEnvFile'...
      process.loadEnvFile(path.resolve(process.cwd(), '.env.example'));
      const after = process.env;
      testContext.assert.deepStrictEqual(before, after);
    }
  );
});

Fixed:

import type Test = require('node:test');
import test = require('node:test');
import path = require('node:path');

test.suite('parseEnv()', { timeout: 10000 }, (suiteContext) => {
  test.test(
    'loadEnvFile',
    { signal: suiteContext.signal },
    (testContext: Test.TestContext) => {
      const before = process.env;
      console.log(before); // 'before' is used until *before* 'loadEnvFile'...
      process.loadEnvFile(path.resolve(process.cwd(), '.env.example'));
      const after = process.env;
      testContext.assert.deepStrictEqual(before, after);
    }
  );
});

console.log itself is of course not the solution... it seems a case that the variable is not actually assigned until used...

Is this a test thing?

Easily verified...

nathanjhood commented 2 months ago

Ok the process.env object is strange...

Taking env vars behaves as expected:

// tst.ts
const t = process.env.BUILD_DIR;
console.log(t);
process.loadEnvFile(__dirname + '/.env.example');
console.log(t);
$ yarn tsx ./tst.ts
yarn run v1.22.22
$ /***/node_modules/.bin/tsx ./tst.ts
undefined
undefined
Done in 2.60s.

t didn't change, since it was taken from before loadEnvFile - this is correct

But the env object itself...

const t = process.env;
console.log(t.BUILD_DIR);
process.loadEnvFile(__dirname + '/.env.example');
console.log(t.BUILD_DIR);
$ yarn tsx ./tst.ts
yarn run v1.22.22
$ /***/node_modules/.bin/tsx ./tst.ts
undefined
dist
Done in 2.60s.

t did change - this is correct

nathanjhood commented 2 months ago

Lots of interest points around this:

We should probably re-implement a dotenv-style loader within the test runner....

nathanjhood commented 1 month ago

Should wrap this up with a comment :D

NodeJS' process.loadEnvFile() has some wierd implementation - it seems to not really care which process instance you attach it to; it will always assign to the global.process...

They probably have good reasons for this, if intentional. However, this makes it a nightmare to test; calling any function or method which calls loadProcessEnv() in its' implementation will mutate the running process object, not the mock process object.

That is a massive no - we can't expect the rest of the tests to run with confidence, knowing that we've completely mutated the environment asyncronously at some undetermined point... clearly a big no.

So, I'm splitting the functionality such that parseEnv is mostly just a wrapper around loadEnvFile, such that we can mostly rely on team NodeJS's own tests. The client-side parsing can happen exclusively in getClientEnvironment, which is actually test-able.

Those parse* helpers will mostly be useful as an opportunity to back-fill support in older versions of NodeJS using things closer to the OG react-scripts that predate NodeJS 20.