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

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

Merged
merged 5 commits into from Nov 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/api/schema/stryker-core.json
Expand Up @@ -400,6 +400,14 @@
"type": "string",
"default": "command"
},
"testRunnerNodeArgs": {
"description": "Configure arguments to be passed as exec arguments to the test runner child process. For example, running Stryker with `--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs --inspect-brk` will allow you to debug the test runner child process. See `execArgv` of [`child_process.fork`](https://nodejs.org/api/child_process.html#child_process_child_process_fork_modulepath_args_options)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nicojs nicojs Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@nicojs nicojs Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Lakitna Lakitna Nov 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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' ]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for helping this feature!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem :)

"type": "array",
"default": [],
"items": {
"type": "string"
}
},
"thresholds": {
"description": "Specify the thresholds for mutation score.",
"$ref": "#/definitions/mutationScoreThresholds",
Expand Down
15 changes: 12 additions & 3 deletions packages/core/README.md
Expand Up @@ -395,9 +395,9 @@ _Note: Use of "testFramework" is no longer needed. You can remove it from your c
<a name="testRunner"></a>
### `testRunner` [`string`]

Default: `'command'`
Command line: `--testRunner karma`
Config file: `testRunner: 'karma'`
Default: `'command'`
Command line: `--testRunner karma`
Config file: `testRunner: 'karma'`

With `testRunner` you specify the test runner that Stryker uses to run your tests. The default value is `command`. The command runner runs a configurable bash/cmd command and bases the result on the exit code of that program (0 for success, otherwise failed). You can configure this command via the config file using the `commandRunner: { command: 'npm run mocha' }`. It uses `npm test` as the command by default.

Expand All @@ -407,6 +407,15 @@ If possible, you should try to use one of the test runner plugins that hook into
For example: install and use the `stryker-karma-runner` to use `karma` as a test runner.
See the [list of plugins](https://stryker-mutator.io/plugins.html) for an up-to-date list of supported test runners and plugins.

<a name="testRunnerNodeArgs"></a>
### `testRunnerNodeArgs` [`string[]`]

Default: `[]`
Command line: `--testRunnerNodeArgs "--inspect-brk --cpu-prof"`
Config file: `testRunnerNodeArgs: ['--inspect-brk', '--cpu-prof']`

Configure arguments to be passed as exec arguments to the test runner child process. For example, running Stryker with `--timeoutMS 9999999 --concurrency 1 --testRunnerNodeArgs "--inspect-brk"` will allow you to debug the test runner child process. See `execArgv` of [`child_process.fork`](https://nodejs.org/api/child_process.html#child_process_child_process_fork_modulepath_args_options).

<a name="thresholds"></a>
### `thresholds` [`object`]

Expand Down
10 changes: 9 additions & 1 deletion packages/core/src/checker/checker-facade.ts
Expand Up @@ -22,7 +22,15 @@ export class CheckerFacade implements Checker, Disposable, Worker {

constructor(options: StrykerOptions, loggingContext: LoggingClientContext) {
if (options.checkers.length) {
this.childProcess = ChildProcessProxy.create(require.resolve('./checker-worker'), loggingContext, options, {}, process.cwd(), CheckerWorker);
this.childProcess = ChildProcessProxy.create(
require.resolve('./checker-worker'),
loggingContext,
options,
{},
process.cwd(),
CheckerWorker,
[]
);
}
}

Expand Down
16 changes: 11 additions & 5 deletions packages/core/src/child-proxy/child-process-proxy.ts
Expand Up @@ -46,11 +46,12 @@ export default class ChildProcessProxy<T> implements Disposable {
loggingContext: LoggingClientContext,
options: StrykerOptions,
additionalInjectableValues: unknown,
workingDirectory: string
workingDirectory: string,
execArgv: string[]
) {
this.worker = fork(require.resolve('./child-process-proxy-worker'), [autoStart], { silent: true, execArgv: [] });
this.worker = fork(require.resolve('./child-process-proxy-worker'), [autoStart], { silent: true, execArgv });
this.initTask = new Task();
this.log.debug('Starting %s in child process %s', requirePath, this.worker.pid);
this.log.debug('Started %s in child process %s%s', requireName, this.worker.pid, execArgv.length ? ` (using args ${execArgv.join(' ')})` : '');
this.send({
additionalInjectableValues,
kind: WorkerMessageKind.Init,
Expand All @@ -77,9 +78,10 @@ export default class ChildProcessProxy<T> implements Disposable {
options: StrykerOptions,
additionalInjectableValues: TAdditionalContext,
workingDirectory: string,
injectableClass: InjectableClass<TAdditionalContext & PluginContext, R, Tokens>
injectableClass: InjectableClass<TAdditionalContext & PluginContext, R, Tokens>,
execArgv: string[]
): ChildProcessProxy<R> {
return new ChildProcessProxy(requirePath, injectableClass.name, loggingContext, options, additionalInjectableValues, workingDirectory);
return new ChildProcessProxy(requirePath, injectableClass.name, loggingContext, options, additionalInjectableValues, workingDirectory, execArgv);
}

private send(message: WorkerMessage) {
Expand Down Expand Up @@ -170,6 +172,10 @@ export default class ChildProcessProxy<T> implements Disposable {
return this.stdoutBuilder.toString();
}

public get stderr() {
return this.stderrBuilder.toString();
}

private reportError(error: Error) {
this.workerTasks.filter((task) => !task.isCompleted).forEach((task) => task.reject(error));
}
Expand Down
7 changes: 7 additions & 0 deletions packages/core/src/config/options-validator.ts
Expand Up @@ -11,6 +11,8 @@ import { coreTokens } from '../di';
import { ConfigError } from '../errors';
import { isWarningEnabled } from '../utils/object-utils';

import CommandTestRunner from '../test-runner/command-test-runner';

import { describeErrors } from './validation-errors';

const ajv = new Ajv({ useDefaults: true, allErrors: true, jsonPointers: false, verbose: true, missingRefs: 'ignore', logger: false });
Expand Down Expand Up @@ -79,6 +81,11 @@ export class OptionsValidator {
options.concurrency = options.maxConcurrentTestRunners;
}
}
if (CommandTestRunner.is(options.testRunner) && options.testRunnerNodeArgs.length) {
this.log.warn(
'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.'
);
}
additionalErrors.forEach((error) => this.log.error(error));
this.throwErrorIfNeeded(additionalErrors);
}
Expand Down
11 changes: 9 additions & 2 deletions packages/core/src/stryker-cli.ts
Expand Up @@ -20,8 +20,10 @@ function deepOption<T extends string, R>(object: { [K in T]?: R }, key: T) {
};
}

function list(val: string) {
return val.split(',');
const list = createSplitter(',');

function createSplitter(sep: string) {
return (val: string) => val.split(sep);
}

function parseBoolean(val: string) {
Expand Down Expand Up @@ -77,6 +79,11 @@ export default class StrykerCli {
`The coverage analysis strategy you want to use. Default value: "${defaultValues.coverageAnalysis}"`
)
.option('--testRunner <name>', 'The name of the test runner you want to use')
.option(
'--testRunnerNodeArgs <listOfNodeArgs>',
'A comma separated list of node args to be passed to test runner child processes.',
createSplitter(' ')
)
.option('--reporters <name>', 'A comma separated list of the names of the reporter(s) you want to use', list)
.option('--plugins <listOfPlugins>', 'A list of plugins you want stryker to load (`require`).', list)
.option(
Expand Down
Expand Up @@ -23,7 +23,8 @@ export default class ChildProcessTestRunnerDecorator implements TestRunner {
options,
{},
sandboxWorkingDirectory,
ChildProcessTestRunnerWorker
ChildProcessTestRunnerWorker,
options.testRunnerNodeArgs
);
}

Expand Down
Expand Up @@ -30,7 +30,9 @@ describe(ChildProcessProxy.name, () => {
const port = await loggingServer.listen();
const options = testInjector.injector.resolve(commonTokens.options);
log = currentLogMock();
sut = ChildProcessProxy.create(require.resolve('./echo'), { port, level: LogLevel.Debug }, options, { name: echoName }, workingDir, Echo);
sut = ChildProcessProxy.create(require.resolve('./echo'), { port, level: LogLevel.Debug }, options, { name: echoName }, workingDir, Echo, [
'--no-warnings', // test if node args are forwarded with this setting, see https://nodejs.org/api/cli.html#cli_no_warnings
]);
});

afterEach(async () => {
Expand Down Expand Up @@ -68,6 +70,11 @@ describe(ChildProcessProxy.name, () => {
expect(actual.name).eq('foobar.txt');
});

it('should use `execArgv` to start the child process', async () => {
await sut.proxy.warning();
expect(sut.stderr).not.includes('Foo warning');
});

it('should be able to receive a promise rejection', async () => {
await expect(sut.proxy.reject('Foobar error')).rejectedWith('Foobar error');
});
Expand Down
4 changes: 4 additions & 0 deletions packages/core/test/integration/child-proxy/echo.ts
Expand Up @@ -35,6 +35,10 @@ export class Echo {
return new File('foobar.txt', 'hello foobar');
}

public warning() {
process.emitWarning('Foo warning');
}

public cwd() {
return process.cwd();
}
Expand Down
20 changes: 19 additions & 1 deletion packages/core/test/unit/child-proxy/child-process-proxy.spec.ts
Expand Up @@ -94,6 +94,17 @@ describe(ChildProcessProxy.name, () => {
expect(childProcessMock.send).calledWith(serialize(expectedMessage));
});

it('should log the exec arguments and require name', () => {
// Act
createSut({
loggingContext: LOGGING_CONTEXT,
execArgv: ['--cpu-prof', '--inspect'],
});

// Assert
expect(logMock.debug).calledWith('Started %s in child process %s%s', 'HelloClass', childProcessMock.pid, ' (using args --cpu-prof --inspect)');
});

it('should listen to worker process', () => {
createSut();
expect(childProcessMock.listeners('message')).lengthOf(1);
Expand All @@ -103,6 +114,11 @@ describe(ChildProcessProxy.name, () => {
createSut();
expect(childProcessMock.listeners('close')).lengthOf(1);
});

it('should set `execArgv`', () => {
createSut({ execArgv: ['--inspect-brk'] });
expect(forkStub).calledWithMatch(sinon.match.string, sinon.match.array, sinon.match({ execArgv: ['--inspect-brk'] }));
});
});

describe('on close', () => {
Expand Down Expand Up @@ -247,6 +263,7 @@ function createSut(
options?: Partial<StrykerOptions>;
workingDir?: string;
name?: string;
execArgv?: string[];
} = {}
): ChildProcessProxy<HelloClass> {
return ChildProcessProxy.create(
Expand All @@ -255,6 +272,7 @@ function createSut(
factory.strykerOptions(overrides.options),
{ name: overrides.name || 'someArg' },
overrides.workingDir || 'workingDir',
HelloClass
HelloClass,
overrides.execArgv ?? []
);
}
28 changes: 21 additions & 7 deletions packages/core/test/unit/config/options-validator.spec.ts
Expand Up @@ -33,7 +33,8 @@ describe(OptionsValidator.name, () => {
});

it('should be invalid with thresholds.high null', () => {
(testInjector.options.thresholds.high as any) = null;
// @ts-expect-error invalid setting
testInjector.options.thresholds.high = null;
actValidationErrors('Config option "thresholds.high" has the wrong type. It should be a number, but was a null.');
});

Expand All @@ -45,7 +46,8 @@ describe(OptionsValidator.name, () => {
});

it('should be invalid with invalid logLevel', () => {
testInjector.options.logLevel = 'thisTestPasses' as any;
// @ts-expect-error invalid setting
testInjector.options.logLevel = 'thisTestPasses';
actValidationErrors(
'Config option "logLevel" should be one of the allowed values ("off", "fatal", "error", "warn", "info", "debug", "trace"), but was "thisTestPasses".'
);
Expand Down Expand Up @@ -97,12 +99,14 @@ describe(OptionsValidator.name, () => {

describe('mutator', () => {
it('should be invalid with non-string mutator', () => {
(testInjector.options.mutator as any) = 1;
// @ts-expect-error invalid setting
testInjector.options.mutator = 1;
actValidationErrors('Config option "mutator" has the wrong type. It should be a object, but was a number.');
});

it('should report a deprecation warning for "mutator.name"', () => {
(testInjector.options.mutator as any) = {
testInjector.options.mutator = {
// @ts-expect-error invalid setting
name: 'javascript',
};
sut.validate(testInjector.options);
Expand All @@ -112,7 +116,8 @@ describe(OptionsValidator.name, () => {
});

it('should report a deprecation warning for mutator as a string', () => {
(testInjector.options.mutator as any) = 'javascript';
// @ts-expect-error invalid setting
testInjector.options.mutator = 'javascript';
sut.validate(testInjector.options);
expect(testInjector.logger.warn).calledWith(
'DEPRECATED. Use of "mutator" as string is no longer needed. You can remove it from your configuration. Stryker now supports mutating of JavaScript and friend files out of the box.'
Expand All @@ -122,7 +127,7 @@ describe(OptionsValidator.name, () => {

describe('testFramework', () => {
it('should report a deprecation warning', () => {
(testInjector.options as any).testFramework = '';
testInjector.options.testFramework = '';
sut.validate(testInjector.options);
expect(testInjector.logger.warn).calledWith(
'DEPRECATED. Use of "testFramework" is no longer needed. You can remove it from your configuration. Your test runner plugin now handles its own test framework integration.'
Expand Down Expand Up @@ -192,9 +197,18 @@ describe(OptionsValidator.name, () => {
actValidationErrors('Config option "maxTestRunnerReuse" has the wrong type. It should be a number, but was a string.');
});

it('should warn when testRunnerNodeArgs are combined with the "command" test runner', () => {
testInjector.options.testRunnerNodeArgs = ['--inspect-brk'];
testInjector.options.testRunner = 'command';
sut.validate(testInjector.options);
expect(testInjector.logger.warn).calledWith(
'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.'
);
});

describe('transpilers', () => {
it('should report a deprecation warning', () => {
(testInjector.options.transpilers as any) = ['stryker-jest'];
testInjector.options.transpilers = ['stryker-jest'];
sut.validate(testInjector.options);
expect(testInjector.logger.warn).calledWith(
'DEPRECATED. Support for "transpilers" is removed. You can now configure your own "buildCommand". For example, npm run build.'
Expand Down
1 change: 1 addition & 0 deletions packages/core/test/unit/stryker-cli.spec.ts
Expand Up @@ -40,6 +40,7 @@ describe(StrykerCli.name, () => {
[['--maxConcurrentTestRunners', '42'], { maxConcurrentTestRunners: 42 }],
[['--tempDirName', 'foo-tmp'], { tempDirName: 'foo-tmp' }],
[['--testRunner', 'foo-running'], { testRunner: 'foo-running' }],
[['--testRunnerNodeArgs', '--inspect=1337 --gc'], { testRunnerNodeArgs: ['--inspect=1337', '--gc'] }],
[['--coverageAnalysis', 'all'], { coverageAnalysis: 'all' }],
[['--concurrency', '5'], { concurrency: 5 }],
[['--cleanTempDir', 'false'], { cleanTempDir: false }],
Expand Down
Expand Up @@ -15,7 +15,6 @@ import ChildProcessTestRunnerDecorator from '../../../src/test-runner/child-proc
import { ChildProcessTestRunnerWorker } from '../../../src/test-runner/child-process-test-runner-worker';

describe(ChildProcessTestRunnerDecorator.name, () => {
let sut: ChildProcessTestRunnerDecorator;
let options: StrykerOptions;
let childProcessProxyMock: {
proxy: sinon.SinonStubbedInstance<Required<TestRunner>>;
Expand All @@ -37,26 +36,34 @@ describe(ChildProcessTestRunnerDecorator.name, () => {
plugins: ['foo-plugin', 'bar-plugin'],
});
loggingContext = { port: 4200, level: LogLevel.Fatal };
sut = new ChildProcessTestRunnerDecorator(options, 'a working directory', loggingContext);
});

function createSut(): ChildProcessTestRunnerDecorator {
return new ChildProcessTestRunnerDecorator(options, 'a working directory', loggingContext);
}

it('should create the child process proxy', () => {
expect(childProcessProxyCreateStub).calledWith(
options.testRunnerNodeArgs = ['--inspect', '--no-warnings'];
createSut();
expect(childProcessProxyCreateStub).calledWithExactly(
require.resolve('../../../src/test-runner/child-process-test-runner-worker.js'),
loggingContext,
options,
{},
'a working directory',
ChildProcessTestRunnerWorker
ChildProcessTestRunnerWorker,
['--inspect', '--no-warnings']
);
});

it('should forward `init` calls', () => {
const sut = createSut();
childProcessProxyMock.proxy.init.resolves(42);
return expect(sut.init()).eventually.eq(42);
});

it('should forward `dryRun` calls', async () => {
const sut = createSut();
const expectedResult = factory.completeDryRunResult({ mutantCoverage: factory.mutantCoverage() });
childProcessProxyMock.proxy.dryRun.resolves(expectedResult);
const runOptions = factory.dryRunOptions({
Expand All @@ -68,6 +75,7 @@ describe(ChildProcessTestRunnerDecorator.name, () => {
});

it('should forward `mutantRun` calls', async () => {
const sut = createSut();
const expectedResult = factory.survivedMutantRunResult();
childProcessProxyMock.proxy.mutantRun.resolves(expectedResult);
const runOptions = factory.mutantRunOptions({
Expand All @@ -80,18 +88,21 @@ describe(ChildProcessTestRunnerDecorator.name, () => {

describe('dispose', () => {
it('should dispose the test runner before disposing the child process itself on `dispose`', async () => {
const sut = createSut();
childProcessProxyMock.proxy.dispose.resolves();
await sut.dispose();
expect(childProcessProxyMock.proxy.dispose).calledBefore(childProcessProxyMock.dispose);
});

it('should not reject when the child process is down', async () => {
const sut = createSut();
childProcessProxyMock.proxy.dispose.rejects(new ChildProcessCrashedError(1, '1'));
await sut.dispose();
expect(childProcessProxyMock.dispose).called;
});

it('should only wait 2 seconds for the test runner to be disposed', async () => {
const sut = createSut();
const testRunnerDisposeTask = new Task();
childProcessProxyMock.proxy.dispose.returns(testRunnerDisposeTask.promise);
const disposePromise = sut.dispose();
Expand Down