Only run tests for changed sources

[novemberborn]

Fixes #115, building on top of the --watch implementation.

Test workers keep track of test dependencies for all registered file extensions. These are sent back to the Api as part of the 'teardown' event, ensuring they're captured irrespective of whether tests passed.

The watcher rejects any dependencies that are not source files, matching how Chokidar watches for source file modifications. It maintains a list of source dependencies for each test file.

The watcher will find the test files that depend on modified source files and rerun those (along with any other modified test files). If any modified source files cannot be mapped to test files all tests are rerun. This is necessary because only require() dependencies are tracked, not file access.

Run DEBUG=ava:watcher $(npm bin)/ava --watch in a project to see the matching in action.

This required some extensive refactoring of the watcher code itself. Worth it in the end since the changes drop in quite nicely. This will conflict heavily with #536 so best if that lands first.

Additionally I found that test workers may receive multiple teardown events. This made the emits dependencies for test files Api test flaky since it would occur for uncaught exceptions. That's fixed in a separate commit.

[mention-bot]

By analyzing the blame information on this pull request, we identified @jokeyrhyme, @ariporad and @sotojuan to be potential reviewers

[novemberborn]

@sindresorhus @vdemedes @jamestalmage any feedback?

[sindresorhus]

@novemberborn I'll try to take a look tomorrow. Been busy IRL and James is off traveling :)

[vadimdemedes]

I'm a bit busy this weekend, I'll take a look during the start of the week. Thanks!

[novemberborn]

@sindresorhus @vdemedes ping

[sindresorhus]

I think it would be a better approach to send it all at once, as you've suggested.

[sindresorhus]

The approach generally looks good.

This tracks dependencies per test.

By test, I assume you mean test file?

[vadimdemedes]

LGTM from me, will clean up later myself ;)

[novemberborn]

By test, I assume you mean test file?

Yes. Naming is hard…

LGTM from me, will clean up later myself ;)

Let me take the feedback and improve on this :)

[vadimdemedes]

@novemberborn it's no big deal, few things I noticed:

1. I never use .reduce()
2. https://github.com/sindresorhus/ava/pull/544/files#diff-b72e3e25f31d6b3c3e5f40b5991dc5f6R94 raises question "old what?", var name needs to be explicit
3. I'd replace arrayUnion with merge.

Most of it and others are stylistic changes, so don't worry ;)

[novemberborn]

Updated! This should be good to go now.

New PR description:

Test workers keep track of test dependencies for all registered file extensions. These are sent back to the Api as part of the 'teardown' event, ensuring they're captured irrespective of whether tests passed.

The watcher rejects any dependencies that are not source files, matching how Chokidar watches for source file modifications. It maintains a list of source dependencies for each test file.

The watcher will find the test files that depend on modified source files and rerun those (along with any other modified test files). If any modified source files cannot be mapped to test files all tests are rerun. This is necessary because only require() dependencies are tracked, not file access.

Run DEBUG=ava:watcher $(npm bin)/ava --watch in a project to see the matching in action.

This required some extensive refactoring of the watcher code itself. Worth it in the end since the changes drop in quite nicely. This will conflict heavily with #536 so best if that lands first.

Additionally I found that test workers may receive multiple teardown events. This made the emits dependencies for test files Api test flaky since it would occur for uncaught exceptions. That's fixed in a separate commit.

---

@vdemedes

3. I'd replace arrayUnion with merge

But @sindresorhus wrote array-union! 😉 If you're referring to https://www.npmjs.com/package/merge that seems to merge objects, not arrays?

[vadimdemedes]

I meant just to change the var name, not the module :) Never mind, it's totally my preferences, not forcing them.

[novemberborn]

Pushed a commit to fix Windows. Paths are insanely confusing so I've tried to simplify a few assumptions:

• test files always use platform-specific slashes
• Chokidar emits file paths using platform-specific slashes
• tracked dependencies use platform-specific slashes
• test file and source patterns always use POSIX slashes
• on Windows, file paths are converted to use POSIX slashes before matching patterns

This way the actual calls to the various matcher libraries are the same no matter the platform and we don't have to make assumptions on how the library adjusts. Just for example with minimatch on El Capitan:

>  require('minimatch').match(['nested/file.js'],'nested/*.js')
[ 'nested/file.js' ]
>  require('minimatch').match(['nested\\file.js'],'nested/*.js')
[]
>  require('minimatch').match(['nested\\file.js'],'nested\\*.js')
[]
>  require('minimatch').match(['nested/file.js'],'nested\\*.js')
[]

On Windows 10:

> require('minimatch').match(['nested/file.js'],'nested/*.js')
[ 'nested/file.js' ]
> require('minimatch').match(['nested\\file.js'],'nested/*.js')
[ 'nested\\file.js' ]
> require('minimatch').match(['nested\\file.js'],'nested\\*.js')
[ 'nested\\file.js' ]
> require('minimatch').match(['nested/file.js'],'nested\\*.js')
[ 'nested/file.js' ]

😖

[novemberborn]

@sindresorhus @vdemedes @jamestalmage rebased on master now.

[vadimdemedes]

Cool, interesting idea @novemberborn! I don't think we need to squash commits here, just merge as it is.

[jamestalmage]

In general LGTM.

How is thorough is coverage of new code? It just seems like a high ratio of new code to tests (gut feeling after reviewing on mobile - I could be off base).

My only concern with the require hook is that it is easily broken by bad actors. Something like babel-detective might be more robust (but maybe wouldn't catch as thorough a dependency graph)

[sindresorhus]

Alright. I've been testing this now in many different projects and seems to be working good.

[sindresorhus]

My only concern with the require hook is that it is easily broken by bad actors.

Is there anything we could do to prevent this?

Something like babel-detective might be more robust (but maybe wouldn't catch as thorough a dependency graph)

I think an actual dependency graph is a lot better than what a static analysis could do.

[sindresorhus]

Merging. We can follow-up with any improvements/requests. Really good stuff @novemberborn :)

[jamestalmage]

Is there anything we could do to prevent this?

Maybe instead of a conventional hook we wrap Module.prototype.compile? That is a no-no for conventional hooks (not really extendable), but since our hook doesn't manipulate the args (it just tracks files) I don't think it would be a problem

I think an actual dependency graph is a lot better than what a static analysis could do.

I agree.

[novemberborn]

@jamestalmage,

How is thorough is coverage of new code? It just seems like a high ratio of new code to tests (gut feeling after reviewing on mobile - I could be off base).

Probably looked that way due to the refactor I did. That had little test impact though.

My only concern with the require hook is that it is easily broken by bad actors. Something like babel-detective might be more robust (but maybe wouldn't catch as thorough a dependency graph)

Yea it's icky. If it's any consolation the require hook is added after the custom requires have loaded, so unless hooks are added afterwards we'll be able to capture any dependencies. I'll make sure to explain this in the documentation.

babel-detective would let us capture the direct dependencies of the test file but not transitive dependencies. That's where this feature really shines: you have tests for Foo and Bar and Baz, the latter two depending on thingybob. When you change thingybob you don't need to retest Foo.

Maybe instead of a conventional hook we wrap Module.prototype.compile? That is a no-no for conventional hooks (not really extendable), but since our hook doesn't manipulate the args (it just tracks files) I don't think it would be a problem

I don't think that would work for native and JSON dependencies?

Merging. We can follow-up with any improvements/requests. Really good stuff @novemberborn :)

Thanks @sindresorhus!

[sindresorhus]

Maybe instead of a conventional hook we wrap Module.prototype.compile? That is a no-no for conventional hooks (not really extendable), but since our hook doesn't manipulate the args (it just tracks files) I don't think it would be a problem

I like this idea. Could even be a separate module we consume. @novemberborn hint hint ;)

@novemberborn Also feel free to extract anything else into modules where it makes sense.

[sindresorhus]

I don't think that would work for native and JSON dependencies?

Yup, that's true, since they don't call ._compile, but those are also rarely overriden, so a combination of ._compile + hooks for JSON/native could work.

https://github.com/nodejs/node/blob/0bea78682a0530cc51d3dfeab504352034a9fc02/lib/module.js#L423-L445

[novemberborn]

Yup, that's true, since they don't call ._compile, but those are also rarely overriden, so a combination of ._compile + hooks for JSON/native could work.

Oh that sounds good. I considered extracting it initially but then figured it wasn't quite interesting enough. Combining it with ._compile does make it more interesting. I've added it to my personal to-do list…