Add unique id for each worker and pass it to the child process #5494

ranyitz · 2018-02-08T01:07:01Z

This PR is related to issue #2284

The Problem

When trying to run integration tests in parallel, there could be a race of conditions.
For example when two tests are trying to modify the same record in a database at the same time.

To get full isolation, the user can choose one of two solutions:

Run the test serially. -> slow... 😞
Create a different DB/server for each worker. -> unlocks the full potential of the machine 😉

But in order to use the second, you need unique identifiers for each worker, so you could assign each test to a different server.

For example:

const port = 3000 + process.env.JEST_WORKER_ID

Summary

Assign a unique id for each worker and pass it to the child process. It will be available via process.env.JEST_WORKER_ID.

The first index would be 1.
We store the id as a number and it is being cast to string only when passed to the newly forked process.
A test that verifies the options that passes to each worker needed to be modified with the new workerId option.
A test that checks that the options have passed to the child uses process.env, lucky enough, jest uses itself for testing, so we could use the same JEST_WORKER_ID 😄

Test plan

Test that each individual worker gets its own unique id from the farm.
Verify that the worker assigns the workerId to the env.JEST_WORKER_ID.

Questions

should we update the readme with this new feature?

…ST_WORKER_ID

codecov-io · 2018-02-08T01:16:47Z

Codecov Report

Merging #5494 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5494      +/-   ##
==========================================
+ Coverage   61.66%   61.67%   +<.01%     
==========================================
  Files         213      213              
  Lines        7070     7071       +1     
  Branches        3        4       +1     
