Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(preprocessor): use absolute paths #163

Merged

Conversation

jamestalmage
Copy link
Contributor

According to the spec, the absolute file path should be used in the coverage report. Currently karma-coverage substitutes this with a path relative to baseDir. This causes problems when trying to combine coverage with other reports not generated by karma-coverage.

Fixing this would help with #1.

According to the spec, the absolute file path should be used in the
coverage report. Currently karma-coverage substitutes this with a
path relative to `baseDir`. This causes problems when trying to
combine coverage with other reports not generated by karma-coverage.
Fixing this would help with karma-runner#1.

coverag.json spec for reference:
 - https://github.com/gotwarlost/istanbul/blob/master/coverage.json.md
@tswaters
Copy link
Contributor

tswaters commented Aug 4, 2015

FWIW - in windows, istanbul outputs paths that look like, c:\\path\\etc\\ -- with this change, the paths generated by karma-coverage look like C:/path/etc/

Suggest using var jsPath = require.resolve(file.originalPath); instead of using file.originalPath -- this ensures the paths are separated in the correction fashion, OS-independent.

@jamestalmage
Copy link
Contributor Author

@tswaters, I don't see how the changes I made here would affect / vs \. This PR just switches to using absolute paths for everything and doesn't drop anything that would have handled cross OS file path separators.

@jamestalmage
Copy link
Contributor Author

@dignifiedquire
This issue is causing real problems, to the extent that I had to write an entire new set of tooling to work around it. Please prioritize getting this fixed.

@@ -76,7 +76,6 @@ function createCoveragePreprocessor (logger, helper, basePath, reporters, covera
return function (content, file, done) {
log.debug('Processing "%s".', file.originalPath)

var jsPath = file.originalPath.replace(basePath + '/', './')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure someone had a reason they thought this was a good idea, but mucking about with istanbuls output breaks it for any downstream consumers that expect the output to conform to the spec (including istanbuls own command line tools). This makes karma-coverage brittle as its manipulating internal implementation details in istanbuls output and breaking encapsulation.

dignifiedquire added a commit that referenced this pull request Aug 6, 2015
fix(preprocessor): use absolute paths
@dignifiedquire dignifiedquire merged commit d4b2dcf into karma-runner:master Aug 6, 2015
@jamestalmage
Copy link
Contributor Author

awesome, thanks!

@dignifiedquire
Copy link
Member

Sorry for the wait. I'll cut a new release larer today.
Thanks for being persistent:)

@jamestalmage jamestalmage deleted the use-absolute-paths branch August 6, 2015 06:46
@dignifiedquire
Copy link
Member

Released as 0.5.0 as this might break custom setups.

@epoberezkin
Copy link

I still can't merge coverage reports generated by this package with other reports. I am using the version 0.5.2.

karma.conf.js:

{
    reporters: ['spec', 'coverage'],
    coverageReporter: {
        type : 'lcov',
        dir : 'coverage'
    }

The report is generated ok in the folder ./coverage/Chrome 45.0... but when I run istanbul report it combines two other folders but seems to ignore this one.

I suspect that my issue can be related to the fact that I am using grunt-istanbul to instrument source files that are then browserified, but it wasn't any different when I was using preprocessor from this plugin - the bundle file wasn't included in the combined report.

What am I doing wrong?

UPDATE:

It seems like regardless whether instrumented file has full paths or relative paths in lcov.info, coverage report has relative paths. And even if I replace those relative paths with full paths it still doesn't get merged

@jamestalmage
Copy link
Contributor Author

@epoberezkin

Have you tried the solutions discussed here. In general, you are far better off bundling using a karma plugin.

Also, you should probably apply your coverage transform during the bundling using browserify-istanbul.

Also, if you are combining with other istanbul files, you want to output type: 'json' from your preliminary test runs, and combine those. You may need to use istanbul-combine to do that, but try this first.

@epoberezkin
Copy link

Thank you very much. type: 'json' for coverage report solved the problem. Grunt-istanbul still doesn't include full paths but that's another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants