Implement --watch

[novemberborn]

Implement --watch

This PR implements --watch #70. Yay!

• Recognizes --watch and -w
• Detects when tests are changed (or added), reruns just those tests 🚀
• If other files are added/changed/deleted it reruns all tests
• Like in nodemon you can type rs\n to rerun all tests. This is useful when you made a change that was not picked up.

There's a fair amount of changes and some unresolved issues, apologies for the length!

---

Tests files can now be run more than once. This means the loggers (reporters) and the API have to be reset between runs. The mini reporter retains state, as does the API.

The API modified the files list when running. It now acts on a copy of the list. The default include and exclude patterns are now exposed on the API instance so the watcher can use them.

Incidentally these changes should make it possible for an 3rd-party API consumer to rerun tests without reinstantiating the API.

The new CLI flags are left undocumented for now.

Errors that occur during the initial run cause AVA to exit. Errors that occur during subsequent runs are logged but AVA does not exit. This is good as AVA picks up new test files as they are created (and might be empty). A case could be made though that --watch should simply never exit (unless chokidar is missing).

--watch logs an error and exits if chokidar is missing, like babel/babel#3307.

There is some debouncing to avoid rerunning tests prematurely. The delay is 10 milliseconds. If further changes are detected the watcher again delays by 10 milliseconds. Currently this delay is not configurable. We should probably keep it that way unless it becomes a problem.

The watcher waits for the previous test run to finish. Even though AVA supports parallel tests, the API does not support parallel test runs.

Before, when running a single test file, the test titles would not be prefixed with the file name. Now, when the watcher reruns specific tests, the test titles are always prefixed. The log should disambiguate between not just the currently running tests but all tests that have run previously. Note though that in the current implementation, when the all tests are rerun the watcher doesn't force the prefix. This causes an inconsistency in case there is only a single test file. Maybe that should be changed.

The title prefix change uncovered a bug in commondir. It returns the full file path if called with only a single file. I've added a commit with a workaround but then swapped it for comomn-path-prefix which handles this case fine.

chokidar can be configured with include and exclude patterns. I've defaulted these to package.json,**/*.js and .git,node_modules,bower_components,.sass-cache respectively. The latter pattern is used by nodemon. We should expand on it a bit and put it in a separate package which can then also be used by nodemon. Matching package.json should allow tests to rerun when dependencies are changed.

I believe that the exclude patterns should be made configurable. E.g. source-map-fixtures has a test which writes to disk. When testing this branch it caused the tests to be rerun continuously. The same goes for the include pattern. Right now it assumes your source files end in *.js.

Putting patterns in a CLI flag is tricky though given that the ava invocation is supposed to end with a pattern. Perhaps we could use --watch-pattern="" for this case. The pattern should behave the same as the files pattern.

Similarly we could bikeshed over the input command that causes all tests to be rerun. I used rs which I'm used to from nodemon. (And we could quibble over the feature itself as well, of course 😄)

One final observation: the API resolves tests recursively if the pattern matched a directory. This makes it harder to verify whether a changed file actually matches the files pattern. Perhaps we can require patterns to explicitly match files inside directories? I.e. change test to test/**/*.js.

---

I still need to update/add tests.

Also it'd be pretty cool to track which files tests are depending on, so if one of those files are changed only the applicable tests are rerun. Perhaps after this lands 😉

Thanks for reading this far.

[jamestalmage]

A case could be made though that --watch should simply never exit

👍 That is what I would expect. If I launch with watch and there are initial problems, I should just be able to fix them and save. What is the argument for the behavior in this PR?

[jamestalmage]

The delay is 10 milliseconds. If further changes are detected the watcher again delays by 10 milliseconds. Currently this delay is not configurable

I agree it should not be configurable, but is 10ms long enough? What happens in popular IDE's when you have multiple large files open and you Save All? How fast does chokidar pick those up? (Especially on slow systems... Especially on slow Windows systems). Would a 50ms/100ms debounce really be noticeable? Would a higher value provide any benefit? I am not disagreeing with the chosen value, I truly don't know the answer, and I am just suggesting we put some thought into it.

