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

improve EXECUTABLE_LINES_DATA_KEY metrics support #1203

Closed
Bertk opened this issue Aug 9, 2017 · 10 comments
Closed

improve EXECUTABLE_LINES_DATA_KEY metrics support #1203

Bertk opened this issue Aug 9, 2017 · 10 comments
Assignees
Milestone

Comments

@Bertk
Copy link
Contributor

Bertk commented Aug 9, 2017

CxxFileLinesVisitor should only consider source code lines from CxxGrammarImpl.functionBody for EXECUTABLE_LINES_DATA_KEY (Set executableLines).
The current implementation also adds functionDefinition, functionDeclSpecifierSeq, and declarator AST node for executable code lines.

Therefore the calculation of code coverage (%) is not correct.

@guwirth guwirth self-assigned this Aug 9, 2017
@guwirth
Copy link
Collaborator

guwirth commented Aug 9, 2017

Hi @Bertk please have a look to the graphics at https://github.com/SonarOpenCommunity/sonar-cxx/wiki/Get-code-coverage-metrics. Seems that at least Bullseye provide also for functionDefinition, functionDeclSpecifierSeq, and declarator coverage information? One reason why VS coverage does typically not match to the numbers using VisualStudio is the "partial" coverage.

@guwirth
Copy link
Collaborator

guwirth commented Aug 10, 2017

Hi @Bertk see a sample in our code base with 100% coverage. First/last bracket from functionBody are covered. Think there is the code to create/destroy local variables.

2017-08-10_16h55_47

@Bertk
Copy link
Contributor Author

Bertk commented Aug 11, 2017

@guwirth Hi, thank you for the hints. Can you please share with me which coverage tool was used.
Here is a snippet from an analysis with SQ 6.5 and bullseye 😕
bullseye
The coverage sensor need an enhancement like it is done in the sonar java plug-in FileLinesVisitor.java and calculate the correct values for executable lines based on the AST.

@guwirth
Copy link
Collaborator

guwirth commented Aug 11, 2017

@Bertk I agree. We have to handle

  • CoreMetrics.NCLOC_DATA_KEY
  • CoreMetrics.COMMENT_LINES_DATA_KEY,
  • CoreMetrics.EXECUTABLE_LINES_DATA_KEY,

different.

The only thing I'm not really sure is what are "executable lines":

  • Are these only lines within a function body?
  • Or also the functionDefinition: because at the beginning/end there is code to create/destroy local variables?
  • What's with global variables in case they have a Ctor?
  • What's with initializer lists?
  • ...

Also don't know what would happen if we follow your proposal to mark only statements within function body and a coverage tool would provide "more coverage" (e.g. for functionDefinition, initializer lists, ...).

Interesting is your sample above. functionDefinition is covered, initialisation list not (can't be) and also body not (can't also be)?

@Bertk
Copy link
Contributor Author

Bertk commented Aug 12, 2017

I have created a small project for TFS vNext build tests with SonarQube and here the coverage looks like this (SQ6.5 and MSTest, MSCoverage).
vs2017
sq6 5

The coverage results should be displayed in SQ identical to Visual Studio Editor or Bullseye Coverage Browser. I do not know whether this can be achieved because multiple coverage tools are supported.

@guwirth
Copy link
Collaborator

guwirth commented Aug 12, 2017

@Bertk think there are two cases:

  • No coverage reports available. In this case in older versions the plugin (forceZeroCiverage) or in newer version the core with metric EXECUTABLE_LINES_DATA_KEY calculates the missing coverage.
  • In case there is a coverage report the report contains typically coverage data for all lines. Means in this case EXECUTABLE_LINES_DATA_KEY is not relevant (maybe only for lines we set EXECUTABLE_LINES_DATA_KEY but the coverage tool does not include into report).

For me this means:

  • To be on the save side we should mark only lines with EXECUTABLE_LINES_DATA_KEY where we are absolutely sure they contain code to cover. Otherwise the risk is high that there are uncovered lines left in the UI.
  • In worst case this means that in case of no coverage reports the calculated coverage is a little bit too small. But in this case there is also nothing available to compare the numbers with.

Implementation proposal:

  • add only lines inside of functionBody to EXECUTABLE_LINES_DATA_KEY,
  • add visitNode and leafNode handling to detect if inside of functionBody or outside. Add only lines inside to EXECUTABLE_LINES_DATA_KEY.

@Bertk
Copy link
Contributor Author

Bertk commented Aug 13, 2017

@guwirth Hi, with the PR the result in SQ6.5 look like this:
sq6 5new
I started an analysis of a legacy project and the result will be available tomorrow.

@guwirth
Copy link
Collaborator

guwirth commented Aug 14, 2017

@Bertk you should also check the other case without report. Which lines are marked red ?

@guwirth guwirth changed the title code coverage should only consider functionBody AST nodes code coverage should only consider functionBody AST nodes: EXECUTABLE_LINES_DATA_KEY Aug 15, 2017
@guwirth guwirth assigned Bertk and unassigned guwirth Aug 15, 2017
@guwirth guwirth added this to the 0.9.8 milestone Aug 15, 2017
@guwirth
Copy link
Collaborator

guwirth commented Aug 16, 2017

Found some good description what is executable:

@guwirth
Copy link
Collaborator

guwirth commented Aug 17, 2017

Below I tried to setup a table from my current knowledge.

  • executable, default = 0: plugin is analysing the source code
  • covered, default = 0: coverage report provides covered lines
executable covered result
0 0 --
0 1 grafik
1 0 grafik
1 1 grafik

Looking to the table there are two critical cases:

  1. No coverage report available. Plugin is marking to less code as executable. In this case the missing coverage is calculated to low.
  2. Plugin marks to much code as executable. Coverage report does not mark a line as covered. In this case a line is a not covered 'false positive'.

Reading also this I think marking a line executable should be very defensive: better to less than to much.

@guwirth guwirth changed the title code coverage should only consider functionBody AST nodes: EXECUTABLE_LINES_DATA_KEY improve EXECUTABLE_LINES_DATA_KEY metrics support Aug 18, 2017
@guwirth guwirth closed this as completed Aug 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants