feat (CodeCoverage): use karma-remap-istanbul #1468

deebloo · 2016-07-27T20:49:26Z

use karma-remap-istanbul to map code coverage back to source typescript file

deebloo · 2016-07-27T21:08:15Z

@Brocco I am seeing the CI builds have been running for like 20 min. I don't believe I would have done anything here to cause this?

use karma-remap-istanbul to map code coverage back to source typescript file (+1 squashed commit) Squashed commits: [b444648] remove trailing comma feat (CodeCoverage): use karma-remap-istanbul use karma-remap-istanbul to map code coverage back to source typescript file

…ular-cli into karma-remap-istanbul

filipesilva · 2016-07-27T22:44:36Z

Heya! Sometimes the builds just take a while. In travis some show has having errored though.

I'm not too sure of the purpose of this PR though, can you tell me what's the issue you're trying to solve?

deebloo · 2016-07-27T22:58:33Z

Sure! So currently. The code coverage report displays the compiled js files and not the source ts files. This should fix that

filipesilva · 2016-07-27T23:07:11Z

Ok I understand it now. Yeap, that needs fixing, thank you for taking it up. Me and @TheLarkInn should be reviewing when the CI is stable, since we worked on the current implementation.

deebloo · 2016-07-27T23:08:36Z

👍

delasteve · 2016-07-28T12:04:49Z

As a maintainer (sure?) of karma-remap-istanbul, I would say this is not the way to go, currently. I got promoted to maintainer because I had a PR out and the owner couldn't find the time to merge my PR in.

I've had issues with the plugin, but don't know enough about karma plugins to be considered a maintainer. If someone would like to take a look at it, by all means, please do. There are two PRs on the repo that use karma-coverage as a base. I like the implementation, but there are still concerns it's not all the way correct.

@devCrossNet was working on a fix that created a task that used remap-istanbul instead of using the flaky plugin. I still recommend this is the way to go, since the current implementation of karma-remap-istanbul needs work.

Edit: Relevant PR - #895

devCrossNet · 2016-07-28T12:20:57Z

@delasteve Hello, it's me ... ;-) I could update my coverage branch to keep my implementation up to date with the new webpack stuff. But I am still waiting for the remap-istanbul release.

Or should I stop working on this feature?

devCrossNet · 2016-07-28T13:25:05Z

I had a short look at the new implementation. the test process changed and there is some new kind of karma plugin. I need some time to dive into the Code and get everything working again.

filipesilva · 2016-07-28T13:27:57Z

The new karma plugin was added to have the whole build process for tests managed via karma. I made it but I wouldn't say I'm very knowledgeable of karma plugins, it's just editing the config really.

filipesilva · 2016-07-28T13:29:50Z

@deebloo tests seem to be failing a lot with this PR, I can restart the build but the instability seems due the the changes introduced in here.

TheLarkInn · 2016-07-28T13:44:59Z

addon/ng2/blueprints/ng2/files/config/karma.conf.js