The easiest solution might just be to go with 10 ms and wait for reported problems.

[jamestalmage]

.nyc_ouput and coverage should almost certainly be in the default excludes. It wouldn't make a lot of sense to watch with coverage enabled, but we should protect against it.

Maybe try to detect if we are wrapped with NYC and issue a warning in watch mode?

[jamestalmage]

RE: include / exclude patterns:

Might we just provide a single config option with a ! prefix for exclusion?

defaults:

ava --watch --watch-files={,lib/**/}*.js test*.js test/**/*.js

with exclusion

ava --watch --watch-files=**/*.js --watch-files=!node_modules/**

[novemberborn]

A case could be made though that --watch should simply never exit

That is what I would expect. If I launch with watch and there are initial problems, I should just be able to fix them and save. What is the argument for the behavior in this PR?

No particular argument, that's why I brought it up :) Will change.

The delay is 10 milliseconds. If further changes are detected the watcher again delays by 10 milliseconds. Currently this delay is not configurable

I agree it should not be configurable, but is 10ms long enough? What happens in popular IDE's when you have multiple large files open and you Save All? How fast does chokidar pick those up? (Especially on slow systems... Especially on slow Windows systems). Would a 50ms/100ms debounce really be noticeable? Would a higher value provide any benefit? I am not disagreeing with the chosen value, I truly don't know the answer, and I am just suggesting we put some thought into it.

I don't know either. Note that the current implementation is somewhat naive in that it debounces again for the full 10ms if a change happened in the interval. Increasing the delay would make that hurt more, but some extra code complexity could deal with that. It'd be good if tests rerun within 150ms if possible though, so that it feels instantaneous.

The easiest solution might just be to go with 10 ms and wait for reported problems.

Yea.

.nyc_ouput and coverage should almost certainly be in the default excludes. It wouldn't make a lot of sense to watch with coverage enabled, but we should protect against it.

I agree with excluding those, but note that you may have watch in one console and run coverage in another.

I don't think we should bail if you run watch inside of nyc, or a CI environment for that matter.

RE: include / exclude patterns:

Might we just provide a single config option with a ! prefix for exclusion?

Yes that was my thinking too. That's how the files are matched.

[novemberborn]

Currently the process exits if a test file cannot be run (an AvaError is thrown). That shouldn't happen when using --watch since it may run test files before you've had a chance to write the AVA boilerplate. We can make sure the watcher doesn't exit, sure, but the behaviour in the API itself still seems subpar. The exception propagates and the results of other (concurrent) tests are ignored.

Getting sensible log output for these scenarios is difficult. Ideally we see as much output as possible, ending with the number of passes, fails, rejections and exceptions, including the AvaErrors.

I'm proposing to handle these AvaErrors as exceptions and logging them in the various reporters. Tests will continue to run and the errors will be shown at the end (depending on the reporter). Then any exception coming out of Api#run() is unexpected, meaning we could even crash the process with an uncaught exception.

I'm adding a commit to this effect. Apologies if it's a tad controversial 😄

[jamestalmage]

AVAError was intended to be used to avoid printing stack traces when they were unlikely to be helpful (i.e. the "no tests in file", or " forgot to require AVA" errors). I don't think this change will be that controversial

[sindresorhus]

Why in the next tick?

[jamestalmage]

Also, nextTick is deprecated since 0.10.0

[novemberborn]

Heh, old habits die hard. Will change to setImmediate(). It's needed to rethrow as an uncaught exception.

[sindresorhus]

This looks very promising. I'm happy about how non-intrusive this is. Great work @novemberborn :)

Putting patterns in a CLI flag is tricky though given that the ava invocation is supposed to end with a pattern. Perhaps we could use --watch-pattern="" for this case. The pattern should behave the same as the files pattern.

How about ava test/*.js --watch=src/*.js --watch=!src/bundle.js?

Like in nodemon you can type rs\n to rerun all tests. This is useful when you made a change that was not picked up.

Do you know why rs\n was chosen? Just r would be faster to type.

[jamestalmage]

How about ava test/*.js --watch=src/*.js --watch=!src/bundle.js

The issue with that is that the following is ambiguous:

ava --watch test/foo.js

Does it mean watch only test/foo.js and run the default test setup? Or only run test/foo.js and watch all the normal files?

[sindresorhus]

Does it mean watch only test/foo.js and run the default test setup? Or only run test/foo.js and watch all the normal files?

That's why we define types here, to make it non-ambigious. Also why I much prefer using --key=val instead of --key val, as it will never be ambiguous.

[jamestalmage]

That's why we define types here,

That won't help here. They are both strings.

Also why I much prefer using --key=val instead of --key val

I agree. But unless we parse args on our own, I don't think we can enforce that (nor do I think we should - it would confuse the people who prefer spaces).

[sindresorhus]

Easier to explain with code:

var minimist = require('minimist');

console.log(minimist(['ava', '--watch', 'src/*.js', 'test/*.js'], {string: ['watch']}));
/*
{
    _: ['ava', 'test/*.js'],
    watch: 'src/*.js'
}
*/

