feat(debugging): allow passing node args to the test runner

[nicojs]

Add the --testRunnerNodeArgs option. With this option, you can pass arguments to the node process that hosts the test runner.

For example, running stryker with --timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs --inspect-brk will allow you to debug the tests running in the dry run or the mutation test run.

Another use case is using --testRunnerNodeArgs --cpu-prof, this allows you to analyse the performance of the child process..

Fixes #2505

[nicojs]

@Lakitna
@Garethp

Feel free to take a look if you have some time.

[Lakitna]

Don't forget to gracefully exit the child processes as I did here: https://github.com/stryker-mutator/stryker/pull/2536/files#diff-9bd013c2c0d5a7d2bcd8594a6bfb8b4a89b54b3db0d4626b61637f00fd37900d

Without it, things that trigger on process.exit will not trigger. The result of this is that, for example, --cpu-prof does not output anything.

[Lakitna]

It's probably a good idea to support and communicate with quotes around the test runner args:

--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs "--inspect-brk"

This allows us to send multiple args like this:

--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs "--inspect-brk --foo --bar --baz"

It also makes it clearer which args are for the main process and which for the test runner.

[Lakitna]

The latter results in process.argv containing:

[
  '--timeoutMS',
  '9999999',
  '--concurrency',
  '1',
  '--testRunnerNodeArgs',
  '--inspect-brk --foo --bar --baz'
]

So commander should be able to handle it without issues.

[nicojs]

The latter results in process.argv containing:

[
  '--timeoutMS',
  '9999999',
  '--concurrency',
  '1',
  '--testRunnerNodeArgs',
  '--inspect-brk --foo --bar --baz'
]

So commander should be able to handle it without issues.

Well. We currently support splitting on "," as you can see here:

https://github.com/stryker-mutator/stryker/blob/b2e541badd08e34826bc0ac6338cfb40f2e8565d/packages/core/src/stryker-cli.ts#L24

I've added an example with 2 arguments in the readme: --testRunnerNodeArgs --inspect-brk,--cpu-prof. Would this be acceptable?

Don't forget to gracefully exit the child processes as I did here: https://github.com/stryker-mutator/stryker/pull/2536/files#diff-9bd013c2c0d5a7d2bcd8594a6bfb8b4a89b54b3db0d4626b61637f00fd37900d

Without it, things that trigger on process.exit will not trigger. The result of this is that, for example, --cpu-prof does not output anything.

Hmm, interesting. I don't seem to get CPU profile files at all this way. Even with the finally(() => process.exit());. I'm using Linux, this kind of behavior will definitly be influenced by OS specific closing details. Needs further investigating. I'm hesitant to drag it into this PR, since it is not directly related.

[Lakitna]

Splitting on ',' would also work, but splitting on /\s+/ (= any whitespace of any length) would make it feel a lot more like a command to me. If I didn't read the docs I would try --testRunnerNodeArgs "--foo --bar" before trying --testRunnerNodeArgs --foo,--bar. Both options will work for me though :)

this kind of behavior will definitly be influenced by OS specific closing details. Needs further investigating.

If you add this to the PR I'll be able to test it on Windows and MacOS. With you testing on Linux we can cover all bases. This might be out of scope for this PR though, we can also cover these bases separately. Also note that --cpu-prof is experimental at this time: https://nodejs.org/api/cli.html#cli_cpu_prof. There might be stuff that doesn't work on every OS because of it.

[nicojs]

The "problem" with splitting on whitespace characters is that there are exotic cases of command-line arguments that require spaces. Not sure about node args though.

I've chosen , to split, because we're already using that for other options, like --mutate or --files (spaces in file names are a bit more common). I could change that for --testRunnerNodeArgs to also accept a space. That might make sense as you point out.

[Lakitna]

Yeah, you're right. There are some exotic cases that can pop up. This is generally the moment where I wonder if I can fix all of this with NPM :)

How about using something like string-argv or spawn-args to prevent the pain of edge cases?

Edit: I got this result by using the code in the spoiler:

Code

  #!/usr/bin/env node

  const commander = require('commander');
  const {parseArgsStringToArgv} = require('string-argv');

  function parseArgs(value, previous) {
      return parseArgsStringToArgv(value.replace(/,/g, ' '));
  }

  const program = new commander.Command();
  program.option('--testRunnerNodeArgs <args>', 'args', parseArgs, []);

  program.parse(process.argv);
  console.log(program.testRunnerNodeArgs);

--testRunnerNodeArgs "--foo --bar --baz \"hello world\" -h,-e,-y"
[ '--foo', '--bar', '--baz', 'hello world', '-h', '-e', '-y' ]

[nicojs]

I've chosen to split on " " (space) for now. I've updated the docs and code. It's a better user experience than splitting on a comma.

Don't want to add an extra dependency if I don't have to. I think splitting on spaces for node-args is fine (don't see a direct reason to support escaped spaces)

[nicojs]

Thanks for helping this feature!

[Lakitna]

No problem :)

[nicojs]

Question. How could we support the command test runner here? Maybe we shouldn't allow it? At least log a warning would be appropriate I guess.

[Lakitna]

If the command runner doesn't use child processes it's kind of moot to support it. Besides, the command runner supports passing arguments by definition.

I can see testing/debugging situations where you might temporarily switch to the command runner. So I would log it at warn for those reasons.

[nicojs]

I've implemented the warning.

$ npx stryker run --testRunnerNodeArgs "--inspect"
21:00:16 (7810) INFO ConfigReader Using stryker.conf.json
21:00:16 (7810) WARN OptionsValidator Using "testRunnerNodeArgs" together with the "command" test runner is not supported, these arguments will be ignored. You can add your custom arguments by setting the "commandRunner.command" option.
21:00:16 (7810) INFO InputFileResolver Found 2 of 8 file(s) to be mutated.
[...]