Implement a sonar processor #54

dopuskh3 · 2014-12-05T13:22:03Z

Hi,

We've implemented a way for sputnik to run Sonar-runner to report issues.
We use it in conjunction with sonar-ndepend (https://github.com/criteo/sonar-ndepend) but should work with
all sonar plugins as long as they provides the line information.

Cheers

pjagielski · 2014-12-05T23:18:27Z

Hi,

Great work!

So it's just running sonar-runner from the cli, no sonar server needed?

pjagielski · 2014-12-05T23:19:44Z

src/test/java/pl/touk/sputnik/processor/sonar/SonarProcessorTest.java

+public class SonarProcessorTest {
+
+    @Test
+    public void testFilterResults() {


Please rename test names in "should<do_something>When<something_happens>" manner, as in rest of the project.

pjagielski · 2014-12-05T23:25:22Z

Can you resolve conflicts with master?

dopuskh3 · 2014-12-08T09:25:41Z

Hi,

You need a sonar server to run this processor. Sonar runner pull its configuration and dependencies from the sonar server. It also pulls issues already indexed and only report the new ones to sputnik. That way sonar runnuer instance run by sputnik only reports issues that where added in the review diff - considering your run sonar running after each commit.
Note the sonar runner launched by sputhnik do not index issues on the sonar server (sonar incremental mode).

Rebased on master, fixed the naming and TestEnvironment comments.

F.

SpOOnman · 2014-12-09T11:18:20Z

Hi!

Thank you for your submission, but I'm not sure if this is a good change. I have a few concerns about it.

First, this processor requires 3rd party server to run. That's an additional integration dependency that increases complecity of a simple Sputnik.

Second, running Sonar runner, that runs its own analysis (including PMD, FindBugs, Checkstyle and Squid) is redundant to Sputnik.

I am not sure about third. Does running Sonar runner slow down Sputnik? My goal is to run Sputnik and all analysis in less than 30 seconds, even for 100 files.

My last concern is emotional. I wrote Sputnik to get rid of Sonar. These are two tools for different purposes. Sonar is a long run project analysis that should be used for tracking progress. Sputnik is a fast static code analyser intended to support code reviewers. With this PR Sonar is getting to Sputnik and I'm spectical about it.

damianszczepanik · 2014-12-09T18:12:21Z

I also think this would be useful:

main goal of using Sonar is that (correct me if I'm wrong) you don't get error that are marked as false positive
if you don't like it you still can switch it off and it will work as it used to
If you already have Sonar installed you need to export rules and configuration and maintain them separately (in case rules are changed or upgraded)
I was able to run Sonar for more than 100 files in less than 30secs via Eclipse plugin so Sputnik should not run longer. Anyway as long as Sputnik runs faster than maven compile test then I guess its worth to think about it

I'm looking for the release with this support!

dopuskh3 · 2014-12-10T09:58:22Z

Hi,

Although it requires additional dependencies note that:

enabling sonar is not mandatory.
it is really usefull for organizations (like us) that are running sonar and gerrit and are willing to make a faster feedback on code quality than just induce developers to have a look on sonar regularily (isn't that the reason why you created sputnik ?)
defining and maintaining rules is a real pain. Having to duplicate this in both sputnik and sonar is even more painful.
this plugin do not comment already indexed issues. That mean that you only get comment on added issues on modified lines which is pretty cool.

Regarding the running time, I don't think the sonar feedback take too much time to run (at least in incremental mode) and:

why would you make code reviews with 100 files ? I think that's a very bad practice to create such huge code reviews.
sonar overhead is really negligible at least when the first run happened (sonar caches needed plugins jars on first run)

Over all this plugin allows to take advandate of the whole sonar plugin library which is really huge (http://docs.codehaus.org/display/SONAR/Plugin+Library).

cupcicm · 2014-12-11T13:28:45Z

I agree that the fact you get all sonar plugins for free with this merge request is compelling.
Also, as noted by @damianszczepanik not having to duplicate your configuration between sputnik and sonar is a good thing IMO.
Although I agree with you, @SpOOnman, that it seems sputnik was not intended to do this (since it makes the findbug / pmd processor kind of redundant.
Could we have more info on why you wanted to remove sonar ? I feel that there are two parts to code analysis :

the dashboard part, where you try to get less and less warnings in your code. For this, sonar is awesome
the review part, where you don't want to add more. For this, sputnik is awesome.

Now I already have all my configuration in sonar, why wouldn't I want sputnik to run a trimmed-down (incremental) version of sonar to give me the warnings ?

SpOOnman · 2014-12-22T15:06:59Z

build.gradle

@@ -40,6 +40,9 @@ dependencies {
    compile 'ch.qos.logback:logback-classic:1.0.13'
    compile 'org.projectlombok:lombok:1.12.6'
    compile 'commons-cli:commons-cli:1.2'
+    compile 'org.codehaus.sonar.runner:sonar-runner-api:2.4'
+    compile 'org.codehaus.sonar-plugins:sonar-issues-report-plugin:1.3'
+    compile 'org.json:json:20140107'


Is this Sonar dependency? We already use Jackson's ObjectMapper to handle JSON. Is this suitable for you to use Jackson instead of a second JSON dependency?

SpOOnman · 2014-12-22T15:18:25Z

Sorry for waiting so long.

I still have personal issues with this change, however I've found it useful. Some users want to use Sputnik in a way I didn't think of at first. Even more - in a way that I dropped. But that's fine so I want this PR to be merged.

I've commented inline some details. This PR has high quality, thank you.

Change-Id: Iaf9b4565e8ac2237d5a4d70627487e6bbb4b86fc

Add a few configuration option for further sonar review. Change-Id: I82b92e291791e838b18b6c9281225ba44161cceb

SpOOnman · 2014-12-23T15:27:29Z

src/test/java/pl/touk/sputnik/processor/sonar/SonarRunnerTest.java

+    }
+
+    @Test(expected=IOException.class)
+    public void shouldRaiseWhenNoSonarFiles() throws IOException {


Raise to throw

SpOOnman · 2014-12-23T15:28:18Z

I've found one raise, besides that it is ready to be submitted.

Load sonar configuration specified in sputnik configuration file, build a valid sonar configuration and run sonar runner inside the same process running sputnik. This is not fully functional as we need to parse sonar issues in order to report them to sputnik. Change-Id: I6117d97541b65a8c575cd178c948632ed7fe76fc

SonarResultParser parses json result and produces a ReviewResult. Change-Id: I11e2fc20567570343b20648f8d7ae8bc04d5940b

Change-Id: I86633f86858efbe9aaea177d68492c1ff937273e

Add test fixtures that test sonar will only report new issues Change-Id: I47dd6d4c088620bd5120a07a155e75ba0687b808

Some sonar plugin do no include git path as the source file path (for example, sonar-visual-studio plugin generates filenames that are relative to the module csproj file). Using the file basename and a recursive match allows Sonar to match modified files with their indexed names in Sonar. Although this can generate useless analysis (as some files that are not included in a review may be analysed), this has a limited additional cost considering that few files have the same basename inside a repository. Change-Id: Ib74289e201098d15c618791f47b2dd657c3a1744

Add sonar as a tool for sputnik in the readme title Change-Id: I53df772174a7b0478a992dc8a45c1cf844c8958b

As this configuration directive holds multiple configuration file paths it has been renamed configurationFiles. Change-Id: I33c4e37a8ac25a81868086a9bdd7d2c62685d517

Rename test and use TestEnvironment class as a base class for test in order to use helper methods. Change-Id: I413e2b5d536201dc59059b1fdc0a0c2c1073cc67

Add the rule name so the user can access the description of this rule to fix the issue Change-Id: Icff9bdde4e520f1b6e09925ea6e03d0cd6a718ef

Implement a sonar processor

SpOOnman · 2014-12-29T21:11:16Z

Thank you! Spread a word about your use cases with Sputnik :)

Implement a sonar processor

dopuskh3 mentioned this pull request Dec 5, 2014

Pull quality profiles from Sonarqube #36

Closed

dopuskh3 force-pushed the sonar branch 3 times, most recently from a15020b to 70ad26a Compare December 5, 2014 18:37

pjagielski reviewed Dec 5, 2014
View reviewed changes

dopuskh3 force-pushed the sonar branch 2 times, most recently from 7905a0f to 6ad34fe Compare December 8, 2014 09:23

dopuskh3 force-pushed the sonar branch 2 times, most recently from b9eff47 to 3692c5d Compare December 8, 2014 09:35

SpOOnman reviewed Dec 22, 2014
View reviewed changes

dopuskh3 force-pushed the sonar branch 2 times, most recently from 8e947e2 to 8cbe570 Compare December 23, 2014 09:58

Add Sonar dependency.

2841add

Change-Id: Iaf9b4565e8ac2237d5a4d70627487e6bbb4b86fc

Add a few options that will be used for sonar

b3072f0

Add a few configuration option for further sonar review. Change-Id: I82b92e291791e838b18b6c9281225ba44161cceb

dopuskh3 force-pushed the sonar branch from 8cbe570 to 421d5b6 Compare December 23, 2014 12:35

SpOOnman reviewed Dec 23, 2014
View reviewed changes

Francois Visconte and others added 9 commits December 23, 2014 16:53

Adds a class to parse Sonar results.

469506e

SonarResultParser parses json result and produces a ReviewResult. Change-Id: I11e2fc20567570343b20648f8d7ae8bc04d5940b

Adds a SonarProcessor class.

4d14106

Change-Id: I86633f86858efbe9aaea177d68492c1ff937273e

Test that sonar parser only reports new issues

4c04f6c

Add test fixtures that test sonar will only report new issues Change-Id: I47dd6d4c088620bd5120a07a155e75ba0687b808

Update README

437da18

Add sonar as a tool for sputnik in the readme title Change-Id: I53df772174a7b0478a992dc8a45c1cf844c8958b

Rename sonar configuration file key

63c02ba

As this configuration directive holds multiple configuration file paths it has been renamed configurationFiles. Change-Id: I33c4e37a8ac25a81868086a9bdd7d2c62685d517

Use TestEnvironment class and fix naming

ba8e29b

Rename test and use TestEnvironment class as a base class for test in order to use helper methods. Change-Id: I413e2b5d536201dc59059b1fdc0a0c2c1073cc67

Add the rule name to sonar sputnik comment

065db12

Add the rule name so the user can access the description of this rule to fix the issue Change-Id: Icff9bdde4e520f1b6e09925ea6e03d0cd6a718ef

dopuskh3 force-pushed the sonar branch from 5203743 to 065db12 Compare December 23, 2014 15:55

SpOOnman added a commit that referenced this pull request Dec 29, 2014

Merge pull request #54 from criteo-forks/sonar

09cce06

Implement a sonar processor

SpOOnman merged commit 09cce06 into TouK:master Dec 29, 2014

rufuslevi pushed a commit to rufuslevi/sputnik that referenced this pull request Mar 12, 2024

Merge pull request TouK#54 from criteo-forks/sonar

656232f

Implement a sonar processor

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a sonar processor #54

Implement a sonar processor #54

dopuskh3 commented Dec 5, 2014

pjagielski commented Dec 5, 2014

pjagielski Dec 5, 2014

pjagielski commented Dec 5, 2014

dopuskh3 commented Dec 8, 2014

SpOOnman commented Dec 9, 2014

damianszczepanik commented Dec 9, 2014

dopuskh3 commented Dec 10, 2014

cupcicm commented Dec 11, 2014

SpOOnman Dec 22, 2014

SpOOnman commented Dec 22, 2014

SpOOnman Dec 23, 2014

dopuskh3 Dec 23, 2014

SpOOnman commented Dec 23, 2014

SpOOnman commented Dec 29, 2014

Implement a sonar processor #54

Implement a sonar processor #54

Conversation

dopuskh3 commented Dec 5, 2014

pjagielski commented Dec 5, 2014

pjagielski Dec 5, 2014

Choose a reason for hiding this comment

pjagielski commented Dec 5, 2014

dopuskh3 commented Dec 8, 2014

SpOOnman commented Dec 9, 2014

damianszczepanik commented Dec 9, 2014

dopuskh3 commented Dec 10, 2014

cupcicm commented Dec 11, 2014

SpOOnman Dec 22, 2014

Choose a reason for hiding this comment

SpOOnman commented Dec 22, 2014

SpOOnman Dec 23, 2014

Choose a reason for hiding this comment

dopuskh3 Dec 23, 2014

Choose a reason for hiding this comment

SpOOnman commented Dec 23, 2014

SpOOnman commented Dec 29, 2014