http://requirebin.com/?gist=0b900158776c2a2a4100

[jamestalmage]

I get that, what I want to be able to do is run --watch in focus mode, so:

ava --watch test/foo.js

Only run test/foo.js, but do it in watch mode. Having to specify:

ava --watch={src,lib,scripts,}**/*.js test/foo.js 

Would be annoying. I am probably going to set up what I want watched in package.json anyways, so I'd prefer --watch be a boolean.

Maybe we add a --sources flag? I mean that's what we are really after right? We always want to watch the tests, if they change, we almost certainly want to rerun. For watch, we need to tell AVA what additional sources need to be looked at above and beyond the tests.

[jamestalmage]

--sources would mirror nicely into package.json as well. (Specifying "watch": "pattern" might seem to suggest that you wanted it to default to watch mode).

[sindresorhus]

Ok, good point. Was just trying to not have to introduce yet another flag... --sources sounds good.

[novemberborn]

@sindresorhus

Do you know why rs\n was chosen? Just r would be faster to type.

Will investigate.

Ok, good point. Was just trying to not have to introduce yet another flag... --sources sounds good.

Yes, I quite like --sources! Will add.

[novemberborn]

OK I found the introduction of rs\n: remy/nodemon#162 (comment)

It's muscle memory for me but I don't really care either way.

[novemberborn]

OK, status update.

Summary of changes:

• Implements --watch (and -w) flags
  • These are undocumented for the time being
• Reruns specific tests when changes to those tests are detected, including running new tests
• Reruns all tests (according to the files patterns) when source changes are detected (non-test changes)
• Type rs and hit return to rerun all tests, like nodemon
• AvaErrors that occur during test runs are treated as unhandled exceptions
  • Tests continue to run
  • Fixes Async uncaught exception not shown in default reporter and has useless stack #511
• Existing tests where updated to reflect changes in behavior
• 100% test coverage for watcher module

Still to do:

• Implement --sources flag so users can customize which source changes should cause all tests to be rerun
• Extract default exclusion patterns for sources into a standalone module (these were copied from nodemon but should be extended, and could be shared)

Future enhancements:

• Track which tests depend on which sources, rerun just those tests when sources change (will open new issue to track)

[novemberborn]

Added --sources support.

[vadimdemedes]

Re https://github.com/sindresorhus/ava/pull/502/files#diff-4fdccd335c2e54ebb16284a24389ce67R294, I don't think we should wait for the previous tests to finish, if new ones are in a queue. If developer made a code update, he obviously does not care whether tests fail/pass before that last update. He wants to see the immediate test log after his change, right?

Impressive work, @novemberborn!

[novemberborn]

I don't think we should wait for the previous tests to finish, if new ones are in a queue. If developer made a code update, he obviously does not care whether tests fail/pass before that last update. He wants to see the immediate test log after his change, right?

If I modify a test that is currently running perhaps I'd like that to be interrupted and restarted. We could see if there's a strong need for that. Alternatively AVA could simply start a second run of the test but I imagine that would make for some very confusing log output.

Right now the API runs multiple test files and retains state during those runs. That state can only be reset after a run has finished, which is why the watcher has to wait. I don't think that's so bad, you'll keep an eye on your test output and wait for it to stop changing.

Impressive work, @novemberborn!

Thanks! 😄

[novemberborn]

And finished the final to-do, ignore-by-default is now used to control the directories ignored by the watcher.

Force-pushed to include some bugfixes. Aside from further feedback this is good to go 🚀

[vadimdemedes]

I meant, we could cancel the "old" test run (so that we no longer get results/callbacks/etc from it) and start a new one along with a new output.

[novemberborn]

I meant, we could cancel the "old" test run (so that we no longer get results/callbacks/etc from it) and start a new one along with a new output.

Yea that'd be an option. Though that would probably preclude after cleanup code from running in the tests so may not be ideal.

[vadimdemedes]

We can leave it as a feature request/improvement, so that this PR gets in faster ;) Don't want that amount of hard work to just wait around!

[jamestalmage]

Why not use a new Api instance each time instead of resetting?

[vadimdemedes]

Very good point.

[novemberborn]

Force pushed some updates:

• Fix merge conflict
• Remove completed TODO as noted by @jamestalmage
• Fix test matching to cover foo/*/*bar patterns hwere foo/qux/bar is a directory
• Changed sources parameter to --source to be in line with --require (both can be repeated)
  • This means the key in package.json is also source, which is different from files, but consistent with require. Thoughts?