@@ -22,10 +23,34 @@ module.exports = function (config) {
      { pattern: './src/test.ts', watched: false }
    ],
    preprocessors: {
+      './src/**/**/*.ts': ['angular-cli-coverage'],


Would you be willing to explain this?

I added a new preprocessor in the karma plugin. While there is a single entry point for the tests in order to get the remap plugin to generate the mapped coverage report it needs to run over the test files themselves.

devCrossNet · 2016-07-28T13:46:45Z

@filipesilva I understand that code. It is not much,I had trouble with the source maps and the processing of code coverage. But I tried it just a few minutes. I'll come back to you when I can't get it working again after a few hours.

deebloo · 2016-07-28T13:57:40Z

@filipesilva I am running my local tests again to see if I can spot what is wrong. Also, based on the conversation here do we want to try NOT using the remap karma plugin?

TheLarkInn · 2016-07-28T14:01:25Z

cc @bryanforbes do you think you could lend your coverage and instrumentation expertise. Id love to make coverage as accurate as possible and you've worked on this extensively also.

filipesilva · 2016-07-28T14:01:53Z

@deebloo I can't say one way or the other since I don't fully understand the question that is being brought up, but I have a considerable amount of respect for @delasteve and @devCrossNet since they put a lot of work into #895 (which I've closed when changing to webpack) so I'd like to understand what a better approach would be.

deebloo · 2016-07-28T14:03:48Z

@filipesilva fair. I will at least try to fix what I have so that this PR doesn't fail the tests, but after that I turn it over to the more experienced folks in the group

deebloo · 2016-08-01T01:13:29Z

update. so the tests just seem to hang the second time the e2e tests run the unit tests a second time. I can't find exactly why that is. It may be an issue with the karam plugin for remap. If that is the case (and based on other feed back in this feed). maybe I should just close this PR for now and let some of the other people who had been working on it take over. Thoughts?

devCrossNet · 2016-08-01T07:35:16Z

I evaluated the code yesterday and I think my approach is not going to work with the new webpack code (there are no files after the karma run, so I can not access the sourcemap files). I'll figure out another solution during the week. maybe the way would be a self-written, CLI optimized karma plugin-in.

bryanforbes · 2016-08-01T15:35:03Z

There are a couple of issues at play that should probably be addressed for more accurate code coverage:

karma-coverage currently outputs the "preamble" for coverage numbers into the top of each module. What this means is that each module that coverage is needed for must be evaluated even if that module doesn't have tests run on it. For instance, let's say we have modules a.js, b.js, and c.js. We write tests for a.js and b.js, but not for c.js. But in order to get c.js into the coverage report, one has to loop over all of the modules in a context and evaluate them (const modules = require.context("../app", true); modules.keys().forEach(modules);). However this presents a problem because c.js should never be evaluated and should have 0% coverage; with the context loop, any module setup code will be evaluated and added to the coverage report as executed statements which will not give an accurate coverage number. Instead, the full preamble of all modules (or all pertinent modules) should be output to the bootstrap chunk and a piece of code to set up the variable that istanbul uses in the module should be output to the top of each module. I currently have a plugin to do just that, but I haven't had the time to submit it to karma-coverage as an enhancement.
karma-remap-istanbul currently watches for a JSON output by karma-coverage which can lead to unnecessary errors during testing or to hangs while it waits for a file or both. I'm currently using a simple plugin that takes the coverage property off the results passed to onBrowserComplete by karma and runs it though remap-istanbul via the programmatic API. This also eliminates the need for karma-coverage to output an intermediate JSON file for karma-remap-istanbul to pick up and users will end up with just the coverage reports they specify (eliminating potential confusion).
The configuration passed to awesome-typescript-loader should not include any source map settings. Instead, the devtool property should be set to inline-source-map which will let webpack properly set up the source maps so remap-istanbul can read them.

I would be glad to submit PRs to both karma-coverage and karma-remap-istanbul to fix the first two issues, but I thought I'd weigh in on this at the request of @TheLarkInn. Let me know if there's any other way I can help.

delasteve · 2016-08-01T18:20:23Z

I merged in @bryanforbes's PR for karma-remap-istanbul and released it as 0.2.0. Assuming this fixes everything with the karma plugin, it should take a quick update to get this in.

deebloo · 2016-08-01T19:43:31Z

I keep getting. Uncaught TypeError: Cannot read property 'f' of undefined with the new plugin

bryanforbes · 2016-08-01T19:45:02Z

@deebloo What version of the plugin? @delasteve just released 0.2.1 which fixes a dumb mistake I made.

deebloo · 2016-08-01T19:50:55Z

@bryanforbes I am using 2.1. still getting that weird f error

deebloo · 2016-08-01T22:10:33Z

@bryanforbes I am getting this error now. Not sure if its my setup or the plugin. I can push my changes to the branch if you would like to see.

01 08 2016 18:09:19.292:ERROR [karma]: Error: Unable to find entry for [src/test.ts]
    at MemoryStore.Store.mix.get (/Users/dannyblue/Documents/projects/foo/node_modules/istanbul/lib/store/memory.js:38:19)
    at HtmlReport.Report.mix.writeDetailPage (/Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/report/html.js:411:67)
    at /Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/report/html.js:489:26
    at SyncFileWriter.extend.writeFile (/Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/util/file-writer.js:57:9)
    at FileWriter.extend.writeFile (/Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/util/file-writer.js:147:23)
    at /Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/report/html.js:488:24
    at Array.forEach (native)
    at HtmlReport.Report.mix.writeFiles (/Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/report/html.js:482:23)
    at /Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/report/html.js:484:22
    at Array.forEach (native)
    at HtmlReport.Report.mix.writeFiles (/Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/report/html.js:482:23)
    at HtmlReport.Report.mix.writeReport (/Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/node_modules/istanbul/lib/report/html.js:566:14)
    at /Users/dannyblue/Documents/projects/foo/node_modules/remap-istanbul/lib/writeReport.js:77:22
    at /Users/dannyblue/Documents/projects/foo/node_modules/amdefine/amdefine.js:125:34
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickCallback (internal/process/next_tick.js:98:9)

bryanforbes · 2016-08-01T23:16:43Z

I believe you'll need to pass the force-sourcemap property to sourcemap-istanbul-instrumenter-loader in the webpack config in order for that loader to produce a source map from the instrumented code to the original source:

        {
          test: /\.(js|ts)$/, loader: 'sourcemap-istanbul-instrumenter-loader',
          exclude: [
            /\.(e2e|spec)\.ts$/,
            /node_modules/
          ],
          query: { 'force-sourcemap': true }
        }

I'm in the process of trying to get a similar change made in istanbul-instrumenter-loader. If the author is unresponsive, I'll publish a loader I'm currently using which addresses point 1 above as well as source maps.

# Conflicts: # addon/ng2/blueprints/ng2/files/config/karma.conf.js # addon/ng2/blueprints/ng2/files/package.json # addon/ng2/models/webpack-build-test.js # plugins/karma.js

deebloo · 2016-08-02T01:13:44Z

should be working and passing all tests now.

delasteve · 2016-08-02T12:51:35Z

Thank you, @deebloo, @TheLarkInn, @bryanforbes, and @devCrossNet, for all the hard work you put into this.

👍 🎉

TheLarkInn · 2016-08-02T13:00:54Z

@deebloo would you be willing to add one or two tests in our e2e workflow that can test this feature.

This will allow us to safeguard this featrue against future changes. Otherwise @filipesilva LGTM.

deebloo · 2016-08-02T13:49:31Z

@TheLarkInn Ill add one after all of the "test tests" run to make sure the correct folders exist

delasteve · 2016-08-02T14:02:20Z

@deebloo This might help you. It's from my PR when I was implementing this.

https://github.com/angular/angular-cli/pull/895/files#diff-233c7c3510139cc00a622f09ce37097bR199

bryanforbes · 2016-08-02T15:46:17Z

addon/ng2/blueprints/ng2/files/config/karma.conf.js

+      reporters: [
+        { type : 'json', subdir: '.', file: 'coverage-final.json'}
+      ]
+    },


