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

Assign each worker a worker id in parallel mode. #4813

Merged
merged 1 commit into from Jan 22, 2022

Conversation

forty
Copy link
Contributor

@forty forty commented Jan 17, 2022

This change is similar to #4463 but is cleaner as is uses a new feature from workerpool (onCreateWorker) to override each process environment when each process is launched. I'm copying most of the description.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

This change addresses #4456. It adds a MOCHA_WORKER_ID environment variable that is unique for each worker, so that it may be used for by any test resources that need to use global resources (network ports, filesystem paths, etc.) but ensure that these are unique in order to avoid conflicts during parallel tests.

Alternate Designs

#4463 is an alternate way of doing the same thing, but this one is cleaner (less hacky).

#4463 suggest forkArgs might have been used, but this seem really unsafe and hacky.

Why should this be in core?

It is common for tests to use global resources like network ports, which can conflict during parallel testing. Without a feature like this in core, users would be left on their own to create a home-brewed solution (using filesystem files as signals, sockets that elect a leader in the cluster so that one process hands out all global data, a standalone cluster orchestration system), all of which add a lot of complication to writing tests. This would also be a difference when writing tests that run in series vs. parallel, as this concern would not exist (and potentially needs to be turned off) for test in series, and would add pain points when taking existing serial tests and having them run in parallel. This makes using mocha significantly less fun.

This feature takes that pain point away. For example, allocating ports would once again be trivial, using const port = 1234 + Number(process.env.MOCHA_WORKER_ID || 0).

Other parallel testing frameworks, like jest, already use this approach: jestjs/jest#2284

Benefits

This solves common pain points for developers writing tests.

Possible Drawbacks

Other than more complexity, I can't think of a drawback for the feature itself. The drawback noted by #4463 is mostly addressed by this PR, by using a cleaner way to override each process environment. Another pool implementation could easily implement a similar feature.

Applicable issues

Enter any applicable Issues here.

closes #4456

Mocha follows semantic versioning: http://semver.org

Is this a breaking change (major release)? No

Is it an enhancement (minor release)? Yes

Is it a bug fix, or does it not impact production code (patch release)? No

@juergba juergba added type: feature enhancement proposal area: node.js command-line-or-Node.js-specific area: parallel mode Regarding parallel mode semver-minor implementation requires increase of "minor" version number; "features" labels Jan 17, 2022
@juergba
Copy link
Member

juergba commented Jan 17, 2022

@forty thank you for this PR.
I will need some time to get into this topic and review your proposition.

lib/nodejs/buffered-worker-pool.js Show resolved Hide resolved
test/integration/fixtures/parallel/testworkerid1.mjs Outdated Show resolved Hide resolved
test/integration/parallel.spec.js Outdated Show resolved Hide resolved
test/integration/parallel.spec.js Outdated Show resolved Hide resolved
@forty
Copy link
Contributor Author

forty commented Jan 18, 2022

@juergba thanks a lot for the review! I answered every comments, let me know if something does not make sense.

@forty forty force-pushed the forty/worker-id branch 2 times, most recently from 8beaf3b to a8ae375 Compare January 21, 2022 11:22
docs/index.md Show resolved Hide resolved
@juergba
Copy link
Member

juergba commented Jan 21, 2022

In your issue description above: please add the keyword closes before the applicable issue No. This is a GH keyword (as fixes).

This uses a new feature from the workerpool module, which allows
overriding process parameters for each process, which is why the
workerpool module has been updated to the latest version (6.2.0).
@juergba juergba added the run-browser-test run browser tests on forked PR if code is safe label Jan 21, 2022
@github-actions github-actions bot removed the run-browser-test run browser tests on forked PR if code is safe label Jan 21, 2022
Copy link
Member

@juergba juergba left a comment

Choose a reason for hiding this comment

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

@forty thank you

@juergba juergba requested a review from a team January 21, 2022 13:55
@juergba juergba added this to the next milestone Jan 21, 2022
@juergba juergba merged commit 8b089a2 into mochajs:master Jan 22, 2022
KuznetsovRoman pushed a commit to gemini-testing/mocha that referenced this pull request Sep 6, 2022
This uses a new feature from the workerpool module, which allows
overriding process parameters for each process, which is why the
workerpool module has been updated to the latest version (6.2.0).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: node.js command-line-or-Node.js-specific area: parallel mode Regarding parallel mode semver-minor implementation requires increase of "minor" version number; "features" type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

parallel workers should have an ID that can be used to prevent conflicts among workers
2 participants