ryanluker / vscode-coverage-gutters

Display test coverage generated by lcov and xml - works with many languages
https://marketplace.visualstudio.com/items?itemName=ryanluker.vscode-coverage-gutters
MIT License
460 stars 88 forks source link

Ability to read relative paths? #29

Closed ndegardin closed 7 years ago

ndegardin commented 7 years ago

A quick question: is it possible to use relative file paths in the LCOV file?

I didn't find any VS Code coverage extension allowing for relative paths, and I'm desperately looking for that, for my development tools to be completely on the server/container side.

ryanluker commented 7 years ago

I dont see why the lcov compare couldnt have a relative option (compare equality based on file name rather then file path as it is now). I will add this to the 0.3.0 milestone and see if I can flesh something out 😄 . Thanks for the feedback!

ndegardin commented 7 years ago

Just to be sure I'm clear, I'm speaking about the SF values of the LCOV file. Some of the code coverage extensions were developed for VSCode Insiders and don't seem to work anymore. The few working extensions (VSCode LCOV and Code Cover) don't seem to accept a SF relative path (or it's not relative to the project path).

The current approach in my node/Typescript project is to generate on the server, after a project build, the Javascript LCOV file (with Istanbul and Mocha) and then to use Istanbul-remap to generate the Typescript LCOV file from the Javascript LCOV one and the source mapping.

But if Istanbul creates SF absolute paths, Remap-Istanbul creates SF relative paths... Which would be perfect for my needs, but isn't handled by the VS Code Coverage extension and led me to these threads:

lcov spec says the SF path should be an absolute path

In geninfo man page:

For each source file referenced in the .da file, there is a section containing filename and coverage data: SF: absolute path to the source file

Well, not sure if the part describing LCOV files can really be considered as a spec. But I was wondering if SF being absolute should really be the case and if there's some reason why no VS Code coverage extension handles project-relative SF paths, which would be more portable than absolute ones (and would save me some transformation scripts). Maybe potential issues I didn't think of?

ryanluker commented 7 years ago

Thanks for the extra info. I was planning on modifying / applying a configurable flag to the section around https://github.com/ryanluker/vscode-coverage-gutters/blob/master/src/gutters.ts#L95 that would turn on fuzzy file matching rather then the absolute version there is now. This fuzzy matching could be glob or regex based (or simply just the filename maybe).

ndegardin commented 7 years ago

I didn't see any line 95 in your source file... ;)

I didn't debug the code (or code VS extensions), I don't know at my level what "window.activeTextEditor.document.fileName" returns, but I guess that's a file with its absolute path, and get the issue now.

If I follow correctly, the SF filename I was speaking about is the section.file.toLocaleLowerCase() of https://github.com/ryanluker/vscode-coverage-gutters/blob/master/src/indicators.ts#L43, and it can contain an absolute path or a relative one according to the tool that spawned the LCOV file. So I don't know if regex file matching is worth... Wouldn't just defining a configuration source files root directory be enough, which would be prepended to the file path if it's relative? (that wouldn't be fuzzy matching). Or not defining this root in a configuration file and guessing it (if the path is relative, it can be relative to the LCOV file directory or the project directory)?

BTW your code is clear and concise, it's tempting to come contribute once I'll have more time...

ryanluker commented 7 years ago

Yah line 95 went away when I refactored the gutters class to be more testable in a recent release 😄 . Thanks for the extra insight as well into the possible solutions, I will see about tinkering up a PR with a workable fixing sometime in the near future! Side note, I managed to recreate this with bash for windows generating my lcov and then vscode consuming it from my C drive.

SF:/mnt/c/dev/vscode-coverage-gutters/example/test.js
FN:6,(anonymous_1)
FN:7,(anonymous_2)
FNF:2
FNH:2
FNDA:1,(anonymous_1)
FNDA:1,(anonymous_2)
DA:3,1
DA:4,1
DA:6,1
DA:7,1
DA:8,1
DA:9,1
LF:6
LH:6
BRF:0
BRH:0
end_of_record
TN:
SF:/mnt/c/dev/vscode-coverage-gutters/example/test-coverage.js
FN:1,test
FNF:1
FNH:1
FNDA:1,test
DA:1,1
DA:2,1
DA:3,0
DA:5,1
DA:6,0
DA:8,1
LF:6
LH:4
BRDA:2,1,0,0
BRDA:2,1,1,1
BRDA:5,2,0,0
BRDA:5,2,1,1
BRF:4
BRH:2
end_of_record

As you can see mnt/c/dev would never match C:\dev! Thanks for the heads up on this @tomitm .

ndegardin commented 7 years ago

Another use case :) Just have to send another request to Microsoft/Canonical to tell them to address all those troubles with permissions and symbolic links when working with containers, and I'm back to Windows!

ryanluker commented 7 years ago

@ndegardin I have a workable solution up in a pull request here https://github.com/ryanluker/vscode-coverage-gutters/pull/33 specifically related to your issue is the new relative option I added here: https://github.com/ryanluker/vscode-coverage-gutters/blob/34797538e15d3b05eed1e89606e7076a3f595d9e/src/indicators.ts#L58 backed by the test cases outlined here: https://github.com/ryanluker/vscode-coverage-gutters/blob/34797538e15d3b05eed1e89606e7076a3f595d9e/test/indicators.test.ts#L39 I made this option default to false for now but ideally next release you would be able to activate this feature 😄 . Looking forward to your feedback!

ndegardin commented 7 years ago

I gave it a try... It's working perfectly! I'm still missing two features for a perfect daily usage, but I wouldn't want to get too greedy...For now I owe you that fifth star on VS Marketplace!

image

ndegardin commented 7 years ago

And to share: My npm test command runs the following shell script to generate Typescript code coverage based on JS test execution (and mappings).

It runs on server/container side, once the compilation is done (by watching some build info file). Hence the importance for me of portable paths and of a coverage that can be refreshed on each build.

# Run the tests
node_modules/.bin/istanbul cover --dir lib/coverage node_modules/.bin/_mocha -- -R spec lib/**/test/**/*.js

# Create TS LCOV file (remap-istanbul outputs relative file paths - or maybe it's related to the source mapping style)
node_modules/.bin/remap-istanbul -i lib/coverage/coverage.json -t lcovonly -o lib/coverage/lcov.info

# Create TS cover report
node_modules/.bin/remap-istanbul -i lib/coverage/coverage.json -t html -o lib/coverage/lcov-report

Maybe there's a simpler way, I didn't have too much time to dig...

ryanluker commented 7 years ago

The way you are doing it makes sense seeming you need to have the server -> client option work, I cant see any simpler way from a high level. Side note, I created a release tonight with a few bug fixes and the relative path fix. When you get a chance to update let me know if the issue is still resolved. The option, altSfCompare, is false by default so make sure you change that to true and open / close the IDE to have the config load properly!

ndegardin commented 7 years ago

Updated to 0.2.1, it's still working great with relative paths. Definitely resolved! Thanks for the feature!