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
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
## master

### Features

* `[jest-worker]` Assign a unique id for each worker and pass it to the child
process. It will be available via `process.env.JEST_WORKER_ID`
([#5494](https://github.com/facebook/jest/pull/5494))

## jest 22.2.1

### Fixes
Expand Down
16 changes: 16 additions & 0 deletions packages/jest-worker/src/__tests__/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,26 @@ it('tries instantiating workers with the right options', () => {
expect(Worker.mock.calls[0][0]).toEqual({
forkOptions: {execArgv: []},
maxRetries: 6,
workerId: 1,
workerPath: '/tmp/baz.js',
});
});

it('create multiple workers with unique worker ids', () => {
// eslint-disable-next-line no-new
new Farm('/tmp/baz.js', {
exposedMethods: ['foo', 'bar'],
forkOptions: {execArgv: []},
maxRetries: 6,
numWorkers: 3,
});

expect(Worker).toHaveBeenCalledTimes(3);
expect(Worker.mock.calls[0][0].workerId).toEqual(1);
expect(Worker.mock.calls[1][0].workerId).toEqual(2);
expect(Worker.mock.calls[2][0].workerId).toEqual(3);
});

it('makes a non-existing relative worker throw', () => {
expect(
() =>
Expand Down
12 changes: 12 additions & 0 deletions packages/jest-worker/src/__tests__/worker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ it('passes fork options down to child_process.fork, adding the defaults', () =>
execPath: 'hello',
},
maxRetries: 3,
workerId: process.env.JEST_WORKER_ID,
workerPath: '/tmp/foo/bar/baz.js',
});

Expand All @@ -70,6 +71,17 @@ it('passes fork options down to child_process.fork, adding the defaults', () =>
});
});

it('passes workerId to the child process and assign it to env.JEST_WORKER_ID', () => {
new Worker({
forkOptions: {},
maxRetries: 3,
workerId: 2,
workerPath: '/tmp/foo',
});

expect(childProcess.fork.mock.calls[0][1].env.JEST_WORKER_ID).toEqual(2);
});

it('initializes the child process with the given workerPath', () => {
new Worker({
forkOptions: {},
Expand Down
5 changes: 4 additions & 1 deletion packages/jest-worker/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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

const workerOptions = {
const sharedWorkerOptions = {
forkOptions: options.forkOptions || {},
maxRetries: options.maxRetries || 3,
workerPath,
};

for (let i = 0; i < numWorkers; i++) {
const workerOptions = Object.assign({}, sharedWorkerOptions, {
workerId: i + 1,
});
const worker = new Worker(workerOptions);
const workerStdout = worker.getStdout();
const workerStderr = worker.getStderr();
Expand Down
1 change: 1 addition & 0 deletions packages/jest-worker/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 :)

|};

// Messages passed from the parent to the children.
Expand Down
4 changes: 3 additions & 1 deletion packages/jest-worker/src/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ export default class {
Object.assign(
{
cwd: process.cwd(),
env: process.env,
env: Object.assign({}, process.env, {
JEST_WORKER_ID: this._options.workerId,
}),
// suppress --debug / --inspect flags while preserving others (like --harmony)
execArgv: process.execArgv.filter(v => !/^--(debug|inspect)/.test(v)),
silent: true,
Expand Down