-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
Use watchAll fallback for non git/hq projects #4737
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
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
packages/jest-cli/src/cli/index.js
Outdated
| const changedFilesPromise = getChangedFilesPromise(globalConfig, configs); | ||
|
|
||
| if (globalConfig.watch && !changedFilesPromise) { | ||
| console.log( |
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.
We usually do process.stdout.write, I think
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.
done :)
|
I think this makes sense. Could you add a test? It'd be best to have an integration test, but I don't know if it's possible to write one seeing as the source code is in a repo. A unit test should be possible, though |
|
I can add some tests using mocks. I don't like to restructure the implementation within this MR. |
|
i'm not sure if we should modify the config, maybe it's better to just crash and yeah, there's no easy way to test watch mode, but you can create an integration test that checks if the process crashes with the correct error. |
|
@aaronabramov thank you for the hint, integration tests makes a lot more sense I think. I also changed the implementation to a simple print out. |
| import runJest from '../run_jest'; | ||
| import Runtime from 'jest-runtime'; | ||
| import TestWatcher from '../test_watcher'; | ||
| import updateGlobalConfig from '../lib/update_global_config'; |
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.
Lint should have yelled at us for leaving this here. It printed it as a warning, not an error
|
📎 |
|
@Journerist this breaks the jest repo. |
|
This should be reverted or fixed asap, watch is broken in git repos in current beta |
|
I ll check it right now |
|
created But to be honest, it's hard to test in a useful way. I would suggest to revert it for now to avoid hasty mistakes. |
|
added another MR added also some unit tests |
|
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. |
Summary
Whenever you run
jest --watchin a non git/hq repository you will see the following error:To make it work I would like to do the following:
Unfortunately the
cli/index.jsis not tested yet. It's hard to test without restructuring a lot of code to increase testability.If this solution is not favored I can adapt the change to just update the error message to mention the problem explicit.
There is already a bug report: #4419
Test plan