angular / angular-cli

CLI tool for Angular
https://cli.angular.io
MIT License
26.76k stars 11.98k forks source link

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

Open westonpace opened 8 years ago

westonpace commented 8 years ago

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

jtesser commented 8 years ago

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

filipesilva commented 7 years ago

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.

jtesser commented 7 years ago

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

westonpace commented 7 years ago

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:

westonpace commented 7 years ago

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.

filipesilva commented 7 years ago

@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.

seeruk commented 7 years ago

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.

blaugold commented 7 years ago

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.

seeruk commented 7 years ago

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.

rashmi-dey commented 7 years ago

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.

arusakov commented 7 years ago

@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).

kpaxton commented 7 years ago

@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.

arusakov commented 7 years ago

@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.

kpaxton commented 7 years ago

@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.

vpassapera commented 7 years ago

Got this to work with

const context = require.context('./', true, /\/app\/.*\.ts$/);
just-jeb commented 6 years ago

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.

kpaxton commented 6 years ago

@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.

just-jeb commented 6 years ago

@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 commented 6 years ago

@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.

jeanpaulattard commented 6 years ago

Is there any updates regarding this.

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

demisx commented 5 years ago

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

just-jeb commented 5 years ago

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

CraigComeOnThisNameCantBeTaken commented 5 years ago

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

CraigComeOnThisNameCantBeTaken commented 4 years ago

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.

johnthagen commented 4 years ago

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

chucknelson commented 2 months ago

Has there been any progress or changes in this issue over the past few Angular versions?

Seems like the two primary options to force coverage reporting on all sources, regardless of if they have a .spec file, are:

  1. Add an app.module.spec.ts file that just imports app.module.ts.
  2. Use https://github.com/kopach/karma-sabarivka-reporter

Is that still the situation today?