---

Any comments on these points?

Regarding instantiating new a new API for each run, and new reporters too. It's icky because cli.js creates the API with its options object. Same goes for the reporters, which hold a reference to the API instance. Further, reporters assume they're the only thing writing to stdout.

We could wrap instantiation in a method and pass that to the watcher but that doesn't seem much cleaner than resetting the existing instances.

---

Regarding the optional files argument to API#run(). Originally I moved it out of the constructor but then noticed #83. We could still make that consistent but it'd be a breaking change.

I might still want a second parameter to force the test titles to be prefixed, as mentioned earlier in this PR.

[jamestalmage]

I think you've got a solid public API in place. I think we should merge and open am issue regarding cleaning up the internals

[sindresorhus]

This means the key in package.json is also source, which is different from files, but consistent with require. Thoughts?

👍

[sindresorhus]

✨Amazing work on this @novemberborn. It turned out really good! 🍻

[sindresorhus]

and open an issue regarding cleaning up the internals

@novemberborn @jamestalmage Can either of you open issues for follow-up things to discuss?

[forresst]

@sindresorhus It would be interesting to add it in the doc (readme.md) ?

[sindresorhus]

@forresst Yes, but undocumented for now as we want to make sure it's good before we get a gadzillion support issues for it.

[forresst]

@sindresorhus 👍

[novemberborn]

✨ Amazing work on this @novemberborn. It turned out really good! 🍻

💥 !

@forresst tracking in #532 now.

[novemberborn]

Full list of follow-up issues:

• Document --watch #532 document --watch
• Investigate instantiating new API and reporter in --watch #533 investigate instantiating new API and reporter in --watch
• Abort old tests and start new ones after a code change in watch mode #518 abort old tests and start new ones after a code change in watch mode
• Consider moving files patterns from API constructor to run() method #534 consider moving files patterns from API constructor to run() method
• Suggestion: Run affected tests when sources change #535 run affected tests when sources change

(Feel free to open more for any issues I missed)

[jamestalmage]

This is just fantastic work @novemberborn! Well done Sir!