Is there a reason to keep using karma-coverage? karma-remap-istanbul produces the coverage report and no longer needs the intermediate file and instrumenting is done by sourcemap-istanbul-instrumenter-loader.

you are correct. I misunderstood the docs for the karma plugin

TheLarkInn · 2016-08-04T22:07:18Z

Thank you everyone for your work.

filipesilva · 2016-08-04T22:07:41Z

Just merged it in. A big thanks to @delasteve, @devCrossNet, @deebloo and @bryanforbes for making this work! It's been long in the backburner and the final solution is very elegant and clean imho.

deebloo · 2016-08-04T22:13:08Z

happy to help! you all are killing it with this newest update. (you know. in a good way).

angular-automatic-lock-bot · 2019-09-10T02:41:26Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

feat (CodeCoverage): use karma-remap-istanbul

803fa36

use karma-remap-istanbul to map code coverage back to source typescript file

googlebot added the cla: yes label Jul 27, 2016

Danny Blue added 3 commits July 27, 2016 17:52

remove trailing comma

b444648

Merge branch 'karma-remap-istanbul' of https://github.com/deebloo/ang…

4c49c59

…ular-cli into karma-remap-istanbul

Merge branch 'master' into karma-remap-istanbul

ce175e3

filipesilva added the state: WIP label Jul 28, 2016

