jest command line arguments are not modified safely

[dustinbyrne]

Jest command line arguments are not being transformed safely, and I've yet to come up with a strategy which doesn't involve mirroring Jests command line logic. Here are some examples where it breaks down.

$ npx jest authn

Would result in the following transformed command:

$ npx jest --runInBand --setupFilesAfterEnv /module/path.js authn

This is incorrect because authn will be read in as a setup file instead of a test path pattern. To retain the same behavior, the command would be one of the following:

$ npx jest --runInBand --setupFilesAfterEnv /module/path.js -- authn
$ npx jest --runInBand --setupFilesAfterEnv /module/path.js --testPathPattern authn

This is complicated by the fact that the user may add other options to the command line:

$ npx jest --detectOpenHandles authn

Would need to resolve to one of the following:

$ npx jest --runInBand --setupFilesAfterEnv /module/path.js --detectOpenHandles -- authn
$ npx jest --runInBand --setupFilesAfterEnv /module/path.js --detectOpenHandles --testPathPattern authn

This is again complicated by the fact that we don't know if authn is a test path pattern or an argument of detectOpenHandles. Any tokens remaining after parsing options are treated as test path patterns. Ideally, we would not want to re-implement this logic as it's likely to change (and probably already has across versions).

[kgilpin]

Can we provide users with an alternative strategy that is bulletproof and doesn’t rely on rewriting the command line? That would be good enough for now. It will get people unblocked at least.

…

On Wed, Jan 18, 2023 at 5:15 PM Dustin Byrne ***@***.***> wrote: Jest command line arguments are not being transformed safely, and I've yet to come up with a strategy which doesn't involve mirroring Jests command line logic. Here are some examples where it breaks down. $ npx jest authn Would result in the following transformed command: $ npx jest --runInBand --setupFilesAfterEnv /module/path.js authn This is incorrect because authn will be read in as a setup file instead of a test path pattern. To retain the same behavior, the command would be one of the following: $ npx jest --runInBand --setupFilesAfterEnv /module/path.js -- authn $ npx jest --runInBand --setupFilesAfterEnv /module/path.js --testPathPattern authn This is complicated by the fact that the user may add other options to the command line: $ npx jest --detectOpenHandles authn Would need to resolve to one of the following: $ npx jest --runInBand --setupFilesAfterEnv /module/path.js --detectOpenHandles -- authn $ npx jest --runInBand --setupFilesAfterEnv /module/path.js --detectOpenHandles --testPathPattern authn This is again complicated by the fact that we don't know if authn is a test path pattern or an argument of detectOpenHandles. Any tokens remaining after parsing options are treated as test path patterns. Ideally, we would not want to re-implement this logic as it's likely to change (and probably already has across versions). — Reply to this email directly, view it on GitHub <#179>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVC63734EB7CZL4AOOULTWTBTO7ANCNFSM6AAAAAAT7TZK6Y> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[dustinbyrne]

I agree, parsing and transforming the command line seems fragile in practice - not just in jest support, but overall (e.g., running with yarn is not supported). A module might be run many ways and I fear we'll always be playing catch up trying to capture every case.

[brikelly]

@lachrist can you comment on how much of this issue (if any) has been resolved now that we have more solid Jest support?

[lachrist]

@brikelly This should be fixed by: de5e92f. The --setupFilesAfterEnv is now in tail position.

[lachrist]

@kgilpin @dustinbyrne I know that parsing and hooking commands is a brittle but what is the alternative?

We don't want to use the API because:

1. It is a burden on the user because nobody use them directly.
2. We are spawning these "hooked" processes so directly invoking the API in the main process does not fit the current architecture of the agent.

As for the CLI, most of these tools do not support configuration by environment variables so only program arguments remains. We could provide a temporary file with the hooked configuration but why would you ever want to do that over adding argv?

@brikelly IMO, unless someone does champion an alternative, we can close this issue.

[lachrist]

@kgilpin @dustinbyrne Are you guys suggesting that we still use the CLI but instead of allowing all these possible invocations: npx mocha node node_modules/mocha/bin/mocha mocha we only support one?

[kgilpin]

I don’t know. Let’s get the dogfooding on our own repos working and see what kind of questions or problems we get through community slack.

…

On Mon, Jan 30, 2023 at 8:56 AM Laurent Christophe ***@***.***> wrote: @kgilpin <https://github.com/kgilpin> @dustinbyrne <https://github.com/dustinbyrne> Are you guys suggesting that we still use the CLI but instead of allowing all these possible invocations: npx mocha node node_modules/mocha/bin/mocha mocha we only support one? — Reply to this email directly, view it on GitHub <#179 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAVC6YMOB4G7GX4MGMB5JLWU7CCDANCNFSM6AAAAAAT7TZK6Y> . You are receiving this because you were mentioned.Message ID: ***@***.***>