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

Fix for Scoverage prefix issue with Gradle #63

Merged
merged 4 commits into from May 17, 2018
Merged

Fix for Scoverage prefix issue with Gradle #63

merged 4 commits into from May 17, 2018

Conversation

@mwz
Copy link
Owner

mwz commented May 17, 2018

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.sources prefix 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.

mwz added 3 commits May 17, 2018
# Conflicts:
#	src/main/scala/com/mwz/sonar/scala/scoverage/ScoverageReportParser.scala
#	src/main/scala/com/mwz/sonar/scala/scoverage/ScoverageSensor.scala
@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented May 17, 2018

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 =>

This comment has been minimized.

Copy link
@mwz

mwz May 17, 2018

Author Owner

@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?

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan May 17, 2018

Contributor

Yes...
I'll comment here a summary of why, just give like 30 minutes I finish something. 👍

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan May 17, 2018

Contributor

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.sources property 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.

@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented May 17, 2018

Hi @mwz, I loved the Log changes 👍.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented May 17, 2018

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.

@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented May 17, 2018

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.
👍

@mwz mwz merged commit 304ab21 into master May 17, 2018
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@mwz mwz deleted the scoverage-prefix-fix branch May 17, 2018
@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented May 17, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
sonar-scala
  
Done
2 participants
You can’t perform that action at this time.