avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

Add option to filter invalid arguments from `execArgv` when launching worker threads #3207

Closed shellscape closed 2 months ago

shellscape commented 1 year ago

Please provide details about:

I'm using a task runner that's constructing Node commands as such:

node --title svc-test:test ../../node_modules/ava/entrypoints/cli.mjs

--title is not valid for creating a worker.

✘ Internal error
  Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid execArgv flags: --title
  Error [ERR_WORKER_INVALID_EXEC_ARGV]: Initiated Worker with invalid execArgv flags: --title
      at new NodeError (node:internal/errors:399:5)
      at new Worker (node:internal/worker:192:13)
      at createWorker (file:///Users/user/code/test/node_modules/.pnpm/ava@5.3.0/node_modules/ava/lib/fork.js:24:12)
      at loadFork (file:///Users/user/code/test/node_modules/.pnpm/ava@5.3.0/node_modules/ava/lib/fork.js:84:39)
      at pMap.concurrency.concurrency (file:///Users/user/code/test/node_modules/.pnpm/ava@5.3.0/node_modules/ava/lib/api.js:289:20)
      at file:///Users/user/code/test/node_modules/.pnpm/p-map@5.5.0/node_modules/p-map/index.js:141:26

I'd expect that Ava would filter out options that would throw. Probably related https://github.com/nodejs/node/issues/41103#issuecomment-1054384840

ghost commented 1 year ago

I also want to do this! Would like to access GC control in my tests, with one of these options:

Temporarily I disabled workers to get it running.

export default {
  verbose: true,
  workerThreads: false
}
novemberborn commented 1 year ago

@shellscape I'm not sure what the best way out of this is.

We do explicitly forward the execArgv, maybe that is not necessary? You can use the nodeArguments option to specify additional arguments.

I think not forwarding execArgv would be a breaking change so now's the time.

novemberborn commented 1 year ago

Looks like the explicit process.execArgv reference was added for a debugging feature back in 2016: https://github.com/avajs/ava/pull/874

The problem though is we need to pass a flag to enable source maps, which means we can never inherit the default execArgv: https://github.com/avajs/ava/blob/63987725957114ea92dab430b7795ecc649ff107/lib/fork.js#L17

Filtering the list seems problematic since it may change between Node.js releases, unless there's a third party dependency we can use for that.

shellscape commented 1 year ago

Yeah it's a tricky one. This is a documented issue across the Node ecosystem as well - it's a gap between process and workers in Node and so far the Node core team won't address it. The tool I was using has since removed use of --title from their code, so my immediate cause is fixed. And that's probably the process for most users - asking their tooling authors (or their own team) to remove that parameter until Node itself acts on some compatibility. Filtering will probably work in the near term, so probably not so problematic until it breaks (if it ever does) again.

If that doesn't feel right, adding to the docs about this edge case would probably suffice.

novemberborn commented 1 year ago

Having thought about this some more, we could add a filterExecArgv() through the ava.config.* files. It could receive the individual values allowing end-users to discard incompatible ones.

As for documentation, how about this? https://github.com/avajs/ava/pull/3215

novemberborn commented 1 year ago

See also https://github.com/avajs/ava/issues/3025.

shellscape commented 1 year ago

Those are both great. I think we're in good shape to close this.

novemberborn commented 1 year ago

I think a filter function could be a useful addition, so I'll leave this open for now.

kirklimbu commented 3 months ago

I came across this error while building my angular 18 app. I solved it by disabling SSR. "prerender": false,"ssr": false inside project.json file