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

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

Merged
merged 6 commits into from
Feb 8, 2018

Conversation

ranyitz
Copy link
Contributor

@ranyitz ranyitz commented Feb 8, 2018

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:

  1. Run the test serially. -> slow... 😞
  2. 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

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

Questions

  • should we update the readme with this new feature?

@codecov-io
Copy link

codecov-io commented Feb 8, 2018

Codecov Report

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

Impacted file tree graph

@@            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.

Copy link
Contributor

@mjesun mjesun left a comment

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Alphabetically sorted props 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep :)

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree

@mjesun
Copy link
Contributor

mjesun commented Feb 8, 2018

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
Copy link
Contributor Author

ranyitz commented Feb 8, 2018

@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
Copy link
Contributor

mjesun commented Feb 8, 2018

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

@ranyitz
Copy link
Contributor Author

ranyitz commented Feb 8, 2018

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
Copy link

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

@mjesun
Copy link
Contributor

mjesun commented Feb 8, 2018

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

@ranyitz
Copy link
Contributor Author

ranyitz commented Feb 8, 2018

@mjesun Example added.
I think this is ready 🎉

@mjesun
Copy link
Contributor

mjesun commented Feb 8, 2018

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

@ranyitz
Copy link
Contributor Author

ranyitz commented Feb 8, 2018

no problem, thanks for the review!

@mjesun mjesun merged commit efec054 into jestjs:master Feb 8, 2018
@ranyitz ranyitz deleted the jest-worker-id branch February 8, 2018 12:01
@evantahler
Copy link

Hey! Thanks for solving #2284!

@alexbyk
Copy link

alexbyk commented Mar 22, 2018

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

@mjesun
Copy link
Contributor

mjesun commented Mar 22, 2018

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
Copy link
Member

SimenB commented Mar 22, 2018

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

@ranyitz
Copy link
Contributor Author

ranyitz commented Mar 22, 2018

@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
Copy link

alexbyk commented Mar 22, 2018

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
Copy link
Contributor

mjesun commented Mar 22, 2018

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
Copy link
Contributor

mjesun commented Mar 22, 2018

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

@alexbyk
Copy link

alexbyk commented Mar 22, 2018

@mjesun thanks for the explanation.

@ranyitz
Copy link
Contributor Author

ranyitz commented Mar 22, 2018

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

@mjesun
Copy link
Contributor

mjesun commented Mar 23, 2018

@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
Copy link

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
Copy link
Member

SimenB commented Apr 18, 2018

It should be there.

@mjesun

@mjesun
Copy link
Contributor

mjesun commented Apr 18, 2018

@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
Copy link

jaredtmartin commented Apr 18, 2018 via email

@evantahler
Copy link

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
Copy link
Contributor

mjesun commented Apr 18, 2018

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

@evantahler
Copy link

Oh! Wonderful.

@SimenB
Copy link
Member

SimenB commented Apr 18, 2018

In #5860 fwiw

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants