Fix for Scoverage prefix issue with Gradle #63
Conversation
# Conflicts: # src/main/scala/com/mwz/sonar/scala/scoverage/ScoverageReportParser.scala # src/main/scala/com/mwz/sonar/scala/scoverage/ScoverageSensor.scala
|
Hello @mwz, excellent work. I still think there should be a simpler (better?) way to solve the paths problems. But it is a topic that requires more discussion. So for now I'm more than happy for your fix. 👍 |
| .toFile | ||
| .exists => | ||
| prefix.resolve(PathUtils.stripOutPrefix(prefix, scoverageFilename)).toString | ||
| filename <- sourcePrefixes map { prefix => |
There was a problem hiding this comment.
@BalmungSan, do you remember why we need to do any of that filename processing in the first place? Can we just not use whatever filename comes from the scoverage report?
There was a problem hiding this comment.
Yes...
I'll comment here a summary of why, just give like 30 minutes I finish something. 👍
There was a problem hiding this comment.
Ok, I'll try to include every important detail. The story goes something like this
First, when I made the ScoverageSensor refactor (#34). I face a problem, the Scoverage reports generated by SBT had filenames were relative to packages folder. (That means something like "mwz/sonar/scala/ScalaPlugin.scala"). But the filenames that SonarQube returns were relative to the project root folder. That means something like "src/main/scala/mwz/sonar/scala/ScalaPlugin.scala").
At that time I naively thought that we only need to add the "src/main/scala" prefix to all Scoverage filenames.
But I wasn't happy enough with that, so I asked you if that was a safe assumption, you suggested that we should add a property to define that prefix and default it to "src/main/scala". I decided to reuse the property sonar.sources, because "I didn't see any value in having another one which will end with the same value." (as you can see here)
Sadly we (mostly me) made bad assumptions:
- Every Scoverage report tool will have the same format.
- The
sonar.sourcesproperty will have only one value, and it will be the same the user provide. - The plugin will work the same with multi-module projects.
So, the problems began. First was #51, there we saw that the mvn SonarQube plugin changes the sonar.source property to be full path and that it can have multiple values. We also noticed that the plugin didn't work in Windows. The solution was to relativize every value in sonar.source to the CWD and test if it + the filename exist.
Then was #56 part 1, there the problem was that gradle generates reports with filenames relative to the project root, so adding the prefix would break the filenames. The solution was to check if the filename already have the prefix or not.
Then in #56 part 2, the problem was that for multi-module projects the sonar.sources property includes the module folder thus the prefix removal process would fail. And the solution is this PR.
Conclusion
We end up with a very complex process to handle all this, but most of it is caused by inconsistencies of the different tools.
I think we could come with a simpler solution, for example add a property to define the prefix (what an irony).
PS: Sorry for taking so long (30 minutes =!= 3 hours 😞). Too many things to do, too little time. And sorry if my English was worse than usual, I'm in a hurry.
|
Hi @mwz, I loved the Log changes 👍. |
|
Thanks for your detailed explanation - it all makes a perfect sense. I agree with you that this is becoming more and more tricky as we're trying to cover all the differences between various build tools. Hopefully, we can come up with a simpler solution to this, but it definitely requires a bit more thought... If you're happy for now I think that we should go with this solution and revisit this in the future if we come across any more issues with prefixes. |
|
Yeah Yeah absolutely, as I said I'm sure there should be a simpler/better way. But it requires more discussion. So for now I'm very happy with this Fix, and you should merge it right way. And latter we should think more about this. |
|
Excellent! I'll do a bit more testing tomorrow before releasing this to make sure there are no regressions with other build tools, which probably means I will also open another PR with example sbt and mvn projects. |
Fixes the issue identified by both @freshka and @sinwe in #56.
The issue was caused by the Scoverage plugin behaving differently in various build tools and resulting in the Scoverage sensor not working correctly for multi-module Gradle projects.
The proposed solution strips out current module path from the
sonar.sourcesprefix when parsing Scoverage report then strips out this modified prefix from the Scoverage filename so it can be then recombined with the sources prefix which results in a full path relative to the root of the project.To test this I used the example Gradle projects introduced in #62, which both now correctly report coverage details to Sonar.