Implement watching #96
Conversation
| var watcher = sane(dir); | ||
| watcher.on('add', handler); | ||
| watcher.on('delete', handler); | ||
| watcher.on('change', handler); |
There was a problem hiding this comment.
I'll probably implement all event and update this amasad/sane#21
Add watching option to the binary. The way this feature works is that it runs all tests on the first run when --watch is passed and then idles while watching the testPathDirs and when something is changed, deleted, or added, tests will be run that relates to the changed file (or dir). I implemented this in TestRunner to be able to reuse it internally. And by doing so I had to move a lot of the file pattern matching logic into the class. But I think it's seems like it belonged there anyways.
|
Ping! |
|
Whoa I hadn't even seen this -- I will take a look when I get back in on Thurs |
| * @return {Promise<Object>} Fulfilled with aggregate pass/fail information | ||
| * about the last run of tests. | ||
| */ | ||
| TestRunner.prototype.start = function() { |
There was a problem hiding this comment.
Hmm, right now TestRunner is structured as a library utility for various methods one might need to implement a CLI.
This is intentionally kept separate from the cli JS to support the ability to build custom CLI scripts as needed (for example, we use a custom CLI at Facebook -- but all of the common operations like finding files given a pattern, running tests in parallel/in-band, etc are just methods on the TestRunner object). The only reason TestRunner isn't just a bag of utility functions (i.e. the reason it's a class) is because all of the util functions depend on stuff from the config object. I'd like to keep this pattern because it will make it easier to write custom uses of the Jest libs -- vs pushing higher level CLI decisions down into the library.
That said, I think it makes a lot of sense for those helpers and methods to return a stream object rather than a Promise. I had started to refactor TestRunner to do this before, but didn't quite finish. It turns out we can squeeze other bits of perf out of doing this as well (but that's kind of a separate topic).
So rather than pushing interface-level decisions like "should I watch? or should I run on changed paths? (etc)" into TestRunner, it would be nice if we could just add another method to TestRunner that streams changed files back out to the CLI. That way CLIs can decide how they want to handle those events (maybe some CLIs may want to do some pre- or post-processing before running a test? Or report back to a logger? etc)
There was a problem hiding this comment.
Hmm, right now TestRunner is structured as a library utility for various methods one might need to implement a CLI.
I got the impression that it's stateful class that's responsible for running the test suite. But now you mention this, things make more sense.
This is intentionally kept separate from the cli JS to support the ability to build custom CLI scripts as needed (for example, we use a custom CLI at Facebook -- but all of the common operations like finding files given a pattern, running tests in parallel/in-band, etc are just methods on the TestRunner object). The only reason TestRunner isn't just a bag of utility functions (i.e. the reason it's a class) is because all of the util functions depend on stuff from the config object. I'd like to keep this pattern because it will make it easier to write custom uses of the Jest libs -- vs pushing higher level CLI decisions down into the library.
Understandable. But in reality before I wrote this diff I had only glanced over the Facebook CLI code and just wrote this to work with the package's CLI. And then when it came time to integrate it in FB it was rather easy (after understanding the CLI code). Just moving of things from if statements and method calls to the options. That's what I thought it boiled down to, leaving a lot of the decisions duplicated in CLIs or push them down to the TestRunner. I guess my object-oriented habits wanted to centralize those decisions and push them down into the class. Not going to lie, for me it makes it easier to reason about the code.
That said, I think it makes a lot of sense for those helpers and methods to return a stream object rather than a Promise. I had started to refactor TestRunner to do this before, but didn't quite finish. It turns out we can squeeze other bits of perf out of doing this as well (but that's kind of a separate topic).
Yeah that sounds like a better design decision.
So rather than pushing interface-level decisions like "should I watch? or should I run on changed paths? (etc)" into TestRunner, it would be nice if we could just add another method to TestRunner that streams changed files back out to the CLI. That way CLIs can decide how they want to handle those events (maybe some CLIs may want to do some pre- or post-processing before running a test? Or report back to a logger? etc)
Cool, that sounds good as well.
|
ping |
|
Ping, could this feature get a bit more love, please? It's so great to have instant feedback on saving, without leaving the editor.... |
|
Ping! |
1 similar comment
|
Ping! |
|
ping |
|
+1 |
|
+1 |
|
Cool, I think I'll send a new PR shortly |
|
+1 |
|
👍 |
|
Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed. |
|
@amasad are you still working on this? |
|
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. |
The way this feature works is that it runs all tests on the first run when
--watchis passed and then idles while watching thetestPathDirsand when something has changed, deleted, or added, tests will be run that relates to the changed file (or dir).I implemented this in
TestRunnerto be able to reuse it internally. And by doing so I had to move a lot of the file pattern matching logic into the class. But I think it's seems like it belonged there anyways.closes #38 #7
cc @benjamn @jeffmo @tomocchino