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

Enable single thread mode for runner #5712

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Mar 3, 2018

Summary

Some runners for tools that can not be run in parallel need a way
to specify that they should not be executed in parallel. This commits
adds this functionality. This closes #5706

Test plan

Unit tests are included

SimenB
SimenB previously requested changes Mar 3, 2018
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.

This needs a test.

Also. please update the changelog 🙂

@SimenB
Copy link
Member

SimenB commented Mar 3, 2018

See CI for flow error (run yarn flow locally to see it)

@DanielMSchmidt
Copy link
Contributor Author

@SimenB Somehow yarn flow has problems on my machine, I can not really tell why. I followed the contributing guide.

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ packages/jest/src/jest.js:10:17

Cannot resolve module jest-cli.

@codecov-io
Copy link

codecov-io commented Mar 4, 2018

Codecov Report

Merging #5712 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5712      +/-   ##
==========================================
+ Coverage   63.65%   63.67%   +0.02%     
==========================================
  Files         216      216              
  Lines        7918     7918              
  Branches        4        4              
==========================================
+ Hits         5040     5042       +2     
+ Misses       2877     2875       -2     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-cli/src/test_scheduler.js 27.92% <ø> (+1.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e82577...2862c29. Read the comment docs.

@DanielMSchmidt DanielMSchmidt force-pushed the add-serial-flag-for-runner branch 2 times, most recently from dfe9545 to a603c5c Compare March 4, 2018 18:45
@@ -703,6 +703,10 @@ async runTests(
): Promise<void>
```

If you need to restrict your test-runner to only run in serial rather then being
exectued in paralell your class should have the static attribute `serial` to be
Copy link
Member

Choose a reason for hiding this comment

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

can we call it isSerial?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

typos:

  • exectued => executed
  • paralell => parallel

@SimenB
Copy link
Member

SimenB commented Mar 5, 2018

Somehow yarn flow has problems on my machine

That's odd. Have you tried yarn clean-all && rm -rf node_modules && yarn? Sometimes it helps if the repo has gotten into a weird state. Make sure you're on yarn 1.3.2 or newer as well.

@DanielMSchmidt
Copy link
Contributor Author

Thank you @SimenB, will try that our 👍

@DanielMSchmidt DanielMSchmidt force-pushed the add-serial-flag-for-runner branch 2 times, most recently from 2648e46 to 0e8934e Compare March 7, 2018 19:50
@DanielMSchmidt
Copy link
Contributor Author

@SimenB Thank you for your help; I installed the dependencies with a different version of yarn.

As a general remark: I added unit tests but they "fail" because of a missing HasteFS that should be passed to the calls. It appeared to me as if I should have passed it through, but I was incapable of retrieving or mocking HasteFS (seems to be used in the jest-haste-map package, but not exposed).
I feel like there should be some more tests for this part if someone would provide a good example I would be able to contribute better tests.

@DanielMSchmidt DanielMSchmidt force-pushed the add-serial-flag-for-runner branch 2 times, most recently from ba9cb3f to 2862c29 Compare March 7, 2018 20:06
@@ -192,7 +192,7 @@ export default class TestScheduler {
onResult,
onFailure,
{
serial: runInBand,
serial: runInBand || testRunners[runner].isSerial,
Copy link
Member

Choose a reason for hiding this comment

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

what about serial: runInBand || Boolean(testRunners[runner].isSerial) to ensure we always pass a boolean down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, that makes sense 👍

@SimenB
Copy link
Member

SimenB commented Mar 8, 2018

@cpojer thoughts on the comments about testing and hastefs?

@cpojer
Copy link
Member

cpojer commented Mar 8, 2018

We should definitely pass the right arguments to the function calls. There is a createHasteContext utility in Jest's tests for this purpose.

Some runners for tools that can not be run in parallel need a way
to specify that they should not be executed in parallel. This commits
adds this functionality.

Closes jestjs#5706
@DanielMSchmidt
Copy link
Contributor Author

I think I mocked the right things now 👍

@cpojer cpojer merged commit 5a07f5f into jestjs:master Mar 8, 2018
@cpojer
Copy link
Member

cpojer commented Mar 8, 2018

Nice!

@DanielMSchmidt
Copy link
Contributor Author

Awesome, only 6 to go for a shirt 😂

@cpojer
Copy link
Member

cpojer commented Mar 8, 2018

You only need 5 total for a tshirt. So one down, 4 to go. You can do it!

thomasjinlo pushed a commit to thomasjinlo/jest that referenced this pull request Mar 9, 2018
Some runners for tools that can not be run in parallel need a way
to specify that they should not be executed in parallel. This commits
adds this functionality.

Closes jestjs#5706
thomasjinlo pushed a commit to thomasjinlo/jest that referenced this pull request Mar 9, 2018
Some runners for tools that can not be run in parallel need a way
to specify that they should not be executed in parallel. This commits
adds this functionality.

Closes jestjs#5706
@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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Allow to run a jest runner in band
6 participants