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

Always use workers when using the watch mode #6647

Merged
merged 1 commit into from
Jul 6, 2018
Merged

Always use workers when using the watch mode #6647

merged 1 commit into from
Jul 6, 2018

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Jul 6, 2018

When using the watch mode, we use the TTY as an input mechanism (for keystrokes), so that different options can be selected. When only one test is selected, it will run by default in band, which causes the TTY to be unresponsive.

Blacklisting "in band" mode when using the watch mode fixes the issue, since tests are executed in workers, freeing the main process.

Fixes #6599.

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

nice fix! can you also add watchAll?

@mjesun
Copy link
Contributor Author

mjesun commented Jul 6, 2018

@thymikee done 🙂

I'll wait for the build to finish.

SimenB
SimenB previously requested changes Jul 6, 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.

Aaaah, great catch!

Missing changelog though :D

@SimenB
Copy link
Member

SimenB commented Jul 6, 2018

Not sure it fixes 6599, though?

@mjesun
Copy link
Contributor Author

mjesun commented Jul 6, 2018

@SimenB added too!

@thymikee
Copy link
Collaborator

thymikee commented Jul 6, 2018

@SimenB looks like it should fix it, now the main thread is not blocked

@mjesun
Copy link
Contributor Author

mjesun commented Jul 6, 2018

@SimenB it fixes it. The issue is that they're executing jest with one single test, which forces it into runInBand mode. This works well for a non-interactive run, but it kills responsiveness if the test takes ~10 secs. From the original issue:

it completely freezes Jest until the test has run. Even if there's a typo we need to quickly fix. And we find it helpful to focus on a single test file.

@mjesun
Copy link
Contributor Author

mjesun commented Jul 6, 2018

cc / @gaearon

@thymikee thymikee dismissed SimenB’s stale review July 6, 2018 13:30

changelog addressed

@cpojer
Copy link
Member

cpojer commented Jul 6, 2018

Can you check out the comment here: #6599 (comment)

If we know the duration of the test, and the test runs faster than about three seconds can we run in band? The reason is that most people's tests are faster than that, so interrupting doesn't make sense. The single test run in band may be lots faster than than the startup time of node + reading a large haste map (at FB). Also consider how we may have an in-memory transform cache in the main process already. I think optimizing for that case is worth it because focusing on a single fast test is a common thing people do.

@mjesun
Copy link
Contributor Author

mjesun commented Jul 6, 2018

I agree with the former, but if we go that way, I'd set up a one second limit. As stated by Nielsen, one second is about the limit for the user's flow of thought to stay uninterrupted.

@cpojer
Copy link
Member

cpojer commented Jul 6, 2018

I'm fine with one second too. While this change makes things better for slow tests, I also wanna make sure we don't make it worse for all the fast tests out there.

@codecov-io
Copy link

Codecov Report

Merging #6647 into master will increase coverage by 0.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6647      +/-   ##
==========================================
+ Coverage   63.73%   63.76%   +0.02%     
==========================================
  Files         235      235              
  Lines        8935     8935              
  Branches        4        4              
==========================================
+ Hits         5695     5697       +2     
+ Misses       3239     3237       -2     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-cli/src/test_scheduler.js 37.89% <50%> (ø) ⬆️
packages/jest-cli/src/reporters/utils.js 91.26% <0%> (+0.97%) ⬆️
packages/jest-cli/src/reporters/Status.js 95.38% <0%> (+1.53%) ⬆️

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 d16d420...ddbbaac. Read the comment docs.

@mjesun mjesun merged commit 664681a into jestjs:master Jul 6, 2018
@gaearon
Copy link
Contributor

gaearon commented Jul 6, 2018

I'm trying this in Prepack, and it definitely fixes p to pause, but for some reason file system changes don't interrupt the test until it finishes. Is this expected?

@gaearon
Copy link
Contributor

gaearon commented Jul 6, 2018

Here's a gif. I'm changing the code to something that would clearly break it. It doesn't interrupt the ongoing test.

Once the test prints success, I save the file again, and now it re-runs and shows failure (this part didn't fit in the gif but trust me it happens).

Testing methodology: I applied this patch alone to compiled test_scheduler.js on top of latest stable Jest.

calebeby referenced this pull request in Pigmice2733/scouting-frontend Jul 11, 2018
This Pull Request updates dependency [jest](https://github.com/facebook/jest) from `v23.3.0` to `v23.4.0`



<details>
<summary>Release Notes</summary>

### [`v23.4.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#&#8203;2340)
[Compare Source](jestjs/jest@v23.3.0...v23.4.0)
##### Features

- `[jest-haste-map]` Add `computeDependencies` flag to avoid opening files if not needed ([#&#8203;6667](`https://github.com/facebook/jest/pull/6667`))
- `[jest-runtime]` Support `require.resolve.paths` ([#&#8203;6471](`https://github.com/facebook/jest/pull/6471`))
- `[jest-runtime]` Support `paths` option for `require.resolve` ([#&#8203;6471](`https://github.com/facebook/jest/pull/6471`))
##### Fixes

- `[jest-runner]` Force parallel runs for watch mode, to avoid TTY freeze ([#&#8203;6647](`https://github.com/facebook/jest/pull/6647`))
- `[jest-cli]` properly reprint resolver errors in watch mode ([#&#8203;6407](`https://github.com/facebook/jest/pull/6407`))
- `[jest-cli]` Write configuration to stdout when the option was explicitly passed to Jest ([#&#8203;6447](`https://github.com/facebook/jest/pull/6447`))
- `[jest-cli]` Fix regression on non-matching suites ([6657](`https://github.com/facebook/jest/pull/6657`))
- `[jest-runtime]` Roll back `micromatch` version to prevent regression when matching files ([#&#8203;6661](`https://github.com/facebook/jest/pull/6661`))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
@mjesun mjesun deleted the watch-needs-parallel branch March 14, 2019 14:59
@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 11, 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.

Run single-file test in a process if we know it takes long
7 participants