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

Refactor scoverage plugin #34

Merged
merged 45 commits into from Mar 24, 2018
Merged

Refactor scoverage plugin #34

merged 45 commits into from Mar 24, 2018

Conversation

@BalmungSan
Copy link
Contributor

BalmungSan commented Mar 20, 2018

This Massive PR intends to refactor the old scoverage plugin.
This PR closes #23 #18 #14 and partially #7

Note: Breaking change.
The scoverage report property is renamed from sonar.scoverage.reportPath to sonar.scala.scoverage.reportPath.
Additionally, this property now has a default value of target/scala-${scala major.minor version}/scoverage-report/scoverage.xml.

This PR changes a lot of things, so it should be reviewed carefully, also I suggest to test it before merging.
I tested it with my projects and worked perfectly, but I suspect it could fail with multi module projects.

BalmungSan added 30 commits Mar 13, 2018
…helper method that abstracts the metric creation logic
…rser, missing line coverage and multiple classes in file
@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Mar 22, 2018

Hello @mwz,

I had made the changes you suggested, except for the metrics one, see my comment for a discussion about it.

I ended using the sonar.sources property because it is a mandatory one, and I don't see any value in having another one (sonar.scala.sources) which will end with the same value.

Additionally, I had already tested the plugin with a multi-module project and it worked perfectly (after some minor changes) 😄

So, I think the PR is now ready to merge, unless you had other suggestions.

SonarQube Rules
I think the new commits have hidden the comments I made about the sonar rules. So I'm pasting them here:

  • Literal Arguments: In some cases the named argument is not only unnecessary, but also would make the code more obscure. For example: BigDecimal(d = 2.0).
    Also the rule does not make sense when working with java code, because it is impossible to use a named argument.
  • Pattern match align: If we plan to have this rule, we must configure scalafmt to do it automatically.
@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Mar 22, 2018

That's excellent news! I haven't finished reviewing it last night, so I'll try to go through the rest of the code this evening and I'll let you know if I have any more comments.

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Mar 22, 2018

Ok!

I would be waiting for any other suggestion, and I would be happy to fix it.

Thanks for taking the time for carefully reviewing this massive PR. 👍

* It is composed of:
* - the overall [[ScoverageMetrics]] of the file.
* - the coverage information of each line of the module.
*/

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

+1 for the comments above

) {

/** Merges two scoverages metrics in one */
private[scoverage] def +(that: Scoverage): Scoverage = {

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

The moment I saw this I thought this would be a good use case for a Semigroup typeclass from cats to combine two Scoverage instances into one without reimplementing a custom + function, e.g. scoverage1 |+| scoverage2, but don't worry about it for now.
At some point though, perhaps when we decide to write a scapegoat module, I would like to start to make use of cats and cats-effect as there would be a lot of good use cases for those libraries in this project.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 22, 2018

Author Contributor

Yeah me too, but I didn't wanted to add a new dependency only for one use.
But yeah, if we find more use cases i would be happy to use it.

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

Pretty much a standard these days so I'm totally happy with importing it whenever we need it - IMHO the additional size of those dependencies shouldn't really be an issue.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 22, 2018

Author Contributor

Ok then, if you want I will add them right now.
Or maybe later, We should identify all uses for the library and open another PR for integrate it.

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

yeah, there is probably no need for adding it as a part of this PR


/* applied on lines of code */

/* applied on tokenized code */

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

What are the above comments referring to?

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 22, 2018

Author Contributor

No idea, not mine.
I only change the package name of that file.

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

Should we get rid of those then?

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 22, 2018

Author Contributor

I think yes, we should.
I also found them confusing.

* Util methods and conversions
*
* @note The Scala.Option <-> Java.Optional conversions were taken from
* https://gist.github.com/julienroubieu/fbb7e1467ab44203a09f

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

Looks like the scala-java8-compat library would have been useful in this case, but yeah I'm happy to keep this if we just need Option conversions.

* @note The Scala.Option <-> Java.Optional conversions were taken from
* https://gist.github.com/julienroubieu/fbb7e1467ab44203a09f
*/
package object util {

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

There's probably no need to wrap those objects and classes in a package object as suggested by Alexandru.

You could just define a chained package instead of the package object like so:

package com.mwz.sonar.scala
package util

... objects and classes

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 22, 2018

Author Contributor

Uhm, but then the same should be done with the scoverage package object ?

This comment has been minimized.

Copy link
@mwz

mwz Mar 23, 2018

Owner

Well, this approach wouldn't work for the scoverage package object because you have a type definition and some vals inside of it.
Perhaps we should just forget about this for now as this is just a minor thing and we can always come back to this and refactor later - definitely not a blocker!

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 23, 2018

Author Contributor

Ok, I will leave it as it is for now.

}

/** Saves in SonarQube the scoverage information of a module */
override def execute(context: SensorContext): Unit = {

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

Previously the sensor would aggregate over all the existing modules and create the total coverage for the whole project here, but given that this sensor now only has in the scope one module at a time does this still work as expected? Does Sonar do the coverage aggregation automatically? I'm not entirely sure how this works now for multi-module projects 🤔

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 22, 2018

Author Contributor

The coverage metric, that is computed internally from sonar using the the lineHits method in the NewCoverage class, gets aggregated as expected.
But custom metrics like scoverage, and branch_scoverage don't.
You can see them by each module.

An alternative, is to make the sensor global, so sonar will execute it for the whole project, but that would mean, that the user should had generate a global scoverage report, or that the sensor should need to open all module's reports and merge the results, like the previous one.

I don't know what you think about it.

This comment has been minimized.

Copy link
@mwz

mwz Mar 23, 2018

Owner

I run analysis on a sample multi-module project and It seems to me that your approach has exactly the same outcome as the previous approach - I was able to see the same measurements that I get using the existing version of the plugin, so I'm onboard with this way of doing it.


object ScoverageSensorInternal {
private val SensorName = "Scoverage Sensor"
private val ScoverageReportPathPropertyKey = "sonar.scala.scoverage.reportPath"

This comment has been minimized.

Copy link
@mwz

mwz Mar 22, 2018

Owner

It is a good idea to align the key name with the scapegoat plugin and use sonar.scala prefix, but this would be a breaking change :/ so perhaps we should wait with making it until we release version 7 of this plugin for SonarQube 7?

Another option would be to deprecate the sonar.scoverage.reportPath key and add a warning and have support for both keys until we remove it in version 7. I'm aware that this option would require more work so I'd be happy to keep the old key for now and we can deprecate it and add the support for both keys later.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Mar 22, 2018

Author Contributor

I agree that the change should be made with more caution.

Probably the best option is to stay with the old key for now.

@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Mar 23, 2018

I've just run analysis on your branch using a local snapshot of the plugin and everything looks good to me. I've noticed that the project statement coverage which previously appeared under the Test section in Measures now shows up under Coverage -> Tests which is probably a much better place for it 👍 !

Before
screen shot 2018-03-23 at 00 09 02

Now
screen shot 2018-03-23 at 00 09 28

Also well done for increasing project coverage to ~89% 🎉🏅

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Mar 23, 2018

If I understood you well, you just analyzed this branch with the compiled plugin from this branch, wow that's what I call sonar-ception 😄.

Ok, leaving the bad jokes aside, I would like to thank you once again for taking all the time for this review.

I will make the final changes tomorrow and this would be ready for merge!

Changes
So, summarizing the changes:

  • Remove this comments.
  • Revert the scoverage report path property to its old key (sonar.scoverage.reportPath).
@mwz

This comment has been minimized.

Copy link
Owner

mwz commented Mar 23, 2018

Haha, yes it's almost like bootstrapping a compiler :)

👍 for making the first two changes and as per my previous inline comment please ignore the package object for now as it's not that important and I don't want to be delaying this PR any longer.

Thank you for doing this massive refactor, I feel like this was a very good call and your changes are definitely going to make it easier to maintain this plugin and implement new features going forward!

@BalmungSan

This comment has been minimized.

Copy link
Contributor Author

BalmungSan commented Mar 23, 2018

Ok, that's all.
👍

@mwz mwz merged commit 3973e6a into mwz:master Mar 24, 2018
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@BalmungSan BalmungSan deleted the BalmungSan:refactor-scoverage-plugin branch Mar 24, 2018
@mwz mwz mentioned this pull request Mar 25, 2018
@BalmungSan BalmungSan mentioned this pull request Mar 25, 2018
@mwz mwz added this to Done in sonar-scala Jul 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
sonar-scala
  
Done
3 participants
You can’t perform that action at this time.