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: add worker count env variable to processes #3821

Merged
merged 10 commits into from
Oct 30, 2022
7 changes: 5 additions & 2 deletions packages/core/src/checker/checker-child-process-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Disposable } from 'typed-inject';
import { ChildProcessProxy } from '../child-proxy/child-process-proxy.js';
import { LoggingClientContext } from '../logging/index.js';
import { Resource } from '../concurrent/pool.js';
import { IdGenerator } from '../child-proxy/id-generator.js';

import { CheckerWorker } from './checker-worker.js';
import { CheckerResource } from './checker-resource.js';
Expand All @@ -17,7 +18,8 @@ export class CheckerChildProcessProxy implements CheckerResource, Disposable, Re
options: StrykerOptions,
fileDescriptions: FileDescriptions,
pluginModulePaths: readonly string[],
loggingContext: LoggingClientContext
loggingContext: LoggingClientContext,
idGenerator: IdGenerator
) {
this.childProcess = ChildProcessProxy.create(
new URL('./checker-worker.js', import.meta.url).toString(),
Expand All @@ -27,7 +29,8 @@ export class CheckerChildProcessProxy implements CheckerResource, Disposable, Re
pluginModulePaths,
process.cwd(),
CheckerWorker,
options.checkerNodeArgs
options.checkerNodeArgs,
idGenerator
);
}

Expand Down
10 changes: 7 additions & 3 deletions packages/core/src/checker/checker-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { FileDescriptions, StrykerOptions } from '@stryker-mutator/api/core';
import { LoggerFactoryMethod } from '@stryker-mutator/api/logging';
import { commonTokens, tokens } from '@stryker-mutator/api/plugin';

import { IdGenerator } from '../child-proxy/id-generator.js';

import { coreTokens } from '../di/index.js';
import { LoggingClientContext } from '../logging/logging-client-context.js';

