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.

Sign up for GitHub

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

Jump to bottom

Concurrency limit #7770

Merged
merged 11 commits into from
Feb 1, 2019
Merged

Concurrency limit #7770

merged 11 commits into from
Feb 1, 2019

Conversation

arcanis
Copy link
Contributor

@arcanis arcanis commented Jan 31, 2019 •
edited by SimenB
Loading

Summary

The current implementation of test.concurrent makes it quite difficult to use since all the tests in a testsuite run concurrently, swamping the CPU (even worse when there's a lot of I/O).

This PR implements a queue so that at most 5 tests ever run at the same time within the same testsuite. I'd like to implement an option for that, but I'm not too sure how to get the Jest options from within Jasmine.

Fixes #6183

Test plan

Added a test.

@rickhanlonii
Copy link
Member

Note this would close #6183

SimenB
SimenB approved these changes Jan 31, 2019
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Nice! Can we fix this for circus as well?

rickhanlonii
rickhanlonii reviewed Jan 31, 2019
View reviewed changes
Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

Dope solution, if we make this configurable and default to Infinity can it go in a minor?

e2e/__tests__/jasmineAsync.test.js Show resolved Hide resolved
packages/jest-jasmine2/src/jasmineAsyncInstall.js Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Jan 31, 2019 •
edited
Loading

I'd like to implement an option for that, but I'm not too sure how to get the Jest options from within Jasmine.

We pass GlobalConfig and ProjectConfig in when initialising jasmine/circus:

https://github.com/facebook/jest/blob/3edccf64a6f46350d0e2688af79ae52a080ad9f4/packages/jest-jasmine2/src/index.js#L25-L31
https://github.com/facebook/jest/blob/3edccf64a6f46350d0e2688af79ae52a080ad9f4/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapterInit.js#L27-L43

@arcanis
Copy link
Contributor Author

arcanis commented Jan 31, 2019

Updated! I've added the maxConcurrency flag (defaults to 5).

SimenB
SimenB requested changes Jan 31, 2019
View reviewed changes
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

CI is failing due to an outdated snapshot, could you update it?

Also, please update the docs to mention this opton

CHANGELOG.md Outdated Show resolved Hide resolved
SimenB
SimenB reviewed Jan 31, 2019
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Jan 31, 2019

The new test is failing on node 6. 😞

@arcanis
Copy link
Contributor Author

arcanis commented Feb 1, 2019

The remaining fails look unrelated, right?

@SimenB
Copy link
Member

SimenB commented Feb 1, 2019

The remaining fails look unrelated, right?

Yup, we're having some issues with windows on CI currently

@SimenB SimenB merged commit 242f3bc into jestjs:master Feb 1, 2019
@arcanis arcanis deleted the concurrency-limit branch February 1, 2019 10:15
@SimenB SimenB mentioned this pull request Mar 14, 2019
Closed
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
@arcanis @captain-yossarian
Concurrency limit (jestjs#7770)
efe8fa6
@SimenB SimenB mentioned this pull request Oct 15, 2019
Closed
@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.
Reviewers

@rickhanlonii rickhanlonii rickhanlonii left review comments

@SimenB SimenB SimenB approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

We need a way to limit to execute tests in a single file concurrently without swamping the CPU
4 participants
@arcanis @rickhanlonii @SimenB @facebook-github-bot