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.

Sign up for GitHub

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

Jump to bottom

Add relative option to lcov format files. #771

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add relative option to lcov format files. #771

wants to merge 1 commit into from
+4 −2

Conversation

miguelrincon
Copy link

@miguelrincon miguelrincon commented Mar 1, 2017 •
edited
Loading

When using coverage with lcov files, reports are created using absolute paths.

There is a good reason to want to generate the report using relative paths. This is both for security reasons and when using an external service in another location such as SonarQube in long delivery pipelines.

This PR offers the option to create the reports with relative paths, this option is turned off by default.

Refers to #104

@miguelrincon miguelrincon mentioned this pull request Mar 1, 2017
Closed
@titocr
Copy link

titocr commented May 16, 2017

Looking forward to seeing this released. It'll clean up the need to do post-processing on the generated lcov file.

@Isabello
Copy link

Isabello commented Jun 9, 2017

I have a real need for this as well. I hope to see it merged soon.

@TimHeap46
Copy link

Hi, is there any update as to when this is likely to be released, we're having issues with sonarqube and istanbul because of absolute vs relative paths ?

@titocr
Copy link

titocr commented Sep 7, 2017

As a workaround I'm using sed to modify the file after it's been generated. Something like this:

# convert absolute paths to relative before sonar runs
sed -i 's|SF:/part_of_path_i_want_to_go_away/|SF:|g' coverage/report-lcov/lcov.info

@TimHeap46
Copy link

TimHeap46 commented Sep 8, 2017 via email

@titocr
Copy link

titocr commented Sep 9, 2017

Glad to help. As for a permanent fix, as I understand it, it's sitting here with the commit @miguelrincon did. I'm just waiting on it like everyone else ;-)

@TimHeap46
Copy link

TimHeap46 commented Sep 10, 2017 via email

@TimHeap46
Copy link

TimHeap46 commented Sep 11, 2017 via email

@titocr
Copy link

titocr commented Sep 11, 2017 •
edited
Loading

Our CI is jenkins and this is the line I use (changed only the last part of the path). I had to try a couple different things before discovering what path was needed.

sed -i 's|SF:/opt/jenkins/workspace/myProject/|SF:|g' coverage/report-lcov/lcov.info

An altered line in lcov.info, before and after:

SF:/opt/jenkins/workspace/myProject/src/app/config/app_config.js

SF:src/app/config/app_config.js

Hope that helps.

@TimHeap46
Copy link

TimHeap46 commented Sep 11, 2017 via email

@TimHeap46
Copy link

TimHeap46 commented Sep 18, 2017 via email

@csotiriou
Copy link

I would love to see this merged. Our build pipeline includes running tests on one docker container and using instanbul to report coverage in another machine using sonarqube.

We really need this option.

@jmpresso
Copy link

Would also like this change

@TimHeap46
Copy link

any idea if this is likely to happen any time soon ?

@TheJefe
Copy link

TheJefe commented Jun 20, 2018 •
edited
Loading

Looks like this repository has been deprecated, nyc is the replacement for this tool, but it too, does not support relative paths.

So I've solved this for myself like some others in this thread with sed, however instead of making the path relative, just encase that brakes something else, I update the path to be absolute to the current directory with sed -i "s|SF:/app|SF:$(pwd)|g" lcov.info

@ozoli
Copy link

ozoli commented Sep 7, 2018 •
edited
Loading

In a Typescript Angular 6 project I needed to make the path to be that of the Docker image used for the sonar-scanner. Since the path could change in the build pipeline (BitBucker) I used the following sed expression:

sed -i "s|SF:/.*/my-project/|SF:/root/src/|g" test-reports/lcov.info

where /root/src is the name of the mapped folder of the current folder in the Docker container that was started by the following command:

docker run -ti -v $(pwd):/root/src newtmitch/sonar-scanner

@aleclarson
Copy link

This is such a small change. What's the blocker? Lack of maintainers?

@melonwannajack
Copy link

melonwannajack commented Nov 22, 2018 •
edited
Loading

Hey, it would be a very useful change, what is blocking ?

@AlexisNoAccor
Copy link

I would love to see this merged too !

@sybers
Copy link

sybers commented Feb 19, 2019

Anything new for this PR ? It would be really useful to have this option !

It's not that I don't like sed but... 😅

@matiasdecarli
Copy link

2 years and still not merged 🤦‍♂️

@itolu94
Copy link

itolu94 commented Jun 7, 2019

What's stopping this from being merged? I really need this feature.

@FlorinAsavoaie
Copy link

