fix: coverage yields empty reports #246

mblarsen · 2017-10-03T10:06:41Z

Fix for #245

The coverage preprocessor was never added when enabling coverage. It was only added as reporter.

UPDATE: After submitting the first PR the code has gone in a different direction after reading hundreds of githubs issues, PRs, boilerplates, etc.

coverage should not be a preprocessor and also you don't have to add your src files to files it is enough to include the test files.

mblarsen · 2017-10-03T10:48:27Z

Although this fixes the issue there are some remaining issues:

Everything in files will be covered.
You'll need to add your source files to files as well. Not just the tests.
The coverage is of the webpack'ed files. Not the source files. I'm wondering if this https://github.com/webpack-contrib/istanbul-instrumenter-loader should be used.

For 1-2 I suggest explicitly telling what to cover:

   coverage: true,
   coverFiles: [ 'src/*.js' ],

And then use that to concat to files and use directly in preprocessors instead of reducing over files.

(this is a breaking change of course :/ )

These should probably be split out into new issues.

mblarsen · 2017-10-04T02:25:41Z

@egoist I'll examine this further. It shouldn't be necessary to specify the source files, so something else must be wrong.

Please ignore this PR for now :)

mblarsen · 2017-10-07T12:47:43Z

Maaaan, this took some time to get right. There are so many test/coverage/transpiler modules out there that are influx. This is not the time for coverage of es6 with tests written in es6 hehe.

But it works now:

A key part of getting it to work was to use an old version of istanbul-instrumenter-loader as the never versions doesn't work with html and lcov reporters (only lcovonly reporter).

egoist · 2017-10-07T13:16:24Z

does this work on .vue files?

mblarsen · 2017-10-07T13:21:13Z

Not sure, will test.

mblarsen · 2017-10-07T13:34:20Z

It does work, however it will includes other files as well -- used to transpile vue.

mblarsen · 2017-10-07T13:35:08Z

Not ideal, will try to get rid of those.

mblarsen · 2017-10-07T13:37:17Z

I think I might need to do something like this vuejs/vue-loader#7 (comment) to get it working correctly with vue.

mblarsen · 2017-10-07T13:57:16Z

Also this may be relevant: webpack-contrib/istanbul-instrumenter-loader#73

mblarsen · 2017-10-07T14:01:14Z

Btw, for context coverage: true doesn't work for anything prior to this PR. But I think we should get vue (and possible and others) to work also before merging anything.

mblarsen · 2017-10-09T06:12:15Z

@egoist I got code coverage working on both regular js and on vue. Also I sorted out the excess files included in the reports to that the coverage values are now accurate.

There is one part of the solution I'm not particular happy with and I was wondering if there are other ways to modify and existing loader than the way I am doing. (the mapping of rules part)

js:

vue:

It is important to note that this is version 0.2.0 of the module. Versions after that does not support the html and lcov reporters (lcovonly is supported).

egoist · 2017-10-09T13:12:21Z

packages/poi-preset-karma/index.js

+          }
+        ].concat(webpackConfig.module.rules)
+        /* for vue (assumes vue-loader) */
+        webpackConfig.module.rules = webpackConfig.module.rules


actually you can update webpack config in poi.extendWebpack('test', config => {}) using webpack-chain

I will update this :)

Thanks. I'm new to webpack-chain will check it out. It looks promising.

egoist · 2017-10-09T14:18:02Z

packages/poi-preset-karma/index.js

+            const options = r.use[vueLoaderPos].options
+            const instrumenterLoader = 'istanbul-instrumenter-loader?esModules=true'
+            options.preLoaders = (options.preLoaders || {})
+            options.preLoaders.js = typeof options.preLoaders.js === 'string' ?


why it's preLoader for .vue file but postLoader for .js file?

Actually it should be pre for both. If you use post it will run the coverage after the babel-plugin-add-module-exports has run which adds something like this to the top:

Do you want me to change both to pre (as they should be) or can you correct it when you add the webpack-chain stuff @egoist

thanks I will webpack-chain-ify it later today

mblarsen force-pushed the fix-245 branch from 1aaa07d to d418021 Compare October 7, 2017 12:44

mblarsen force-pushed the fix-245 branch 2 times, most recently from 0b71a8c to 2ef1ea6 Compare October 9, 2017 06:11

mblarsen changed the title ~~fix: coverage was never added as preprocessors. Fixes #245~~ fix: coverage yields empty reports. Oct 9, 2017

mblarsen changed the title ~~fix: coverage yields empty reports.~~ fix: coverage yields empty reports Oct 9, 2017

fix: make use of istanbul-instrumenter-loader to get correct coverage

e400c2f

It is important to note that this is version 0.2.0 of the module. Versions after that does not support the html and lcov reporters (lcovonly is supported).

mblarsen force-pushed the fix-245 branch from 9275b1f to e400c2f Compare October 9, 2017 07:08

Merge branch 'master' into fix-245

2025db9

egoist reviewed Oct 9, 2017

View reviewed changes

mblarsen and others added 3 commits October 10, 2017 14:19

fix: istanbul-instrumenter-loader for JS enforces pre not post

26681a2

refactor to use webpack-chain

1071f21

tweaks

e86d6c9

egoist merged commit 6309119 into egoist:master Oct 12, 2017

mblarsen deleted the fix-245 branch October 12, 2017 14:25

mblarsen restored the fix-245 branch October 17, 2017 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: coverage yields empty reports #246

fix: coverage yields empty reports #246

mblarsen commented Oct 3, 2017 •

edited

mblarsen commented Oct 3, 2017 •

edited

mblarsen commented Oct 4, 2017

mblarsen commented Oct 7, 2017 •

edited

egoist commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 9, 2017

egoist Oct 9, 2017

egoist Oct 9, 2017

mblarsen Oct 10, 2017

egoist Oct 9, 2017

mblarsen Oct 10, 2017

egoist Oct 10, 2017

fix: coverage yields empty reports #246

fix: coverage yields empty reports #246

Conversation

mblarsen commented Oct 3, 2017 • edited

mblarsen commented Oct 3, 2017 • edited

mblarsen commented Oct 4, 2017

mblarsen commented Oct 7, 2017 • edited

egoist commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 7, 2017

mblarsen commented Oct 9, 2017

egoist Oct 9, 2017

Choose a reason for hiding this comment

egoist Oct 9, 2017

Choose a reason for hiding this comment

mblarsen Oct 10, 2017

Choose a reason for hiding this comment

egoist Oct 9, 2017

Choose a reason for hiding this comment

mblarsen Oct 10, 2017

Choose a reason for hiding this comment

egoist Oct 10, 2017

Choose a reason for hiding this comment

mblarsen commented Oct 3, 2017 •

edited

mblarsen commented Oct 3, 2017 •

edited

mblarsen commented Oct 7, 2017 •

edited