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

process.env.PATH missing in tests on Windows #3014

Closed
emilis-ideait opened this issue Apr 22, 2022 · 3 comments
Closed

process.env.PATH missing in tests on Windows #3014

emilis-ideait opened this issue Apr 22, 2022 · 3 comments

Comments

@emilis-ideait
Copy link

TL;DR: { ...process.env }.PATH is undefined on Windows. Probably need to fix lib/fork.js.

Tried to do:

I upgraded Ava from 2.4.0 to 4.2.0 in our internal project and tests started failing on our Windows build server with this message:

Error: Cannot read property 'split' of undefined TypeError: Cannot read property 'split' of undefined
          at Object.findInPath (...\node_modules\selenium-webdriver\io\index.js:215:45)

The failing code was trying to execute process.env['PATH'].split().

I added some console.log() statements to our tests and found that process.env.PATH is missing, but process.env.Path is available instead.

Expected:

process.env.PATH available in tests on Windows.

Investigation:

  1. process.env.PATH is problematic in Node.js on Windows. See related issues: https://github.com/nodejs/node/issues?q=is%3Aissue+process.env.PATH+

  2. I suspect this code (I'm not familiar with Ava codebase) in lib/fork.js:

    • env: {NODE_ENV: 'test', ...process.env, ...options.environmentVariables},

This minimal code in Node.js on windows shows what's happening with ...process.env:

image

Temporary workaround for other users

I renamed Path to PATH in Windows System Variables.

(Settings -> System -> About -> Advanced system settings -> Environment Variables...)

This fixed the issue with the tests failing, but now the tests get both process.env.PATH AND process.env.Path, which is different to how it works in Node.js (you only see Path when printing out process.env).

@novemberborn
Copy link
Member

...process.env is meant to do a shallow copy of the process.env object. However the Node.js documentation says this:

On Windows operating systems, environment variables are case-insensitive.

https://nodejs.org/api/process.html#processenv

This implies some magic that would not survive the copying, though presumably at least one version of the variable is copied. That said, I'd think within the tests Node.js should again make the variables case-insensitive.

What Node.js version are you using? And could you share your AVA config? Could you try disabling AVA's use of worker threads to see if that helps? What does console.dir({...process.env}) print?

@emilis-ideait
Copy link
Author

I am using Node.js v14.19.1 on Windows.

Turning off workerThreads helped.

I created a small repository to test the issue: https://github.com/emilis-ideait/ava-process-env-path-on-windows

npm test in this repository fails:
image

npm run test-no-worker-threads succeeds:
image

I should be able to test with Node.js v16 on Windows in a couple of weeks.

@novemberborn
Copy link
Member

My impression is that somehow in worker threads process.env doesn't behave the same as in the main thread. This seems like unexpected behavior on Node.js part.

@novemberborn novemberborn closed this as not planned Won't fix, can't repro, duplicate, stale Jul 3, 2023
ericcornelissen added a commit to ericcornelissen/shescape that referenced this issue Aug 20, 2023
Update the implementation of `resolveExecutable` to accept the
environment variables so that they can explicitly be provided to
which [1]. All internal code and tests have been updated accordingly,
no external changes.

This is in an effort to fix a problem (bug?) where environment variables
aren't always passed on correctly to subprocesses. The problem is
described in [2], though if Shescape is used in a forked process on
Windows it would face the same problem.

Funny to see that `getShellName` name unit tests for both Unix and
Windows already generated and provided environment variables (:

--
1. https://www.npmjs.com/package/which
2. avajs/ava#3014
ericcornelissen added a commit to ericcornelissen/shescape that referenced this issue Aug 21, 2023
Update the implementation of `resolveExecutable` to accept the
environment variables so that they can explicitly be provided to
which [1]. All internal code and tests have been updated accordingly,
no external changes.

This is in an effort to fix a problem where environment variables aren't
always passed on correctly to subprocesses. The problem is described in
[2], though if Shescape is used in a forked process on Windows it would
face the same problem. (As a problem found through the test runner, use
of which [1] in tests has been updated similarly.

As a result of this change, the integration and e2e test on Windows are
quite a bit slower (because now they actually look up the executable).
Hence, both of these suites have been updated to use separated test
files in order to better leverage AVA's concurrency - speeding up
individual files and avoiding timeouts as well as running tests quicker
if cores are available.

This change also allowed for including executables without a file
extension in the integration and e2e tests, further improving coverage.
(This replaced the `.EXE` coverage as extensionless executables are
resolved to `.EXE`, thus reducing test count without reducing coverage.)

As a side note, it was funny to see that `getShellName` name unit tests
for both Unix and Windows already generated and provided environment
variables (:

--
1. https://www.npmjs.com/package/which
2. avajs/ava#3014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants