angular-cli icon indicating copy to clipboard operation
angular-cli copied to clipboard

Option for Istanbul coverage to include all source files (as opposed to only files loaded during test)

Open westonpace opened this issue 7 years ago • 25 comments

Please provide us with the following information:

  1. OS? Windows 7, 8 or 10. Linux (which distribution). Mac OSX (Yosemite? El Capitan?) Any
  2. Versions. Please run ng --version. If there's nothing outputted, please run in a Terminal: node --version and paste the result here:

angular-cli: 1.0.0-beta.11-webpack.2 node: 6.2.2 os: linux x64

  1. Repro steps. Was this an app that wasn't created using the CLI? What change did you do on your code? etc.

Create an application. Test some, but not all of the source files. Generated coverage will not include files that were not loaded by the tests.

  1. The log given by the failure. Normally this include a stack trace and some more information.

N/A

  1. Mention any other details that might be useful.

Thanks! We'll be in touch soon.

Similar to gotwarlost/istanbul#275 and karma-runner/karma-coverage#125

westonpace avatar Aug 17 '16 21:08 westonpace

+1 karma-coverage has an option includeAllSources which could be used.

jtesser avatar Oct 20 '16 20:10 jtesser

If anyone has is willing to do a PR, or point me in the right direction to do it with webpack/karma-remap-istanbul, I'd be happy to look at it.

filipesilva avatar Oct 24 '16 01:10 filipesilva

@filipesilva what exactly are you asking for I am confused.

jtesser avatar Oct 26 '16 20:10 jtesser

Given that I understand webpack a little better now the includeAllSources option in karma would not work. Under the hood that is including all files that were instrumented. Webpack won't instrument code that isn't loaded so includeAllSources would not see it.

Instead I ended up using a simpler workaround. I added a single file:

src/app.module.spec.ts

which is simply (e.g. no test suites or cases):

import './app.module';

This file will cause webpack to load the entire application. The istanbul coverage will only be generated for local code (since node_modules is excluded). As far as I can tell this is a pretty ideal solution with 2 caveats:

  • It is still possible for code to be missed in coverage if that code is not included in the application at all. One could potentially mitigate this with a webpack plugin that checked for unused code in ./src and reported it.
  • It will cause an increase in test times depending on the size of the application.

westonpace avatar Oct 27 '16 08:10 westonpace

Let me know if you want me to make a pull request with this particular solution (or go ahead and implement it as it is simple enough). I wasn't sure creating a file named app.module.spec.ts would be universally accepted (some might be confused by its presence) and was hoping that someone might have a better, more invisible, idea on how to accomplish the same thing.

westonpace avatar Oct 27 '16 09:10 westonpace

@westonpace I see how your solution works. It's something that's worth discussing before being implemented though...

A more direct use of your solution would be to add import './app.module'; to src/test.ts instead.

I think the biggest drawback is now that the whole app is indeed loaded on every test, whereas we only realistically want it for code coverage tests.

Not sure of what the best approach is for now, but at least we have a workaround. Let's let this issue sit open for a while, see what people say.

filipesilva avatar Nov 02 '16 19:11 filipesilva

Another option may be to modify the following line in test.ts to require everything:

.then(() => require.context('./', true, /\.spec\.ts/))
// Becomes
.then(() => require.context('./app', true, /\.ts/))

If you use lazy loaded modules importing AppModule won't include everything.

It seems like the main downside with showing coverage for everything is that you may have files that can't be covered, or don't make sense to be covered that will be included in the report, skewing your numbers a little.

seeruk avatar Nov 04 '16 14:11 seeruk

I'm successfully using

.then(() => require.context('./', true, /\/app\/.*\.ts$/))

in src/test.ts. @SeerUK's solution seems to trigger a weback bug in the context loader where files are loaded as directories. Putting the 'app' path part in the regex works.

blaugold avatar Nov 07 '16 23:11 blaugold

Aah, perhaps it needed a slash on the end of the path. I'd not actually tried that solution with angular-cli, I'm just doing something similar in the boilerplate I've been making.

seeruk avatar Nov 07 '16 23:11 seeruk

I am not using angular-cli. Is there an option to include all source files in coverage while using karma-test-shim.js to load systemjs configuration and all test files.

rashmi-dey avatar Mar 02 '17 22:03 rashmi-dey

@filipesilva

I want to suggest separated file coverage.ts near test.ts and coverage option in .angular-cli.json. In coverage.ts we can do all stuff related to full code coverage and developers can configure what file use for coverage via coverage option (use "coverage": "coverage.ts" - for full coverage, and "coverage": "test.ts" keeps current behaviour).

arusakov avatar Mar 25 '17 15:03 arusakov

@arusakov I have implemented something similar to what you are suggesting in my own application. The downfall to this is that I then have two test start points that need to stay in sync when things might be changed. I implemented a coverage.ts in the same folder as test.ts and updated the karma-conf.js to actually change the files array and preprocessors object to reflect the new start point if config.angularCli.codeCoverage is true. It works pretty well, but having a flag available to test.ts to allow for switching the context based on that flag I think would be slightly more ideal than having two separate files.

kpaxton avatar May 16 '17 14:05 kpaxton

@kpaxton Thank you for response. Maybe. But different starting points is more flexible. Also in my case coverage.ts can import test.ts, so sync problem is not a problem.

arusakov avatar May 16 '17 15:05 arusakov

@arusakov, @filipesilva I recently upgrade our cli version to 1.1.3 and the method I had previously in place to get this to work (see above) is now no longer working. I am getting an Unexpected token import error when trying to run coverage against my coverage.ts file with the expanded context. Is there a new workaround that I need to put in place since the cli version upgrade? Looks like the test.js default webpack config only takes the test.ts file set up in the cliConfig.json. I can no longer manually modify the karma.conf to take what I want to pass in as the test start point.

kpaxton avatar Jun 26 '17 14:06 kpaxton

Got this to work with

const context = require.context('./', true, /\/app\/.*\.ts$/);

vpassapera avatar Jul 23 '17 11:07 vpassapera

The problem with requiring all the .ts files in test.ts is that it will slow down the tests even when run without coverage (it will still load all the files although it doesn't have to). Here is the way to include all the sources when the -cc option has been specified and stay with .spec.ts files when it was not.

@kpaxton I think this solution does exactly what you wanted in your comment:

It works pretty well, but having a flag available to test.ts to allow for switching the context based on that flag I think would be slightly more ideal than having two separate files.

just-jeb avatar Nov 26 '17 11:11 just-jeb

@meltedspark yes, that worked for a little while. I just upgraded to the CLI 1.6.1 and it no longer works. I get a bunch of Warnings that files weren't included in compilation. So I guess all workarounds that I previously had to get this to work have been removed and/or prevented as we've updated.

kpaxton avatar Dec 20 '17 12:12 kpaxton

@kpaxton right, not working after upgrade. Looking for solution. I believe the problem is in AngularCompilerPlugin which probably looks now only on tsconfig.json and disregards the context. I bet if you change

  "include": [
    "**/*.spec.ts"
  ]

to

  "include": [
    "**/*.ts"
  ]

it will work

In that case the best way is probably using two tsconfig.spec.json files - one for coverage and second for just tests and choosing the appropriate according to the parameter.

just-jeb avatar Dec 24 '17 09:12 just-jeb

@kpaxton well, I've implemented another workaround (see updated SO answer). In short, you should add another tsconfig which includes all the files, add another app (which uses this tsconfig) to apps array in .angular-cli.json, and use this config in karma.conf.js according to the coverage parameter.

just-jeb avatar Dec 24 '17 15:12 just-jeb

Is there any updates regarding this.

@meltedspark I couldn't get your workaround to work with v6

jeanpaulattard avatar Jun 20 '18 09:06 jeanpaulattard

Can't get any workarounds to work with NG7/NgRx. The tests simply time out.

demisx avatar Mar 27 '19 01:03 demisx

@jeanpaulattard @demisx Yeah, Angular CLI has changed quite a lot so the workaround won't work. I personally ended up switching to Jest.

just-jeb avatar Mar 27 '19 06:03 just-jeb

I have been unable to implement any of the given work-arounds. Is there any news on this please?

Since this is still open, I intend to solve this issue in my project by creating a schematic that adds tests where theyre missing, I found that even if the spec files are empty as long as they import the file it seems to generate correct coverage.

I've had good luck using the karma-sabarivka-reporter plugin to pick up missed source files into coverage.

johnthagen avatar Feb 23 '20 20:02 johnthagen