feat: add worker count env variable to processes #3821

ariba-05 · 2022-10-29T12:37:56Z

nicojs

This is great stuff! Got some remarks.

nicojs · 2022-10-29T12:40:45Z

packages/core/src/child-proxy/id-generator.ts

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


Suggested change

private childId: number;

constructor() {

this.childId = 0;

}

public next(): number {

return this.childId++;

}

private id = 0;

public next(): number {

return this.id++;

}

nicojs · 2022-10-29T12:41:19Z

packages/core/src/process/2-mutant-instrumenter-executor.ts

@@ -58,6 +60,7 @@ export class MutantInstrumenterExecutor {

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


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

nicojs · 2022-10-29T12:45:15Z

packages/core/test/unit/checker/checker-child-process-proxy.spec.ts


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


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);

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

Yes, that is good 👌

nicojs · 2022-10-29T12:47:50Z

packages/core/test/unit/child-proxy/child-process-proxy.spec.ts

+      expect(logMock.debug).calledWith(
+        'Started %s in child process %s%s & env var STRYKER_MUTATOR_WORKER as %s',
+        'HelloClass',
+        childProcessMock.pid,
+        ' (using args --cpu-prof --inspect)',
+        nextWorkerId
+      );


Suggested change

expect(logMock.debug).calledWith(

'Started %s in child process %s%s & env var STRYKER_MUTATOR_WORKER as %s',

'HelloClass',

childProcessMock.pid,

' (using args --cpu-prof --inspect)',

nextWorkerId

);

expect(logMock.debug).calledWith(

'Started %s in worker process %s with pid %s %s',

'HelloClass',

nextWorkerId,

childProcessMock.pid,

' (using args --cpu-prof --inspect)'

);

I think this is cleaner :)

…ker-js into 3231-mutant-worker

nicojs · 2022-10-29T14:45:55Z

packages/core/src/child-proxy/child-process-proxy.ts

+    this.worker = childProcess.fork(fileURLToPath(new URL('./child-process-proxy-worker.js', import.meta.url)), {
+      silent: true,
+      execArgv,
+      env: { STRYKER_MUTATOR_WORKER: workerId },


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.

ariba-05 · 2022-10-30T14:20:48Z

I can't seem to find the root cause of time out error in the CI e2e tests (https://github.com/stryker-mutator/stryker-js/actions/runs/3351682875/jobs/5553525949)9). The e2e test errors I get in my local setup are different. I've pushed everything else. @nicojs , could you please maybe trigger the workflow again to see if the errors are the same now? :/

nicojs · 2022-10-30T16:21:15Z

packages/core/src/test-runner/index.ts

@@ -20,15 +22,17 @@ createTestRunnerFactory.inject = tokens(
  coreTokens.sandbox,
  coreTokens.loggingContext,
  commonTokens.getLogger,
-  coreTokens.pluginModulePaths
+  coreTokens.pluginModulePaths,
+  'worker-id-generator'


Suggested change

'worker-id-generator'

coreTokens.workerIdGenerator

nicojs · 2022-10-30T16:23:59Z

packages/core/test/unit/child-proxy/id-generator.spec.ts

+  beforeEach(() => {
+    idGenerator = new IdGenerator();
+  });
+  it('should increment workerId on calling `next`', async () => {


Since we control the SUT (System Under Test, namely the IdGenerator class), we can test that the first call to next provides the value 0.

nicojs

Wow, this looks great! You did a great job. Just 2 nit remarks. I'll merge it later tonight if you don't have time to fix them 👍.

ariba-05 · 2022-10-30T16:37:14Z

Made the changes, @nicojs. Thanks for the review and helping me through my first open source contribution 😄

nicojs

Looks good to me!

nicojs · 2022-10-30T21:46:53Z

I've released this feature in 6.3 🎉
https://github.com/stryker-mutator/stryker-js/releases/tag/v6.3.0

This feature still needs documentation. I've opened #3828 for it. Feel free to pick it up if you want 😉

nicojs · 2022-10-30T22:02:08Z

Thanks for the review and helping me through my first open source contribution 😄

The first of many, I hope 🤝. My pleasure; you are a fast learner. Keep it up! 🤗

ariba-05 added 4 commits October 29, 2022 09:49

initial changes

e56b6aa

Merge branch 'master' into 3231-mutant-worker

b5baf75

errors resolved

a30b41f

Merge branch 'stryker-mutator:master' into 3231-mutant-worker

d0901c4

nicojs requested changes Oct 29, 2022

View reviewed changes

ariba-05 added 2 commits October 29, 2022 15:53

review comments addressed

056fec8

Merge branch '3231-mutant-worker' of https://github.com/ariba-05/stry…

e32aec5

…ker-js into 3231-mutant-worker

nicojs reviewed Oct 29, 2022

View reviewed changes

ariba-05 added 2 commits October 30, 2022 12:03

1 unit test for id_gen & other review changes

d85804a

nit: improvement

47525b9

ariba-05 changed the title ~~Mutant Worker Id~~ feat: add worker count env variable to processes Oct 30, 2022

nicojs reviewed Oct 30, 2022

View reviewed changes

nicojs approved these changes Oct 30, 2022

View reviewed changes

added missed coreToken and added one unit test

9d90ae0

Merge branch 'stryker-mutator:master' into 3231-mutant-worker

7f55a39

nicojs approved these changes Oct 30, 2022

View reviewed changes

nicojs merged commit efb6fd6 into stryker-mutator:master Oct 30, 2022

nicojs mentioned this pull request Oct 30, 2022

Document the way StrykerJS uses parallel workers #3828

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add worker count env variable to processes #3821

feat: add worker count env variable to processes #3821

ariba-05 commented Oct 29, 2022

nicojs left a comment

nicojs Oct 29, 2022

nicojs Oct 29, 2022

nicojs Oct 29, 2022

ariba-05 Oct 30, 2022

nicojs Oct 30, 2022

nicojs Oct 29, 2022

nicojs Oct 29, 2022

ariba-05 commented Oct 30, 2022 •

edited

nicojs Oct 30, 2022

nicojs Oct 30, 2022

nicojs left a comment

ariba-05 commented Oct 30, 2022

nicojs left a comment

nicojs commented Oct 30, 2022

nicojs commented Oct 30, 2022

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

feat: add worker count env variable to processes #3821

feat: add worker count env variable to processes #3821

Conversation

ariba-05 commented Oct 29, 2022

nicojs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariba-05 commented Oct 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicojs left a comment

Choose a reason for hiding this comment

ariba-05 commented Oct 30, 2022

nicojs left a comment

Choose a reason for hiding this comment

nicojs commented Oct 30, 2022

nicojs commented Oct 30, 2022

ariba-05 commented Oct 30, 2022 •

edited