fix: throws when pitching loader is used

[nkbt]

Oh, this issue is still here...

I'm sorry not to respond in time, but @sokra - you were completely right. The only issue, that your solution does not work very well for instrumenter. It generates crazy filenames and file tree structure becomes totally broken.

Our solution was to always require files in the same manner and do not use different transformations on the same file in the project (including tests). Which worked quite well until I hit the issue again today.

My particular problem now was that in one case module is required as is (in one legacy test) and in the other case - with inject-loader. I debugged this loader and found that the same file is included twice, but the code is different (this.request is obviously different for them either: one has inject-loader and the other one does not). The difference in code is minimal, but enough to give bunch of errors with istanbul report.

I came up with two solutions at the moment:

1. Update legacy testcase with proper dependency (with inject-loader). Completely legit and working solution with minimal effort
2. Update istanbul-instrumenter-loader itself to handle this issue.

Here is a PR which fixes the issue.

It is up to discussion if this solution is completely correct, but it solves the problem and works for us. I'd love to hear some comments on this and how this solution can potentially distort coverage report in case of some funky loaders being used.

The other thing that I am not sure if the solution is completely correct - coverage report generations in file watching mode. It seems like an edge case.

[sokra]

This will break incremental compilation. Loaders shouldn't keep state between runs: http://webpack.github.io/docs/how-to-write-a-loader.html#not-keep-state-between-runs-and-modules

[nkbt]

@sokra yep, that is exactly what I was talking about in the last paragraph. In case of coverage report that might not be critical (coverage report is usually run by CI or on dev machine without incremental updates), though the solution is definitely not nice.

I was also thinking about adding little hash to the filename in a way it will not break report structure. Downside of it - in coverage report you will end up with bunch of duplicates (almost duplicates), which will decrease (or increase) your overall coverage percentage and add unnecessary noise.

Any other thoughts?

[ghengeveld]

For the record (and hopefully to revive this PR). I'm using istanbul-instrumenter-loader with inject-loader (2.0.1) and karma-coverage (0.5.5). Istanbul would crash with the following stacktrace:

TypeError: Cannot read property 'start' of undefined
    at /Users/ghengeveld/.../node_modules/karma-coverage/node_modules/istanbul/lib/object-utils.js:59:44
    at Array.forEach (native)
    at Object.addDerivedInfoForFile (/Users/ghengeveld/.../node_modules/karma-coverage/node_modules/istanbul/lib/object-utils.js:58:37)
    at Object.Collector.fileCoverageFor (/Users/ghengeveld/.../node_modules/karma-coverage/node_modules/istanbul/lib/collector.js:94:15)
    at /Users/ghengeveld/.../node_modules/karma-coverage/node_modules/istanbul/lib/report/html.js:558:90
    at Array.forEach (native)
    at HtmlReport.Report.mix.writeReport (/Users/ghengeveld/.../node_modules/karma-coverage/node_modules/istanbul/lib/report/html.js:557:27)
    at writeReport (/Users/ghengeveld/.../node_modules/karma-coverage/lib/reporter.js:62:16)
    at /Users/ghengeveld/.../node_modules/karma-coverage/lib/reporter.js:290:11
    at FSReqWrap.oncomplete (fs.js:82:15)

I'm now using nkbt/istanbul-instrumenter-loader, which fixes the problem. I'd rather not have an npm dependency to a Github URL, so I'd love to see this PR merged.

[ghengeveld]

Update: Unfortunately, nkbt's version isn't working properly either. Because of the cache, if a dependency is required normally and then later on it's required again using the inject-loader (e.g. in a unit test), the result will be the original dependency instead of the injectable function. In other words, it won't do anything.

[nkbt]

The only really working solution we have found - always mock deps in tests
and never mix mocked/unmocked during the run. That is actually good for
tests due to proper components isolation, so you don't test them in
combination
On Thu, 7 Apr 2016 at 06:46, Gert Hengeveld notifications@github.com
wrote:

Update: Unfortunately, nkbt's version isn't working properly either.
Because of the cache, if a dependency is required normally and then later
on it's required again using the inject-loader (e.g. in a unit test), the
result will be the original dependency instead of the injectable function.

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#12 (comment)

[ghengeveld]

I'm not sure that's a solution for us. We use karma-webpack which bundles everything into one file (src+test) and passes that to the browser for testing. We're in the process of moving from Angular (hence Karma) to React and I'm looking to remove Karma from the testing stack (use JSDOM instead of PhantomJS). For now our 'solution' is to disable code coverage reporting.

[michael-ciniawsky]

@nkbt Is this still relevant, was there a solution found etc ? :)

[michael-ciniawsky]

Fixes #3

[nkbt]

No solution found except not using kerma/istanbul-loader/etc. we don't do these things anymore. Pure Tape + babel runtime = no issues

[michael-ciniawsky]

@nkbt kk since this doesn't seem to be a 'clean' fix, can I close this ? :)

[nkbt]

Yeah, I think so!