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

fix: the number of concurrent job with --parallel option in mocha:cli:run:helpers #4416

Merged
merged 3 commits into from
Oct 12, 2020

Conversation

sujin-park
Copy link
Contributor

@sujin-park sujin-park commented Aug 25, 2020

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

When users run mocha with --parallel option wihtout --jobs option, undefined was passed to the parallelRun method.

I changed options.jobs to the number of CPU cores less 1 when options.jobs was false.

AS-IS
스크린샷 2020-08-25 오후 6 09 07

TO-BE
스크린샷 2020-08-25 오후 6 08 37

Alternate Designs

in lib/cli/run-helpers.js from line 179

AS-iS

options.jobs

TO-BE

options.jobs ? options.jobs : workerpool.cpus - 1

Why should this be in core?

It's a confirmed bug as described in #4415.

Benefits

Users can know exact current concurrent job count instead NaN.

Possible Drawbacks

N/A

Applicable issues

#4415

@coveralls
Copy link

coveralls commented Aug 25, 2020

Coverage Status

Coverage increased (+0.3%) to 94.078% when pulling e0ae3a5 on sujin-park:sujin-park/fix-parallel into 4d7a171 on mochajs:master.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks! Just have a small suggestion to simplify things

@@ -175,7 +176,7 @@ const parallelRun = async (mocha, options, fileCollectParams) => {
debug(
'executing %d test file(s) across %d concurrent jobs',
files.length,
options.jobs
options.jobs ? options.jobs : workerpool.cpus - 1
Copy link
Contributor

@boneskull boneskull Aug 26, 2020

Choose a reason for hiding this comment

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

I would just remove the job count. It's printed in the debug logs elsewhere:

debug(
'run(): starting worker pool of max size %d, using node args: %s',
maxWorkers,
process.execArgv.join(' ')
);

maybe just say executing %d test file(s) in parallel mode

Copy link
Contributor Author

@sujin-park sujin-park Aug 26, 2020

Choose a reason for hiding this comment

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

I fixed your feedback. I removed options.jobs params and added text in parallel mode.
Thank you for your comment.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Aug 26, 2020
@sujin-park sujin-park requested a review from boneskull September 9, 2020 13:07
@boneskull boneskull merged commit 31116db into mochajs:master Oct 12, 2020
@boneskull
Copy link
Contributor

thanks!

@boneskull boneskull added this to the next milestone Oct 12, 2020
@boneskull boneskull modified the milestones: next, v8.2.0 Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch implementation requires increase of "patch" version number; "bug fixes" type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants