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

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

Closed
1 task
fschwaiger opened this issue Apr 15, 2021 · 5 comments · Fixed by #360
Closed
1 task

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

fschwaiger opened this issue Apr 15, 2021 · 5 comments · Fixed by #360
Labels
dependencies Pull requests that update a dependency file good first issue
Milestone

Comments

@fschwaiger
Copy link
Contributor

fschwaiger commented Apr 15, 2021

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):

  • OS: macOS
  • Extension Version: v2.7.2
  • VSCode Version: v1.55.2
@fschwaiger
Copy link
Contributor Author

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
Copy link
Owner

@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. #224 (comment)

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@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: qingguo-yu/cobertura-parse@4cb916d

@ryanluker
Copy link
Owner

@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 🤔 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants