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

MATLAB coverage.xml with abstract classes cannot be read #304

Closed fschwaiger closed 2 years ago

fschwaiger commented 3 years ago

Summary

Upgrade cobertura to use a none archived library

Work


Original Ticket

Describe the bug MATLAB has an option to generate code coverage reports in standard XML format. However, for abstract interface classes, there can be blocks that have no testable lines of code. The report generator will then produce empty <lines/> nodes:

<method branch-rate="NaN" line-rate="NaN" name="rnd" signature="sample = rnd(self, mu)">
  <lines/>
</method>

This behaviour is not anticipated by this extension. Following the stack trace (sorry, I don't have it at hand currently) leads to the following code snippet in this extension's JS, where the existence of c.methods[ 0 ].method[ x ].lines[ 0 ].line[ 0 ] is assumed:

                details: !c.methods || !c.methods[ 0 ].method ? [] : c.methods[ 0 ].method.map( function ( m )
                {
                    return {
                        name: m.$.name,
                        line: Number( m.lines[ 0 ].line[ 0 ].$.number ),
                        hit: Number( m.lines[ 0 ].line[ 0 ].$.hits )
                    };
                } )

To Reproduce Steps to reproduce the behaviour:

  1. start with a valid coverage.xml
  2. empty a <lines/> node
  3. reload

Expected behaviour The file should be loadable. I have not yet figured out what the correct value for missing lines would be. We could either check right before extracting the line number and return 0 or NAN:

line: (m.lines && m.lines[0].line) ? Number( m.lines[ 0 ].line[ 0 ].$.number ) : 0,

or filter out methods that have no lines in a preliminary filter() step:

c.methods[ 0 ].method.filter(...).map(...)

Desktop (please complete the following information):

fschwaiger commented 3 years ago

I can do a pull request for either of the two solutions named above. I would like to discuss though what you consider the best option before starting.

ryanluker commented 3 years ago

@fschwaiger Thanks for creating a ticket for this issue! The code you are referencing is definitely not apart of this repo (coverage parsers provide a distinct typescript data structure that we consume). https://github.com/ryanluker/vscode-coverage-gutters/search?q=hit%3A+Number

Mostly likely you will need to submit a fix to one of the many parsers we support (some sadly are not very widely supported or maintained). https://github.com/ryanluker/vscode-coverage-gutters/blob/master/src/files/coverageparser.ts#L37-L46

Another option we have planned is to bring more of this parsing "in-house" by having the extension also do that work. https://github.com/ryanluker/vscode-coverage-gutters/issues/224#issuecomment-513604453

Supported LCOV parsers https://github.com/coverage-report/clover-json https://github.com/vokal/cobertura-parse https://github.com/vokal/jacoco-parse https://github.com/davglass/lcov-parse

fschwaiger commented 3 years ago

I see, thank you for making me aware that this is within a dependency, I should have considered that. It definitely makes more sense to fix it there: https://github.com/vokal/cobertura-parse/blob/master/source/index.js

I will create an issue there and update here when done.

fschwaiger commented 3 years ago

@ryanluker I just saw that the dependency https://github.com/vokal/cobertura-parse is archived and quite outdated.

However it appear the following fork has attempted a fix: https://github.com/qingguo-yu/cobertura-parse/commit/4cb916d0e6660a87b5f0485794fc3df5c6216e4c

ryanluker commented 3 years ago

@fschwaiger Great find! I would swap out the version we have installed here with the forked one to see if the build passes and the issue is fixed 🤔 .