Someone really enjoys using sed and hacks... :(

@atwright147
Copy link

atwright147 commented Oct 24, 2019 •
edited
Loading

On Github (with actions), I successfully used sed -i "s|SF:/.*/src/|SF:/github/workspace/src/|g" coverage/lcov.info

Thank you @ozoli

wmfgerrit pushed a commit to wikimedia/mediawiki-extensions-Wikibase that referenced this pull request Oct 31, 2019
@wiese
bridge: wire up with sonar
66873cc 
Allow sonar to access the bridge coverage.
Apparently codehealth jobs invoke `--if-present test:unit`, not the
complete `test` script, so redirect this script to bridge as well.
See
https://sonarcloud.io/documentation/analysis/coverage/#test-execution
https://www.npmjs.com/package/jest-sonar-reporter

In order to run coverage only for this codehealth job and to not have
that overhead in the normal CI, an extra specialised script is added to
the data-bridge package.json.

The jest coverage creates absolute paths in lcov.info. However, the
tests are run in a docker-container whereas the analysis is performed
outside of it and therefore the paths don't match. Since an option to
create relative paths in lcov.info is not yet available[0], this uses
sed to manually make the paths relative.

Also, it seems that collecting coverage is only really useful for unit
tests and so the coverage related configuration has been moved there.

This patch tries to use the multi module functionality of sonar in order
to split up the sonar configuration. It seems promising so far, but
whether it really works as intended can probably only be seen after it
has been merged into master.

[0]: gotwarlost/istanbul#771
[1]: https://docs.sonarqube.org/display/SONARQUBE51/Analyzing+with+SonarQube+Runner#AnalyzingwithSonarQubeRunner-Multi-moduleProject

Change-Id: Ic2954e35c7c053eae1ba958751334dc23d2f3f3a
wmfgerrit pushed a commit to wikimedia/mediawiki-extensions that referenced this pull request Oct 31, 2019
Update git submodules
ddea6dc 
* Update Wikibase from branch 'master'
  to a00815c9502b58389714fa9f903504e00216606e
  - Merge "bridge: wire up with sonar"
  - bridge: wire up with sonar
    
    Allow sonar to access the bridge coverage.
    Apparently codehealth jobs invoke `--if-present test:unit`, not the
    complete `test` script, so redirect this script to bridge as well.
    See
    https://sonarcloud.io/documentation/analysis/coverage/#test-execution
    https://www.npmjs.com/package/jest-sonar-reporter
    
    In order to run coverage only for this codehealth job and to not have
    that overhead in the normal CI, an extra specialised script is added to
    the data-bridge package.json.
    
    The jest coverage creates absolute paths in lcov.info. However, the
    tests are run in a docker-container whereas the analysis is performed
    outside of it and therefore the paths don't match. Since an option to
    create relative paths in lcov.info is not yet available[0], this uses
    sed to manually make the paths relative.
    
    Also, it seems that collecting coverage is only really useful for unit
    tests and so the coverage related configuration has been moved there.
    
    This patch tries to use the multi module functionality of sonar in order
    to split up the sonar configuration. It seems promising so far, but
    whether it really works as intended can probably only be seen after it
    has been merged into master.
    
    [0]: gotwarlost/istanbul#771
    [1]: https://docs.sonarqube.org/display/SONARQUBE51/Analyzing+with+SonarQube+Runner#AnalyzingwithSonarQubeRunner-Multi-moduleProject
    
    Change-Id: Ic2954e35c7c053eae1ba958751334dc23d2f3f3a
@JackLeo
Copy link

JackLeo commented Dec 3, 2019 •
edited
Loading

I've put a hack for this in our CI in 2017, it's still an issue now...

@calvin-barker
Copy link

I've added the sed hack as well, but it'd be nice to get this merged...

@gmauch
Copy link

gmauch commented Feb 14, 2020

I'd really like to have this 2-year old (and counting) much needed fix to be merged, also!
Any way to speed that up?

@Thilaknath
Copy link

When will this be merged, it's been more than 2 years ?

@kishanchaitanya
Copy link

3+ years and finally approved, Please merge it in

@fatihdestegul
Copy link

I started to use sed as a work-around and this is approved. lol

@moos
Copy link

moos commented Jul 9, 2020

Believe for the PR to work properly (at least in CLI mode), some additional changes are needed. I've include this PR in my fork istanbul-moos, which includes the fix for the CLI if anyone is interested.

@Isabello
Copy link

Isabello commented Jul 9, 2020 via email

@flaval
Copy link

flaval commented Jan 27, 2021

I don't know if I'm authorized to approve the commit but I did it 😅
I'm interested by this fix too, how can we move forward about it ?

@Isabello
Copy link

I don't know if I'm authorized to approve the commit but I did it 😅
I'm interested by this fix too, how can we move forward about it ?

@gotwarlost or someone else with Write Access would have to come click merge...

@flaval
Copy link

flaval commented Jan 27, 2021

Thanks for explanations @Isabello, I just figured out the Deprecation Notice in this repo's README.md file :

Deprecation Notice: this version of istanbul is deprecated, we will not be landing pull requests or releasing new versions. But don't worry, the Istanbul 2.0 API is now available and is being actively developed in the new istanbuljs organization.

So I suppose we can't expect anything from this pull request 😕
As advised in the notice, I turn myself to this repo

@runlevel5
Copy link

Wow it's hard to believe that it's been 2 years and this issue has not been resolved. I also run into exact issue

@flaval
Copy link

flaval commented Mar 11, 2021

@runlevel5 Like I said in post above, this repo is deprecated and, IMO, not maintained anymore.
You can take a look at this repo instead.

@jcnauta
Copy link

jcnauta commented Sep 21, 2022 •
edited
Loading

Indeed, I believe this issue has been resolved there in the active fork of istanbul as @flaval pointed out.

@gotwarlost if you have the time could you archive your original version of istanbul, or at least add a giant banner that it's no longer maintained with a pointer to the current version? This repo still pops up in search results far more than it should.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flaval flaval flaval approved these changes

@icame21 icame21 icame21 approved these changes

@JackLeo JackLeo JackLeo approved these changes

@subinvarghesein subinvarghesein subinvarghesein approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.