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

JUnit report sensor #143

Merged
merged 32 commits into from Dec 28, 2018
Merged

JUnit report sensor #143

merged 32 commits into from Dec 28, 2018

Conversation

@mwz
Copy link
Owner

mwz commented Dec 20, 2018

Initial implementation of the JUnit XML report sensor.
Unit tests and some minor refactoring is still left to do on this PR.

Example result of scanning sonar-scala:

screen shot 2018-12-20 at 22 20 52

screen shot 2018-12-20 at 22 21 08

screen shot 2018-12-20 at 22 24 48

Implements #32.

mwz added 5 commits Dec 19, 2018
override def describe(descriptor: SensorDescriptor): Unit = {
descriptor
.name(SensorName)
.onlyOnLanguage(Scala.LanguageKey)

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 21, 2018

Contributor

I believe this sensor only works for test files.
Thus, you may add
.onlyOnFileType(InputFile.Type.TEST)

This comment has been minimized.

Copy link
@mwz

mwz Dec 21, 2018

Author Owner

yes definitely


val directories: List[File] =
reports.flatMap(path => Try(pathResolver.relativeFile(fs.baseDir, path.toString)).toOption)
// TODO: Is the injected fileSystem different from context.fileSystem?

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 21, 2018

Contributor

To be honest, I had the same question before...
I don't remember why, but I ended up using the context one - I believe it was only to avoid dependency injection.

This comment has been minimized.

Copy link
@mwz

mwz Dec 21, 2018

Author Owner

I'll find this out, but if it turns out it's the same then I think it's easier to just inject it instead of accessing it via the context or extracting it into a local value - but it's not a big difference though.

.asScala

// Collect the first file here.
files.headOption.flatMap(file => reports.headOption.map((file, _)))

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 21, 2018

Contributor

Given you only care for the first file... - maybe, because it only makes sense that it matches just one file
Shouldn't you log a warning if more than one file is matched and also if any file is matched?
BTW, this reminds me #64 we may try to do this in a standardized way for all module.

This comment has been minimized.

Copy link
@mwz

mwz Dec 21, 2018

Author Owner

Shouldn't you log a warning if more than one file is matched

That's something I thought about improving, although we're using fully qualified names combined with the tests folders which should result in unique files, so I'm not worried too much about this.

The reason why I'm using inputFiles instead of a single inputFile is to avoid catching exceptions if the file is not found, but actually, now I think that it would probably be more useful to do it that way and log an error if an expected file is not found.

This comment has been minimized.

Copy link
@mwz

mwz Dec 21, 2018

Author Owner

BTW, this reminds me #64 we may try to do this in a standardised way for all module.

Agreed, I think we have been doing this in a wrong way using the cwd which leads to a whole load of path issues like, e.g. those discussed in #122. Using the FileSystem and PathResolver is the right way to go about this.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 22, 2018

Contributor

Cool, so if everything goes right we may replace our old attempts with the new resolve helper method.
Probably work for another PR (on which we should be sure to test all reported issues).

def apply[T](clazz: Class[T], module: String): Log = Log(clazz, Some(module))
def apply[T](clazz: Class[T], module: Option[String] = None): Log = {
val log: Logger = Loggers.get(clazz)
val prefix: String = "sonar-scala" + module.map("-" + _).getOrElse("")

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 21, 2018

Contributor

What about

val prefix: String = s"sonar-scala${module.fold("")("-" + _)}"

This comment has been minimized.

Copy link
@mwz

mwz Dec 21, 2018

Author Owner

yes, that's much nicer - thanks

/**
* Save a new measure for the given metric.
*/
def save[T <: java.io.Serializable](

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 21, 2018

Contributor

You may use this helper in the ScoverageSensor too.

This comment has been minimized.

Copy link
@mwz

mwz Dec 21, 2018

Author Owner

I'll roll this out across the other modules, but I'd prefer to do this in a separate pr.

mwz added 5 commits Dec 21, 2018
@@ -46,39 +48,52 @@ final class JUnitSensor(
descriptor
.name(SensorName)
.onlyOnLanguage(Scala.LanguageKey)
.onlyOnFileType(InputFile.Type.TEST)
// TODO: Add a flag to disable the sensor.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 22, 2018

Contributor

// TODO: Add a flag to disable the sensor.

👍

@@ -27,7 +28,8 @@ import org.sonar.api.batch.measure.Metric
import org.sonar.api.batch.sensor.SensorContext
import org.sonar.api.config.Configuration

import scala.language.implicitConversions
import scala.language.{higherKinds, implicitConversions}

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 22, 2018

Contributor

Where/Why exactly do we need higherKinds here ?

mwz added 4 commits Dec 22, 2018
new SensorContextOps(context)
}

final class SensorContextOps(val context: SensorContext) extends AnyVal {

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 22, 2018

Contributor

Why using an implicit conversion, instead of an implicit class?
The same goes to all syntax classes...

Also, what about having just one syntax.scala file with a object syntax { ... } and put there all the syntax helper classes?

This comment has been minimized.

Copy link
@mwz

mwz Dec 22, 2018

Author Owner

I've tried to achieve something similar to how the syntax package is structured in cats (see an example here), but what I haven't done yet was to put everything into the package object, but I 'll do this soon (example) - I think it's a nice way to organise implicit conversions.

Why using an implicit conversion, instead of an implicit class?

I think that implicit classes get compiled to an implicit function and a class, it's syntax sugar, so perhaps it was done that way in cats to optimise compile times? That's the only reason I can think of.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 23, 2018

Contributor

"I've tried to achieve something similar to how the syntax package is structured in cats"

Oh, that would be nice - I assume we won't have an all shortcut.

"I think that implicit classes get compiled to an implicit function and a class, it's syntax sugar".

I am almost sure that is correct, but it would allow us to omit the import scala.language.implicitConversions
Also, I have the impression that is the recommendation to use implicit classes over implicit conversions - but can't find nothing related to that, maybe I just heard about it on a conference.

"so perhaps it was done that way in cats to optimise compile times?"

No idea...
Maybe we should ask to the very sage wizards for clarification? 🤔

This comment has been minimized.

Copy link
@mwz

mwz Dec 23, 2018

Author Owner

Oh, that would be nice - I assume we won't have an all shortcut.

Yes, that's right - even though it's useful (and lazy!), my preference is to be more conscious about which implicit conversions I have in the scope, so I'll stay away from all 😆

I am almost sure that is correct, but it would allow us to omit the import scala.language.implicitConversions
Also, I have the impression that is the recommendation to use implicit classes over implicit conversions - but can't find nothing related to that, maybe I just heard about it on a conference.

This sounds sensible, and I'm happy with making that change unless we can find any evidence strongly supporting implicit conversions. I'll also try to dig a bit deeper and see if I can find anything more about it.

Thanks for your valuable feedback! 👍

This comment has been minimized.

Copy link
@mwz

mwz Dec 23, 2018

Author Owner

It turns out that a value class cannot be a member of another class (trait), so if we want to have this nice structure with individual syntax traits instantiated in the package object, we need to keep the implicit conversions in those traits and have the classes sitting outside. I'll push those changes for now but feel free to propose an alternative way of doing this.

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 23, 2018

Contributor

"It turns out that a value class cannot be a member of another class (trait)".

Oh, that explains it all.

Well, to be honest I think the only reason in Cats they use traits is to be able to combine them all in the all shortcut.
Since we don't plan to do that, we may leave them as objects and use implicit classes.

But at the end, there wouldn't be too much difference, so I will just leave it to your personal preference 👍


/**
* Scala.Option <-> Java.Optional conversions.
* @note Taken from https://gist.github.com/julienroubieu/fbb7e1467ab44203a09f.
*/
object JavaOptionals {
implicit def toRichOption[T >: Null](opt: Option[T]): RichOption[T] = new RichOption[T](opt)
implicit def toRichOptional[T](optional: Optional[T]): RichOptional[T] = new RichOptional[T](optional)
implicit final def toRichOption[T >: Null](opt: Option[T]): RichOption[T] =

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 22, 2018

Contributor

Since the method is part of an object and an object is already final...
marking a def final is not redundant? - or does it help the JIT inlining or something like that?

This comment has been minimized.

Copy link
@mwz

mwz Dec 22, 2018

Author Owner

This should be a trait (as per the examples above), but I haven't finished refactoring everything yet 😄

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 23, 2018

Contributor

Oh that makes sense.

}

/** Transform this Option to an equivalent Java Optional */
class RichOption[T >: Null](opt: Option[T]) {
final class RichOption[T >: Null](val opt: Option[T]) extends AnyVal {

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 22, 2018

Contributor

👍 for making this a value class - (I believe I was the one that first coded this, so thanks for improving it)
And again, why don't make this an implicit class instead of using implicit conversions?

Also, I think this counts as syntax too. Thus, we should move them to the syntax object/package and rename them OptionOps and OptionalOps just to be consistent.

This comment has been minimized.

Copy link
@mwz

mwz Dec 22, 2018

Author Owner

👍 for making this a value class - (I believe I was the one that first coded this, so thanks for improving it)

👍

I think that I addressed the first part of your comment above, but regarding those option conversions I didn't want to refactor too much at once and tried to touch as little as possible that affects other modules, so, for now, I might only add a TODO to refactor this later or possibly in a separate PR.

mwz added 5 commits Dec 22, 2018
@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Dec 23, 2018

I've also refactored the optional conversions. Almost finished, but still have some more tests to write.

mwz added 10 commits Dec 23, 2018
@mwz mwz changed the title WIP JUnit report sensor JUnit report sensor Dec 24, 2018
@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Dec 24, 2018

Ok, I'm finished - I also updated all of the examples projects with the junit settings required to get this to work. I'll open a separate PR with an updated readme and release notes in a separate PR once this is merged.

I'd appreciate if you could have another look @BalmungSan.

…thout ‘sonar.tests’)
@@ -31,12 +31,13 @@ class SonarConfigSpec extends FlatSpec with Matchers {
val conf = new MapSettings()
.setProperty("path", "this/is/a/path, another/path")
.asConfig()
val defaultPath = List(Paths.get("default/path"), Paths.get("default/path2"))

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Dec 25, 2018

Contributor

Just one minor thing, this should be called defaultPaths (since now there are many).

This comment has been minimized.

Copy link
@mwz

mwz Dec 25, 2018

Author Owner

yes of course, I'll fix that

@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented Dec 25, 2018

"I'd appreciate if you could have another look @BalmungSan."

I have been reviewing as you pushed, I just haven't commented anything because I saw everything O.K. 👍

Just one thing, why did you remove the disable property?
I just read that the sensor needs the sonar.tests property. But anyways I still think it makes sense to have the disable property. For example I have the sonar.tests property in my projects, but I don't plan to use this module.

Also, we must not forget to apply some of the changes we have discussed here to other parts of the project. I don't know if you want to to that before or after the new release.

BTW, Happy holidays 🎉

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Dec 26, 2018

I have been reviewing as you pushed, I just haven't commented anything because I saw everything O.K. 👍

Cool, thanks!

Just one thing, why did you remove the disable property?

I came to the conclusion that there is no need to allow users to explicitly control when to disable this sensor because 1. the sensor won't run if you don't specify the sonar.tests property, 2. if you define it, but not for Scala tests (e.g. just for Java), then the sensor won't find any Scala test files and it won't save any metrics, so in my opinion, there is no need to for this flag. The same effect can be achieved by simply not configuring the sensor correctly, i.e. not changing any of your existing scanner properties.

Also, we must not forget to apply some of the changes we have discussed here to other parts of the project. I don't know if you want to to that before or after the new release.

I don't want to delay the release too much, so I'll do this afterwards. I have some nice features planned in the pipeline, so there will be a little bit of refactoring this and early next year :)

BTW, Happy holidays 🎉

Happy holidays to you too!

@mwz mwz merged commit 9cd167e into master Dec 28, 2018
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@mwz mwz deleted the unit-tests-sensor branch Dec 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.