Expand All @@ -14,20 +16,22 @@ createCheckerFactory.inject = tokens(
commonTokens.fileDescriptions,
coreTokens.loggingContext,
coreTokens.pluginModulePaths,
commonTokens.getLogger
commonTokens.getLogger,
'worker-id-generator'
);
export function createCheckerFactory(
options: StrykerOptions,
fileDescriptions: FileDescriptions,
loggingContext: LoggingClientContext,
pluginModulePaths: readonly string[],
getLogger: LoggerFactoryMethod
getLogger: LoggerFactoryMethod,
idGenerator: IdGenerator
): () => CheckerFacade {
return () =>
new CheckerFacade(
() =>
new CheckerRetryDecorator(
() => new CheckerChildProcessProxy(options, fileDescriptions, pluginModulePaths, loggingContext),
() => new CheckerChildProcessProxy(options, fileDescriptions, pluginModulePaths, loggingContext, idGenerator),
getLogger(CheckerRetryDecorator.name)
)
);
Expand Down
25 changes: 20 additions & 5 deletions packages/core/src/child-proxy/child-process-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { ChildProcessCrashedError } from './child-process-crashed-error.js';
import { InitMessage, ParentMessage, ParentMessageKind, WorkerMessage, WorkerMessageKind } from './message-protocol.js';
import { OutOfMemoryError } from './out-of-memory-error.js';
import { ChildProcessContext } from './child-process-proxy-worker.js';
import { IdGenerator } from './id-generator.js';

type Func<TS extends any[], R> = (...args: TS) => R;

Expand Down Expand Up @@ -51,11 +52,23 @@ export class ChildProcessProxy<T> implements Disposable {
fileDescriptions: FileDescriptions,
pluginModulePaths: readonly string[],
workingDirectory: string,
execArgv: string[]
execArgv: string[],
idGenerator: IdGenerator
) {
this.worker = childProcess.fork(fileURLToPath(new URL('./child-process-proxy-worker.js', import.meta.url)), { silent: true, execArgv });
const workerId = idGenerator.next().toString();
this.worker = childProcess.fork(fileURLToPath(new URL('./child-process-proxy-worker.js', import.meta.url)), {
silent: true,
execArgv,
env: { STRYKER_MUTATOR_WORKER: workerId },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
env: { STRYKER_MUTATOR_WORKER: workerId },
env: { STRYKER_MUTATOR_WORKER: workerId, ...process.env },

We shouldn't forget to also pass along the process.env of the parent process.

});
this.initTask = new Task();
this.log.debug('Started %s in child process %s%s', namedExport, this.worker.pid, execArgv.length ? ` (using args ${execArgv.join(' ')})` : '');
this.log.debug(
'Started %s in child process %s%s & env var STRYKER_MUTATOR_WORKER as %s',
namedExport,
this.worker.pid,
execArgv.length ? ` (using args ${execArgv.join(' ')})` : '',
workerId
);
// Listen to `close`, not `exit`, see https://github.com/stryker-mutator/stryker-js/issues/1634
this.worker.on('close', this.handleUnexpectedExit);
this.worker.on('error', this.handleError);
Expand Down Expand Up @@ -87,7 +100,8 @@ export class ChildProcessProxy<T> implements Disposable {
pluginModulePaths: readonly string[],
workingDirectory: string,
injectableClass: InjectableClass<ChildProcessContext, R, Tokens>,
execArgv: string[]
execArgv: string[],
idGenerator: IdGenerator
): ChildProcessProxy<R> {
return new ChildProcessProxy(
modulePath,
Expand All @@ -97,7 +111,8 @@ export class ChildProcessProxy<T> implements Disposable {
fileDescriptions,
pluginModulePaths,
workingDirectory,
execArgv
execArgv,
idGenerator
);
}

Expand Down
9 changes: 9 additions & 0 deletions packages/core/src/child-proxy/id-generator.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
export class IdGenerator {
private childId: number;
constructor() {
this.childId = 0;
}
public next(): number {
return this.childId++;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private childId: number;
constructor() {
this.childId = 0;
}
public next(): number {
return this.childId++;
}
private id = 0;
public next(): number {
return this.id++;
}

}
3 changes: 3 additions & 0 deletions packages/core/src/process/2-mutant-instrumenter-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { TemporaryDirectory } from '../utils/temporary-directory.js';
import { UnexpectedExitHandler } from '../unexpected-exit-handler.js';
import { FileSystem, Project } from '../fs/index.js';

import { IdGenerator } from '../child-proxy/id-generator.js';

import { DryRunContext } from './3-dry-run-executor.js';

export interface MutantInstrumenterContext extends PluginContext {
Expand Down Expand Up @@ -58,6 +60,7 @@ export class MutantInstrumenterExecutor {

const checkerPoolProvider = concurrencyTokenProviderProvider
.provideValue(coreTokens.checkerConcurrencyTokens, concurrencyTokenProvider.checkerToken$)
.provideClass('worker-id-generator', IdGenerator)
Copy link
Member

Choose a reason for hiding this comment

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

We should add the 'worker-id-generator' to the coreTokens, as we've done with the others.

.provideFactory(coreTokens.checkerFactory, createCheckerFactory)
.provideFactory(coreTokens.checkerPool, createCheckerPool);
const checkerPool = checkerPoolProvider.resolve(coreTokens.checkerPool);
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/process/3-dry-run-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import { CheckerFacade } from '../checker/index.js';
import { StrictReporter } from '../reporters/index.js';
import { objectUtils } from '../utils/object-utils.js';

import { IdGenerator } from '../child-proxy/id-generator.js';

import { MutationTestContext } from './4-mutation-test-executor.js';
import { MutantInstrumenterContext } from './2-mutant-instrumenter-executor.js';

Expand Down Expand Up @@ -69,8 +71,9 @@ export class DryRunExecutor {

public async execute(): Promise<Injector<MutationTestContext>> {
const testRunnerInjector = this.injector
.provideValue(coreTokens.testRunnerConcurrencyTokens, this.concurrencyTokenProvider.testRunnerToken$)
.provideClass('worker-id-generator', IdGenerator)
.provideFactory(coreTokens.testRunnerFactory, createTestRunnerFactory)
.provideValue(coreTokens.testRunnerConcurrencyTokens, this.concurrencyTokenProvider.testRunnerToken$)
.provideFactory(coreTokens.testRunnerPool, createTestRunnerPool);
const testRunnerPool = testRunnerInjector.resolve(coreTokens.testRunnerPool);
const { result, timing } = await lastValueFrom(testRunnerPool.schedule(of(0), (testRunner) => this.executeDryRun(testRunner)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import { ChildProcessCrashedError } from '../child-proxy/child-process-crashed-e
import { ChildProcessProxy } from '../child-proxy/child-process-proxy.js';
import { LoggingClientContext } from '../logging/index.js';

import { IdGenerator } from '../child-proxy/id-generator.js';

import { ChildProcessTestRunnerWorker } from './child-process-test-runner-worker.js';

const MAX_WAIT_FOR_DISPOSE = 2000;
Expand All @@ -25,7 +27,8 @@ export class ChildProcessTestRunnerProxy implements TestRunner {
sandboxWorkingDirectory: string,
loggingContext: LoggingClientContext,
pluginModulePaths: readonly string[],
private readonly log: Logger
private readonly log: Logger,
idGenerator: IdGenerator
) {
this.worker = ChildProcessProxy.create(
new URL('./child-process-test-runner-worker.js', import.meta.url).toString(),
Expand All @@ -35,7 +38,8 @@ export class ChildProcessTestRunnerProxy implements TestRunner {
pluginModulePaths,
sandboxWorkingDirectory,
ChildProcessTestRunnerWorker,
options.testRunnerNodeArgs
options.testRunnerNodeArgs,
idGenerator
);
}

Expand Down
11 changes: 8 additions & 3 deletions packages/core/src/test-runner/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import { LoggingClientContext } from '../logging/index.js';
import { coreTokens } from '../di/index.js';
import { Sandbox } from '../sandbox/sandbox.js';

import { IdGenerator } from '../child-proxy/id-generator.js';

import { RetryRejectedDecorator } from './retry-rejected-decorator.js';
import { TimeoutDecorator } from './timeout-decorator.js';
import { ChildProcessTestRunnerProxy } from './child-process-test-runner-proxy.js';
Expand All @@ -20,15 +22,17 @@ createTestRunnerFactory.inject = tokens(
coreTokens.sandbox,
coreTokens.loggingContext,
commonTokens.getLogger,
coreTokens.pluginModulePaths
coreTokens.pluginModulePaths,
'worker-id-generator'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'worker-id-generator'
coreTokens.workerIdGenerator

);
export function createTestRunnerFactory(
options: StrykerOptions,
fileDescriptions: FileDescriptions,
sandbox: Pick<Sandbox, 'workingDirectory'>,
loggingContext: LoggingClientContext,
getLogger: LoggerFactoryMethod,
pluginModulePaths: readonly string[]
pluginModulePaths: readonly string[],
idGenerator: IdGenerator
): () => TestRunner {
if (CommandTestRunner.is(options.testRunner)) {
return () => new RetryRejectedDecorator(() => new TimeoutDecorator(() => new CommandTestRunner(sandbox.workingDirectory, options)));
Expand All @@ -48,7 +52,8 @@ export function createTestRunnerFactory(
sandbox.workingDirectory,
loggingContext,
pluginModulePaths,
getLogger(ChildProcessTestRunnerProxy.name)
getLogger(ChildProcessTestRunnerProxy.name),
idGenerator
)
),
options
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import { CheckerFacade, createCheckerFactory } from '../../../src/checker/index.
import { coreTokens } from '../../../src/di/index.js';
import { LoggingClientContext } from '../../../src/logging/index.js';

import { IdGenerator } from '../../../src/child-proxy/id-generator.js';

import { TwoTimesTheCharm } from './additional-checkers.js';

describe(`${createCheckerFactory.name} integration`, () => {
Expand All @@ -34,6 +36,7 @@ describe(`${createCheckerFactory.name} integration`, () => {
createSut = testInjector.injector
.provideValue(coreTokens.loggingContext, loggingContext)
.provideValue(coreTokens.pluginModulePaths, pluginModulePaths)
.provideClass('worker-id-generator', IdGenerator)
.injectFunction(createCheckerFactory);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { currentLogMock } from '../../helpers/log-mock.js';
import { Mock } from '../../helpers/producers.js';
import { sleep } from '../../helpers/test-utils.js';

import { IdGenerator } from '../../../src/child-proxy/id-generator.js';

import { Echo } from './echo.js';

describe(ChildProcessProxy.name, () => {
Expand All @@ -25,6 +27,7 @@ describe(ChildProcessProxy.name, () => {
let fileDescriptions: FileDescriptions;
const testRunnerName = 'echoRunner';
const workingDir = '..';
const idGenerator: IdGenerator = new IdGenerator();

beforeEach(async () => {
fileDescriptions = {
Expand All @@ -35,6 +38,7 @@ describe(ChildProcessProxy.name, () => {
const port = await loggingServer.listen();
testInjector.options.testRunner = testRunnerName;
log = currentLogMock();
idGenerator.next();
sut = ChildProcessProxy.create(
new URL('./echo.js', import.meta.url).toString(),
{ port, level: LogLevel.Debug },
Expand All @@ -46,7 +50,8 @@ describe(ChildProcessProxy.name, () => {
[
'--no-warnings', // test if node args are forwarded with this setting, see https://nodejs.org/api/cli.html#cli_no_warnings
'--max-old-space-size=32', // reduce the amount of time we have to wait on the OOM test
]
],
idGenerator
);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import { sleep } from '../../helpers/test-utils.js';
import { coreTokens } from '../../../src/di/index.js';
import { TestRunnerResource } from '../../../src/concurrent/index.js';

import { IdGenerator } from '../../../src/child-proxy/id-generator.js';

import { additionalTestRunnersFileUrl, CounterTestRunner } from './additional-test-runners.js';

describe(`${createTestRunnerFactory.name} integration`, () => {
Expand Down Expand Up @@ -45,6 +47,7 @@ describe(`${createTestRunnerFactory.name} integration`, () => {
.provideValue(coreTokens.sandbox, { workingDirectory: path.dirname(fileURLToPath(import.meta.url)) })
.provideValue(coreTokens.loggingContext, loggingContext)
.provideValue(coreTokens.pluginModulePaths, pluginModulePaths)
.provideClass('worker-id-generator', IdGenerator)
.injectFunction(createTestRunnerFactory);

rmSync(CounterTestRunner.COUNTER_FILE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,23 @@ import { CheckerChildProcessProxy } from '../../../src/checker/checker-child-pro
import { CheckerWorker } from '../../../src/checker/checker-worker.js';
import { ChildProcessProxy } from '../../../src/child-proxy/child-process-proxy.js';
import { LoggingClientContext } from '../../../src/logging/index.js';
import { IdGenerator } from '../../../src/child-proxy/id-generator.js';

describe(CheckerChildProcessProxy.name, () => {
let childProcessProxyCreateStub: sinon.SinonStubbedMember<typeof ChildProcessProxy.create>;
let loggingContext: LoggingClientContext;
let fileDescriptions: FileDescriptions;
let idGenerator: IdGenerator;

beforeEach(() => {
childProcessProxyCreateStub = sinon.stub(ChildProcessProxy, 'create');
loggingContext = { port: 4200, level: LogLevel.Fatal };
fileDescriptions = { 'foo.js': { mutate: true } };
idGenerator = new IdGenerator();
Copy link
Member

Choose a reason for hiding this comment

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

This is a unit test, so it's better not to depend on the implementation of IdGenerator. That way we can change the implementation details later without breaking the tests.

Something like this:

let idGeneratorMock: sinon.SinonStubbedInstance<IdGenerator>;

// in before each:

idGeneratorMock = sinon.createStubInstance(IdGenerator);

// In your test where you verify that the worker id is passed correctly:
idGeneratorMock.next.returns(42);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made similar changes in child-process-proxy.spec.ts. Could you pls review if I've done it correctly as you suggested?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is good 👌

});

function createSut(): CheckerChildProcessProxy {
return new CheckerChildProcessProxy(testInjector.options, fileDescriptions, ['plugin', 'paths'], loggingContext);
return new CheckerChildProcessProxy(testInjector.options, fileDescriptions, ['plugin', 'paths'], loggingContext, idGenerator);
}

describe('constructor', () => {
Expand All @@ -36,7 +39,8 @@ describe(CheckerChildProcessProxy.name, () => {
['plugin', 'paths'],
process.cwd(),
CheckerWorker,
[]
[],
idGenerator
);
});
it('should provide arguments', () => {
Expand All @@ -51,7 +55,8 @@ describe(CheckerChildProcessProxy.name, () => {
sinon.match.any,
sinon.match.any,
sinon.match.any,
['foo', 'bar']
['foo', 'bar'],
idGenerator
);
});
});
Expand Down
Loading