add information when measure fails to save

[jmecosta]

got some issues with 5.3, i was usig a old coverage file and was now in 5.3 throwing exceptions because of mismatch lines. this pr adds a bit more information to help debug such cases. functionally it does nothing, same functionality as before

[jmecosta]

this has evolved a bit, in our organization we are collecting 3 levels of coverage.

unit, it, and overall (and overall contains a functional coverage + ut +it). sonar does not allow to collect anthing other than unit and it, so we decided to collect the remaining under overall. this gives us a idea of the ammount of coverage we have from all our testing levels.

the problem is that this functional coverage builds run only at the weekend, and we decided that its ok to use the measusres from the weekend during the week, so this pr allows to ignore cases where changes in source files can cause mismatch between the coverage and the source files.this is disable by default and in 5.3 it will fail if you try to add a measure to a file that does not contain that line

[guwirth]

@jmecosta see my comments on #758

[Bertk]

I also discovered this issue with SQ 5.3 and temporarily disabled coverage analysis. We also use distributed system for build and test.
👍 but this should be the default configuration and therefore the configuration options:

CxxCoverageSensor.IGNORE_INVALID_UNIT_COV_MEASURES
CxxCoverageSensor.IGNORE_INVALID_IT_COV_MEASURES
CxxCoverageSensor.IGNORE_INVALID_OVERALL_COV_MEASURES

should not be introduced.

[jmecosta]

I am not so inclined to allow invalid measures to be skipped without
acknowledgement. If this happens more likely the valid measures are also
invalid and the results are not relevant. This is definitely the case for
unit tests and issues. The other stuff it depends who's looking and how you
have your builds setup. If nightly then you might want to enforce that no
invalid measures reaches database. In our specific case, we will allow only
overall coverage because its suppose to be updated weekly. Overall coverage
is a indication for QA people which do not require real time data.

I think by default the analysis should fail since this is now the default
in 5.3 and we should be inline with core.

I also think each case is one case and should be analysed individually.

Regarding the 3 suggestions, 2 and 3 are interesting ideas however I think
it's not the responsibility of plugin the implement this bit core should
have this concept available for plugins to use. So for me approach one is
what should be used until core supports the others

On Sat, Jan 16, 2016, 11:48 Bert [email protected] wrote:

I also discovered this issue with SQ 5.3 and temporarily disabled coverage
analysis. We also use distributed system for build and test.
[image: 👍] but this should be the default configuration and therefore
the configuration options:

CxxCoverageSensor.IGNORE_INVALID_UNIT_COV_MEASURES
CxxCoverageSensor.IGNORE_INVALID_IT_COV_MEASURES
CxxCoverageSensor.IGNORE_INVALID_OVERALL_COV_MEASURES

should not be introduced.

—
Reply to this email directly or view it on GitHub
#757 (comment)
.

[Bertk]

I am not sure to break the analysis execution because of synchronization issues. Maybe this is more a philosophical discussion whether reliable execution or accurate measures shall have higher priorities.

The error messages will anyway indicate a problem with the coverage report file.

      CxxUtils.LOG.error("Saving   '{}' = '{}'", measure.getMetricKey(), measure.getValue());  
      CxxUtils.LOG.error("       cov measure data = '{}' ", measure.getMetricKey(), measure.getData());  
      CxxUtils.LOG.error("Obtained '{}' = '{}'", convertedMeasure.getMetricKey(), convertedMeasure.getValue());  
      CxxUtils.LOG.error("       cov measure data = '{}'", convertedMeasure.getMetricKey(), convertedMeasure.getData());  
      CxxUtils.LOG.error("Ctype : '{}'     Exception '{}'", ctype, ex.getMessage());  

For example one of my project overall analysis runs 7+ hours and I would accept some out-dated measures before executing it again 😟

[jmecosta]

I totally agree with you, that it depends on the team using it. But for
sure this should be made optional. Now the default behaviour I think we
should call a vote ?

On Sat, Jan 16, 2016, 21:11 Bert [email protected] wrote:

I am not sure to break the analysis execution because of synchronization
issues. Maybe this is more a philosophical discussion whether reliable
execution or accurate measures shall have higher priorities.

The error messages will anyway indicate a problem with the coverage report
file.

  CxxUtils.LOG.error("Saving   '{}' = '{}'", measure.getMetricKey(), measure.getValue());
  CxxUtils.LOG.error("       cov measure data = '{}' ", measure.getMetricKey(), measure.getData());
  CxxUtils.LOG.error("Obtained '{}' = '{}'", convertedMeasure.getMetricKey(), convertedMeasure.getValue());
  CxxUtils.LOG.error("       cov measure data = '{}'", convertedMeasure.getMetricKey(), convertedMeasure.getData());
  CxxUtils.LOG.error("Ctype : '{}'     Exception '{}'", ctype, ex.getMessage());

For example one of my project overall analysis runs 7+ hours and I would
accept some out-dated measures before executing it again [image:
😟]

—
Reply to this email directly or view it on GitHub
#757 (comment)
.

[Bertk]

👌 let us vote on the default behavior.

[guwirth]

Independent how we decide things for overall coverage seems to change anyway:

• [SONAR-6225] Deprecate the "Overall condition coverage" related metrics and stop storing related measures - SonarSource https://jira.sonarsource.com/browse/SONAR-6225
• [SONAR-6202] Overall coverage measures should be computed by the platform - SonarSource https://jira.sonarsource.com/browse/SONAR-6202

Default behaviour, also with SQ core it's not consistent at the moment:

• issues: are ignored if an issue without a valid source file / line is used
• measures: measures for invalid source files seems to throw an exception

[jmecosta]

oh, thats quite bad. i was not aware of this. overall, is the one that causes the most issues. if we dont have it then i am not that interested in this feature.

so the discussion is only about if invalid measures should be skipped or fail analysis.

i vote for default to fail.

@guwirth issues? did you try with 5.3, adding a issue to a line that does not exist?

[guwirth]

@guwirth issues? did you try with 5.3, adding a issue to a line that does not exist?

@jmecosta there is still this code: https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/utils/CxxReportSensor.java

 private int getLineAsInt(String line, int maxLine) {
    int lineNr = 0;
    if (line != null) {
      try {
        lineNr = Integer.parseInt(line);
        if (lineNr < 1) {
          lineNr = 1;
        } else if (lineNr > maxLine) { // https://jira.sonarsource.com/browse/SONAR-6792
          lineNr = maxLine;
        }
      } catch (java.lang.NumberFormatException nfe) {
        CxxUtils.LOG.warn("Skipping invalid line number: {}", line);
        lineNr = -1;
      }
    }
    return lineNr;
  }

So I would expect it's ignored?

[jmecosta]

No :) I understood you were referring to the core behaviour. And was not
aware that we do this. In fact what we do its even worse, basically all
issues that fall outside a file will be reported against the last line. To
be honest I don't think this is a good thing since all issues in for that
file can be misplaced.

On Mon, Jan 18, 2016, 18:34 Günter Wirth [email protected] wrote:

@guwirth https://github.com/guwirth issues? did you try with 5.3,
adding a issue to a line that does not exist?

@jmecosta https://github.com/jmecosta there is still this code:
https://github.com/SonarOpenCommunity/sonar-cxx/blob/master/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/utils/CxxReportSensor.java

private int getLineAsInt(String line, int maxLine) {
int lineNr = 0;
if (line != null) {
try {
lineNr = Integer.parseInt(line);
if (lineNr < 1) {
lineNr = 1;
} else if (lineNr > maxLine) { // https://jira.sonarsource.com/browse/SONAR-6792
lineNr = maxLine;
}
} catch (java.lang.NumberFormatException nfe) {
CxxUtils.LOG.warn("Skipping invalid line number: {}", line);
lineNr = -1;
}
}
return lineNr;
}

So I would expect it's ignored?

—
Reply to this email directly or view it on GitHub
#757 (comment)
.

[guwirth]

@jmecosta think this is no more needed and have to be reworked anyway. I close it.