==========================================
+ Hits         4360     4361       +1     
  Misses       2709     2709              
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-worker/src/types.js	`100% <ø> (ø)`	⬆️
packages/jest-worker/src/worker.js	`100% <ø> (ø)`	⬆️
packages/jest-worker/src/index.js	`85.89% <100%> (+0.18%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8d1c79...75670ef. Read the comment docs.

mjesun

LGTM, just some minor changes and we're ready to merge!

mjesun · 2018-02-08T01:19:43Z

packages/jest-worker/src/types.js

@@ -47,6 +47,7 @@ export type WorkerOptions = {|
  forkOptions: ForkOptions,
  maxRetries: number,
  workerPath: string,
+  workerId: number,


Alphabetically sorted props 🙂

mjesun · 2018-02-08T01:21:35Z

packages/jest-worker/src/index.js

@@ -67,13 +67,16 @@ export default class {
    }

    // Build the options once for all workers to avoid allocating extra objects.


Good to keep all shared props at a common object, but probably the comment does not apply anymore, since we need to create one object per worker now.

mjesun · 2018-02-08T01:24:17Z

Regarding the README, it'd be good to mention it; I think that not only Jest itself can benefit from this (e.g. Metro also uses jest-worker in its core).

ranyitz · 2018-02-08T01:32:40Z

@mjesun that's awesome!

About the README, do you think that something like a small note regarding the existence of such environment param on each worker would be enough?

what do you say about:

Note: Each worker has a unique id (index that starts with 1) which is available on process.env.JEST_WORKER_ID

Which will be placed right before this part

mjesun · 2018-02-08T01:40:19Z

Yep, that sounds good to me! If you can demonstrate it in a concise example too, it would be awesome 😄

ranyitz · 2018-02-08T01:59:03Z

What do you say about extending the original example of the standard usage?

something like:

import Worker from 'jest-worker';

async function main() {
  const myWorker = new Worker(require.resolve('./worker'), {
    exposedMethods: ['foo', 'bar', 'getWorkerId'],
    numWorkers: 4,
  });

  console.log(await myWorker.foo('Alice')); // "Hello from foo: Alice"
  console.log(await myWorker.bar('Bob')); // "Hello from bar: Bob"
  console.log(await myWorker.getWorkerId()); // "3" -> this message has sent from the 3rd worker

  myWorker.end();
}

main();

File `worker.js`

export function foo(param) {
  return 'Hello from foo: ' + param;
}

export function bar(param) {
  return 'Hello from bar: ' + param;
}

export function getWorkerId() {
  return process.env.JEST_WORKER_ID;
}

maximilianschmitt · 2018-02-08T09:51:14Z

You're awesome @ranyitz, thanks for implementing this! :)

mjesun · 2018-02-08T10:44:48Z

The comment you added LGTM, but if you want to extend the example it'd be awesome!

ranyitz · 2018-02-08T11:43:13Z

@mjesun Example added.
I think this is ready 🎉

mjesun · 2018-02-08T11:47:41Z

Yup! Let's wait for the green light in CI and we'll merge. Thanks for your contribution!

ranyitz · 2018-02-08T11:49:56Z

no problem, thanks for the review!

evantahler · 2018-03-16T21:40:14Z

Hey! Thanks for solving #2284!

alexbyk · 2018-03-22T02:41:21Z

Is there a way to access this workerId from the test function?

mjesun · 2018-03-22T11:01:08Z

I assume that process.env.JEST_WORKER_ID would contain the id in the test method. Notice that when executing a single test, the test is executed on the main process for performance reasons, so there you won't have it.

SimenB · 2018-03-22T11:20:34Z

Or using runInBand or --workers 1. We might want to consider adding it even when run on the main process

ranyitz · 2018-03-22T12:38:50Z

@mjesun @SimenB Yes, as Jest does this in various cases:

https://github.com/facebook/jest/blob/6a77ee37ec2d46ece7e9cfd0f891d11c113cc4c4/packages/jest-cli/src/test_scheduler.js#L88-L96

We should probably assign something to the process.env.JEST_WORKER_ID in runInBand case.

It could be just 0, as child workers start from 1.

alexbyk · 2018-03-22T22:00:50Z

thanks, @mjesun, your answer helped.
...

I have a question related to this. All tests within a single file run one-by-one. Could we say the same about tests in different files but per one worker (process)?

I mean, could we say that there is always only one test running per worker (and that would mean maxWorker == max running parallel tests, because each worker has only 1 running test)

I want to do something like this:

let db: TestDb;
const POOL_ID = process.env.JEST_WORKER_ID || '1';

beforeAll(async () => { db = await TestDb.connect(POOL_ID); });
beforeEach(async () => {
  await db.clearAll();
});
afterAll(async () => { db.manager.connection.close(); });

If there is always only one test per worker in the "running" state, that's fine. If not, that wouldn't work and I would need to implement some sort of mutex per file that locks a connection in beforeAll and releases it afterAll.

Documentation say nothing about it at all.

mjesun · 2018-03-22T22:04:40Z

Yes, that is precisely what happens internally in Jest. Each of the test files is sent to an available worker, only one at a time per worker.

What you have to be aware though, is to fully clean your stuff using the available after hooks. If you leak an opened connection letting your test file finish, a new one will be scheduled to its worker, which could run into a collision.

mjesun · 2018-03-22T22:34:17Z

Regarding the --runInBand, I think it sounds reasonable. PRs welcome ☺️

alexbyk · 2018-03-22T22:40:12Z

@mjesun thanks for the explanation.

ranyitz · 2018-03-22T23:32:05Z

@mjesun I've created a PR here #5860 👌

mjesun · 2018-03-23T00:13:26Z

@alexbyk @ranyitz just created a PR that I merged that will also provide JEST_WORKER_ID when running in band. That should avoid you the ||; I will release that probably as an alpha tomorrow 🙂

jaredtmartin · 2018-04-18T14:56:01Z

What version of jest do I need to have to use this feature? I just updated to v23.0.0-alpha.6r and process.env.JEST_WORKER_ID gives me undefined. Am I missing something?

SimenB · 2018-04-18T14:56:40Z

It should be there.

@mjesun

mjesun · 2018-04-18T15:15:04Z

@SimenB is right. Without any other context it's hard to say what is going on. Can you share the test code or a minimal repro?

jaredtmartin · 2018-04-18T15:18:01Z

Yes. He was right. My global jest was on version 23 but my local jest was on version 22. I’m appending it to my database names to avoid contamination. I wanted to do a bit more testing before I responded, but I upgraded and it seems to be working beautifully. Thanks a lot!

…

On Apr 18, 2018, at 9:15 AM, Miguel Jiménez Esún ***@***.***> wrote: @SimenB <https://github.com/SimenB> is right. Without any other context it's hard to say what is going on. Can you share the test code or a minimal repro? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#5494 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAYGCuZOPWjBOMccRcNky2iqb7nDNP8uks5tp1h_gaJpZM4R9pOo>.

evantahler · 2018-04-18T15:20:51Z

I would always suggest process.env.JEST_WORKER_ID || 0 as you can see in this project. Depending on how many test files you have, jest might not actually run your tests in parallel.

mjesun · 2018-04-18T15:33:38Z

@evantahler That was fixed some time ago by @ranyitz :)

evantahler · 2018-04-18T15:41:32Z

Oh! Wonderful.

SimenB · 2018-04-18T15:49:06Z

In #5860 fwiw

github-actions · 2021-05-12T17:23:41Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

Add unique id for each worker and pass it to the child process.env.JE…

a00ec03

…ST_WORKER_ID

facebook-github-bot added the CLA Signed ✔️ label Feb 8, 2018

update changelog with the addition of unique id for each worker

931f73a

ranyitz mentioned this pull request Feb 8, 2018

Workers in parallel worker pool should have JEST_WORKER_ID assigned in ENV #2284

Closed

mjesun suggested changes Feb 8, 2018

View reviewed changes

ranyitz added 2 commits February 8, 2018 03:27

Alphabetically sorted props

d8430be

remove non relevant comment

27d512e

document the addition of env.JEST_WORKER_ID on each worker child process

ef15a6b

mjesun approved these changes Feb 8, 2018

View reviewed changes

add example for documentation regarding JEST_WORKER_ID

75670ef

mjesun merged commit efec054 into jestjs:master Feb 8, 2018

ranyitz deleted the jest-worker-id branch February 8, 2018 12:01

renovate bot mentioned this pull request Feb 14, 2018

fix(deps): update dependency jest to ^22.0.0 manekinekko/devoxx-assistant#3

Open

ranyitz mentioned this pull request Mar 22, 2018

assign process.env.JEST_WORKER_ID=1 when in runInBand mode #5860

Merged

marcos8896 mentioned this pull request Sep 7, 2018

Support parallel tests marcos8896/nutrition-care-node-api#33

Closed

META-DREAMER mentioned this pull request Apr 24, 2019

Support for single reinstall in Jest multi worker mode wix/Detox#1331

Open

adamwathan mentioned this pull request May 9, 2019

Avoid test collisions by using different directories in runInTempDirectory tailwindlabs/tailwindcss#902

Merged

PaulMest mentioned this pull request Nov 3, 2019

Include Worker ID in env similar to Jest mocha-parallel/mocha-parallel-tests#270

Open

groupsky mentioned this pull request Nov 4, 2019

JEST_WORKER_ID is not set rogeliog/jest-runner-mocha#20

Open

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

		@@ -67,13 +67,16 @@ export default class {
		}

		// Build the options once for all workers to avoid allocating extra objects.

Add unique id for each worker and pass it to the child process #5494

Add unique id for each worker and pass it to the child process #5494

Conversation

ranyitz commented Feb 8, 2018

The Problem

Summary

Test plan

Questions

codecov-io commented Feb 8, 2018 • edited Loading

Codecov Report

mjesun left a comment

Choose a reason for hiding this comment

mjesun Feb 8, 2018

Choose a reason for hiding this comment

ranyitz Feb 8, 2018

Choose a reason for hiding this comment

mjesun Feb 8, 2018

Choose a reason for hiding this comment

ranyitz Feb 8, 2018

Choose a reason for hiding this comment

mjesun commented Feb 8, 2018

ranyitz commented Feb 8, 2018 • edited Loading

mjesun commented Feb 8, 2018

ranyitz commented Feb 8, 2018 • edited Loading

File worker.js

maximilianschmitt commented Feb 8, 2018

mjesun commented Feb 8, 2018

ranyitz commented Feb 8, 2018

mjesun commented Feb 8, 2018

ranyitz commented Feb 8, 2018

evantahler commented Mar 16, 2018

alexbyk commented Mar 22, 2018

mjesun commented Mar 22, 2018

SimenB commented Mar 22, 2018 • edited Loading

ranyitz commented Mar 22, 2018 • edited Loading

alexbyk commented Mar 22, 2018 • edited Loading

mjesun commented Mar 22, 2018

mjesun commented Mar 22, 2018

alexbyk commented Mar 22, 2018

ranyitz commented Mar 22, 2018

mjesun commented Mar 23, 2018

jaredtmartin commented Apr 18, 2018

SimenB commented Apr 18, 2018

mjesun commented Apr 18, 2018

jaredtmartin commented Apr 18, 2018 via email

evantahler commented Apr 18, 2018

mjesun commented Apr 18, 2018

evantahler commented Apr 18, 2018

SimenB commented Apr 18, 2018

github-actions bot commented May 12, 2021

codecov-io commented Feb 8, 2018 •

edited

Loading

ranyitz commented Feb 8, 2018 •

edited

Loading

ranyitz commented Feb 8, 2018 •

edited

Loading

File `worker.js`

SimenB commented Mar 22, 2018 •

edited

Loading

ranyitz commented Mar 22, 2018 •

edited

Loading

alexbyk commented Mar 22, 2018 •

edited

Loading