Add fallback for 'reactor' report search in CxxCoverageSensor

[Zetten]

Our mixed Java/C++ project setup follows quite a traditional multi-module Maven project configuration, but the reactor changes introduced in e714d3e break the CxxCoverageSensor for us.

It's no longer possible to use a relative report path for sonar.cxx.coverage.reportPath such as target/gcovr/report.xml in a multi-module project. This filter is used from the reactor root (reactor.getRoot().getBaseDir().getAbsolutePath()) and won't match the individual module reports.

All other Sensor classes inherit a reports.isEmpty() check from CxxReportSensor to revert from the reactor-based root directory to a local root directory when searching for reports, so other sensors are not affected by this.

This commit adds the same check to the three types of coverage reports.

[guwirth]

Someone else having mixed Java/C++ project could test this?

[jmecosta]

It should apply to any language, but I guess this should be considered to
make all sensors consistent.

On Wed, May 20, 2015, 22:47 Günter Wirth [email protected] wrote:

Someone else having mixed Java/C++ project could test this?

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

[Zetten]

Yeah, I'm not sure it's specific to the languages, but more the hierarchical nature of our project. Take something like this, where each cpp_# module generates its own reports in a target/ subdir:

project/
├── module1
│   ├── cpp_1
│   └── cpp_2
└── module2
    └── cpp_3

If you run the SonarQube analysis from project/ with a reportPath of target/gcovr/report.xml it will only ever match project/target/gcovr/report.xml because that's the reactor root.

I guess part of the problem is we're a fully Maven-based project, with properties carried through the pom.xml hierarchy. In a more normal (for C/C++ projects) sonar-runner/sonar-project.properties sort of usage I guess the issue is less likely to be noticed since this usage tends towards a more flat project structure.

[jmecosta]

Yep this the expected behavior currently. The idea was to make sure the
other way works, so having a file in root it would be pick up by module.
This should be visible if using vs bootstrapper with the same settings.
Just not sure why we left the coverage sensor without fallback. But to me
the PR looks good

On Thu, May 21, 2015, 00:05 Peter van Zetten [email protected]
wrote:

Yeah, I'm not sure it's specific to the languages, but more the
hierarchical nature of our project. Take something like this, where each
cpp_# module generates its own reports in a target/ subdir:

project/
├── module1
│ ├── cpp_1
│ └── cpp_2
└── module2
└── cpp_3

If you run the SonarQube analysis from project/ with a reportPath of
target/gcovr/report.xml it will only ever match
project/target/gcovr/report.xml because that's the reactor root.

I guess part of the problem is we're using a fully Maven-based structure,
with properties carried through the pom.xml hierarchy. In a more normal
(for C/C++ projects) sonar-runner/sonar-project.properties sort of usage
I guess the issue is less likely to be noticed since this usage tends
towards a more flat project structure.

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

[guwirth]

@Bertk I think you are the second one using this multi-module thing. Does this work for you?

[Zetten]

Perhaps a more abstract way of searching for reports would be a better way of approaching this. All of the other sensors make use of the analyse() method in the CxxReportSensor class, which embeds the report search (with the reactor-local fallback).

If the report search were extracted to a general findReports() method (taking key and default path) it could be used from both the generic analyse() method as well as individually from the CxxCoverageSensor which looks for multiple reports.

This way any future work on the report matching will be in the same place, and the CxxCoverageSensor won't be left out.

[Zetten]

On inspection, that's actually quite a hefty bit of refactoring. If it's wanted I can slog through it, but consolidating and straightening out report matching will force us to modify a fair few other classes and tests.

The already-submitted fix to CxxCoverageSensor is certainly a lower-impact fix.

[jmecosta]

Yep, pretty much all sensors require this. Perhaps when core decides to
invent a new of lookup files we can do this refactoring

On Thu, May 21, 2015, 15:29 Peter van Zetten [email protected]
wrote:

On inspection, that's actually quite a hefty bit of refactoring. If it's
wanted I can slog through it, but consolidating and straightening out
report matching will force us to modify a fair few other classes and tests.

The already-submitted fix to CxxCoverageSensor is certainly a lower-impact
fix.

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

[Bertk]

I also found this issue but did not create a PR until now. Please check another small impact solution on this branch https://github.com/Bertk/sonar-cxx/blob/Bert/sonar-cxx-plugin/src/main/java/org/sonar/plugins/cxx/coverage/CxxCoverageSensor.java.
There is one part missing and the exception in getCoverageReports shall be logged (CxxUtils.LOG.warn).

[guwirth]

@Zetten could you please add the feedback from @Bertk

[Zetten]

The commit from @Bertk is basically exactly the same as mine, except the search/re-search construct was extracted into a separate method.

The Exception thing is actually a bit more interesting though. In CxxReportSensor the reactor path is found with getCanonicalPath(), which can throw an IOException. This is handled in a try/catch. However in the current CxxCoverageSensor (and a few other places, e.g. [1]) getAbsolutePath() is used instead, which throws no checked exceptions.

So my commit is really a bare-minimum change to CxxCoverageSensor; literally just adding for each reports search "if empty, search in project baseDir".

I think it stands pretty well on its own. If the desired approach is to extract the reports search/fallback to its own method with error checking/logging I'd rather do it for all sensors and refactor it fully.

[Bertk]

Please check the URL http://stackoverflow.com/questions/1099300/whats-the-difference-between-getpath-getabsolutepath-and-getcanonicalpath. There is a nice description for getCanonicalPath() and getAbsolutePath(). I prefer getCanonicalPath() which gives a nice path also for logging purpose. This can be used to easily to check every given path for troubleshooting.
I saw paths like "C:\temp\myapp\bin....\file.txt" many times and think this is not useful.
I like your proposal to harmonize the search/fallback for all sensors 😉

[guwirth]

I merge this as a bugfix. Fell free to refactor the code.