🚀 Feature: Integrate with code coverage for a "--changed" style option

[craigtaub]

We have had reports of issues using 3rd party code coverage tools and the question around why mocha doesn't come with one natively. We could potentially use the Node v8 coverage tool if its works well enough, but we would have to be clear about support for browser + node usage. This could even be a separate package.

If we were to add one we would need to make sure we were still able to integrate with any others, keeping our philosophy around being flexible and composable.

[craigtaub]

@boneskull will be speaking to the Loopback + TypeScript teams at IBM, who have both reported issues, to understand the problem.

[Munter]

I have had success using nyc in all the places where I needed code coverage. I think it would add a great deal of overhead if we added code coverage into mocha itself

[boneskull]

to be clear: LoopBack team is at IBM and the TypeScript team is the TypeScript team. 😄

[boneskull]

The LB team doesn't want this per se; it's a means to an end. Quoting @bajtos:

A watch mode that's using code coverage to determine which tests to re-run after a change

This doesn't necessarily mean we need code coverage to implement "minimal re-runs"; I'll create a new issue for that.

However, there's still some unknown ask from the TypeScript team on this; still trying to get more information.

[Munter]

A watch mode with incremental tests should start with just the dependency graph knowledge based on the files. Potentially that could be further improved with extending that knowledge into es module import/export pairs. The problem when you go beyond the file level is always figuring out if there are side effects that need to be taken into account.

Jest has an incremental mode. Maybe we can learn something there. Ava has dependency tracking as well

[bajtos]

Hello from the LoopBack team 👋🏻 Thank you for starting this discussion!

In LoopBack, we have been using Mocha with nyc for years, there are no major issues there. (I think the code runs a bit slower because of nyc/istanbul instrumentation, but it's not too bad.)

We have been looking into the options to replace nyc with built-in code coverage provided by v8/Node.js, see https://github.com/strongloop/loopback-next/issues/1871 and https://github.com/strongloop/loopback-next/pull/4688. It looks like c8 is adding a lot of overhead (~15-30 seconds) on Node.js 10.x, therefore we decided to put the migration on hold for now.

As mentioned by @boneskull, my main interest is in using code coverage to decide which tests to execute after a production code file has been changed.

A watch mode with incremental tests should start with just the dependency graph knowledge based on the files.

I think this approach is not going to work for monorepos. Consider the following case in a lerna-managed monorepo with two packages packages/core and packages/app:

core:

• packages/core/lib/foo.js exports function foo()
• packages/core/lib/bar.js exports function bar()
• packages/core/index.js re-exports everything from lib/foo.js and lib/bar.js
• packages/core/test/foo.test.js imports from ../lib/foo
• packages/core/test/bar.test.js imports from ../lib/bar

app:

• packages/app/lib/foo.controller.js imports foo via const {foo} = require('core')
• packages/app/lib/bar.controller.js imports bar via const {bar} = require('core')
• packages/app/test/foo.controller.test.js imports ../lib/foo.controller.js
• packages/app/test/bar.controller.test.js imports ../lib/bar.controller.js

When I make a change in core/lib/foo.js, I would like Mocha to run only the tests in foo.controller.test.js. However, because all app is importing from core, it's not able to differentiate between individual core/lib files and will have to run all tests in app as a result.

This problem is not specific to monorepos only. Consider the case where a directory contains index.js file to re-export all files from that directory, and a file in a different directory that imports things via the index.

• lib/models/product.model.js exports class Product
• lib/models/category.model.js exports class Category
• lib/models/index.js exports all models
• lib/controllers/product.controller.js calls const {Product} = require('../models')

Potentially that could be further improved with extending that knowledge into es module import/export pairs.

I think this would be a great improvement that should solve the problematic use cases I described above. However, how are we going to detect import/export pairs in CommonJS files (most of the npm ecosystem has not adopted ES6 modules yet)? It may also require special support for TypeScript and other transpiled languages (related: #4351).

The problem when you go beyond the file level is always figuring out if there are side effects that need to be taken into account.

That's a valid concern.

To me, it's ok if the watch mode sometimes does not detect all dependencies and "forgets" to run tests that are affected by the changed code. I can always explicitly tell Mocha to re-run all tests and if I forget, then there is CI to catch that.

I also suspect that in many cases, such side effects will be caused incorrectly structured code (see spooky action at a distance), so once can argue it's a problem in the tested application. However, I understand that when a user encounters the issue, they will likely open a GitHub issue and thus put even more pressure on Mocha maintainers, so I will understand if you decide to not implement coverage-based dependency detection that's not reliable.

[boneskull]

I've looked at node-tap's implementation, and it seems the information we need is generated by nyc and placed into a JSON file, which we can then read to determine which tests need to be re-run when a file changes. If it's really as simple as that, we could enable this "minimal re-run" behavior when a user runs nyc mocha with --watch just by checking for the existence of the file. Mocha would not need to have nyc in its dependencies, and the "re-run everything" behavior would be the default if the file did not exist.

[boneskull]

...and we could add an option (e.g., --changed) which would check this file and only re-run affected tests--no watch mode necessary.

[bajtos]

I like your proposal, @boneskull, it looks like a great improvement requiring not too much effort to implement (hopefully 🤞🏻 ). I think it should work well with TypeScript in the watch mode, because the developer is in control of the timing - when should Mocha start to look for changed files to decide which tests to (re)run.

It makes me wonder, would this feature allow incremental tests on CI too? I am thinking about caching the mocha/nyc files used to compute which files were already tested, so that pull request builds can compile & test only files that have been changed (or affected by changes) when compared to master (the value stored in the cache). This is a nice to have feature for longer term, but we may want to design the filesystem layout to make caching easy. For example, TypeScript is creating a new .tsbuildinfo file for each (sub)project, which makes caching on Travis CI a bit cumbersome, considering it's directory-based cache config (see https://docs.travis-ci.com/user/caching/#caching-directories-bundler-dependencies).

[boneskull]

--changed would theoretically work w CI, assuming you cache the cache file 🙂

[boneskull]

this is not going to be easy

[JoshuaKGoldberg]

Related: #4352

[JoshuaKGoldberg]

Marking as accepting PRs. But emphasizing https://github.com/mochajs/mocha/issues/4350#issuecomment-668721125: this is not going to be easy. It's a big feature ask. I'd strongly suggest anybody looking at this issue to get through multiple other issues first before trying this. 🙂