TheLarkInn reviewed Jul 28, 2016
View reviewed changes

bryanforbes mentioned this pull request Aug 1, 2016

Remap coverage directly from coverage property instead of using a file watcher marcules/karma-remap-istanbul#17

Merged

Merge branch 'master' into karma-remap-istanbul

40c5b88

Danny Blue added 2 commits August 1, 2016 20:47

update karma-remap-istanbul

cdebee1

Merge branch 'karma-remap-istanbul-3' into karma-remap-istanbul

b0d7618

# Conflicts: # addon/ng2/blueprints/ng2/files/config/karma.conf.js # addon/ng2/blueprints/ng2/files/package.json # addon/ng2/models/webpack-build-test.js # plugins/karma.js

delasteve mentioned this pull request Aug 2, 2016

chore(docs): add webpack update instructions #1456

Merged

2 tasks

Danny Blue added 2 commits August 2, 2016 10:18

chore: add unit tests for coverage report

b460f24

chore: remove trailing comma

66ff31f

bryanforbes reviewed Aug 2, 2016
View reviewed changes

Danny Blue added 3 commits August 2, 2016 12:26

chore: remove dependency on karma-coverage

7297a13

Merge branch 'master' into karma-remap-istanbul

3ed174b

Merge branch 'master' into karma-remap-istanbul

5fae7f4

filipesilva added pr_state: LGTM and removed state: WIP labels Aug 4, 2016

filipesilva merged commit 36c0ee5 into angular:master Aug 4, 2016

deebloo deleted the karma-remap-istanbul branch August 4, 2016 22:18

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 10, 2019

feat (CodeCoverage): use karma-remap-istanbul #1468

feat (CodeCoverage): use karma-remap-istanbul #1468

Conversation

deebloo commented Jul 27, 2016

deebloo commented Jul 27, 2016

filipesilva commented Jul 27, 2016

deebloo commented Jul 27, 2016

filipesilva commented Jul 27, 2016

deebloo commented Jul 27, 2016

delasteve commented Jul 28, 2016 • edited Loading

devCrossNet commented Jul 28, 2016

devCrossNet commented Jul 28, 2016

filipesilva commented Jul 28, 2016

filipesilva commented Jul 28, 2016 • edited Loading

TheLarkInn Jul 28, 2016

Choose a reason for hiding this comment

deebloo Jul 28, 2016

Choose a reason for hiding this comment

devCrossNet commented Jul 28, 2016

deebloo commented Jul 28, 2016

TheLarkInn commented Jul 28, 2016

filipesilva commented Jul 28, 2016 • edited Loading

deebloo commented Jul 28, 2016

deebloo commented Aug 1, 2016

devCrossNet commented Aug 1, 2016

bryanforbes commented Aug 1, 2016

delasteve commented Aug 1, 2016

deebloo commented Aug 1, 2016

bryanforbes commented Aug 1, 2016

deebloo commented Aug 1, 2016

deebloo commented Aug 1, 2016 • edited Loading

bryanforbes commented Aug 1, 2016 • edited Loading

deebloo commented Aug 2, 2016

delasteve commented Aug 2, 2016

TheLarkInn commented Aug 2, 2016

deebloo commented Aug 2, 2016

delasteve commented Aug 2, 2016

bryanforbes Aug 2, 2016

Choose a reason for hiding this comment

deebloo Aug 2, 2016

Choose a reason for hiding this comment

TheLarkInn commented Aug 4, 2016

filipesilva commented Aug 4, 2016

deebloo commented Aug 4, 2016

angular-automatic-lock-bot bot commented Sep 10, 2019

delasteve commented Jul 28, 2016 •

edited

Loading

filipesilva commented Jul 28, 2016 •

edited

Loading

filipesilva commented Jul 28, 2016 •

edited

Loading

deebloo commented Aug 1, 2016 •

edited

Loading

bryanforbes commented Aug 1, 2016 •

edited

Loading