-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
98ab223
to
aae81f9
Compare
@forty thank you for this PR. |
aae81f9
to
fc859be
Compare
@juergba thanks a lot for the review! I answered every comments, let me know if something does not make sense. |
8beaf3b
to
a8ae375
Compare
In your issue description above: please add the keyword |
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).
a8ae375
to
368c9d2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@forty thank you
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).
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
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