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

Added SonarQube parsers #67

Merged
merged 1 commit into from Jul 10, 2018
Merged

Conversation

CarlesCapdevila
Copy link
Contributor

Added two parsers for SonarQube as in the Warnings plugin:

  • SonarQube Issues -> Used to parse issues retrieved from the SonarQube api (api/issues/search)
  • SonarQube Diff -> Used to parse new issues retrieved from a SonarQube differential analysis

Some things that didn't go as expected and which made that the tests had to be changed:

  • Line numbers below 0 (-1 in my case) are clipped to 0.
  • Expected 32 issues and got 31; think that's because there was one which was nearly identical to other one and got filtered out of the report. Is that the case?

Thanks for your attention :)

@codecov-io
Copy link

codecov-io commented Jul 9, 2018

Codecov Report

Merging #67 into master will decrease coverage by 0.15%.
The diff coverage is 80.76%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #67      +/-   ##
============================================
- Coverage     87.74%   87.58%   -0.16%     
- Complexity     1057     1083      +26     
============================================
  Files           140      143       +3     
  Lines          3451     3529      +78     
  Branches        359      374      +15     
============================================
+ Hits           3028     3091      +63     
- Misses          293      298       +5     
- Partials        130      140      +10
Impacted Files Coverage Δ Complexity Δ
.../hafner/analysis/parser/SonarQubeIssuesParser.java 75% <75%> (ø) 2 <2> (?)
...hm/hafner/analysis/parser/SonarQubeDiffParser.java 77.77% <77.77%> (ø) 3 <3> (?)
...edu/hm/hafner/analysis/parser/SonarQubeParser.java 81.96% <81.96%> (ø) 21 <21> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b081b3...1ec6e8e. Read the comment docs.

@CarlesCapdevila
Copy link
Contributor Author

Didn't had time to check out how to put the default pattern and icon, could you maybe please take care of that? :'D

Maybe put these icons from the SQ scanner plugin?
and this default pattern: "**/target/sonar/sonar-report.json"?

@uhafner
Copy link
Member

uhafner commented Jul 10, 2018

Thanks, I will add the pattern and icons by myself...

@uhafner
Copy link
Member

uhafner commented Jul 10, 2018

To answer your questions:

  1. a line number must be a positive number, a zero indicates an issue for the whole file (i.e. a line number is not available or makes no sense). What is semantics of negative numbers in Sonar?
  2. The reason that one issue is marked as duplicate is because all properties are the same. Seems that some issues do not have a line number attribute set, so 2 issues are marked as equal (which are actually not).

Shouldn't the block

			"textRange": {
				"startLine": 162,
				"endLine": 162,
				"startOffset": 0,
				"endOffset": 109
			},

be considered as well? (At least in cases where lineNumber is not given...). An issue has the properties lineStart and end as well as columnStart and end.

@CarlesCapdevila
Copy link
Contributor Author

Very kind of you to check that and answer, thanks!
1.- It seems that it was me who arbitrarily put the -1 as default number, to make clear that there was no number.
2.- Yes, the "textRange" block should also be considered (only for SonarQubeIssuesParser). I didn't do it because AFAIK there's no "endLine" for the old warnings plugin and I didn't know if a change in behaviour from the warnings plugin would be bad.

If there's no need for the parsers to have the same behaviour in the two plugins, this can be implemented in this one by adding SonarQubeParser.parseEnd() (like "parseStart()"), and then overriding parseStart() and parseEnd() on SonarQubeDiffParser and SonarQubeIssuesParser. This way we'd be solving both issues.

I understand that this is not really your task, so please let me know if you need my support :)

@uhafner uhafner merged commit 1ec6e8e into jenkinsci:master Jul 10, 2018
uhafner added a commit to jenkinsci/warnings-plugin that referenced this pull request Jul 10, 2018
@uhafner
Copy link
Member

uhafner commented Jul 10, 2018

I added the parser partially in the warnings plug-in now, see jenkinsci/warnings-plugin@d5fa787.

One thing that is not clear to me yet: how does a user decide which parser should be used? (Diff or Issues). Or should both parsers run on the same file and automatically only one of them reports issues? Or does a user really need to select one of the parsers?

The new warnings plugin supports both styles: we can have a composite parser that runs all parsers of one ID one after the other and aggregates the issues using one id. Or you can have two separate parsers that produce different results. What is the best way for your parsers?

@uhafner
Copy link
Member

uhafner commented Jul 10, 2018

I also merged you changes, otherwise I can't reference them in the other plug-in.

The change you made for the old warnings plugin is kind of obsolete, the new 3.0 branch will hopefully released in august. So it makes sense to use all of the properties of the new API. So if you find some more time a subsequent PR would be nice ;-)

@CarlesCapdevila
Copy link
Contributor Author

Cool, thank you! :)
Should be SonarQube instead of SonarCube though :'D if this is not relevant ignore it.

To answer your question:

  • Diff: This is used for SonarQube reports generated through a preview analysis. It only parses the newly introduced issues.
  • Issues: This is used for reports obtained through the SonarQube api ("/api/issues/search" at the time of writing)

In our use case we use each parser depending on the branch and analysis performed:

  • Topic branches use preview analysis so "Diff"
  • Main branches "master, maint..." use standard analysis and then retrieve the issues from the api, so "Issues"

Hopefully I can make the aforementioned changes and PR when I have time, nice to know the release date :) keep up the good work!

Will there be any release of the old warnings plugin containing the last parsers?

@uhafner
Copy link
Member

uhafner commented Jul 10, 2018

I added two issues, so we won't forget the open tasks:

@uhafner
Copy link
Member

uhafner commented Jul 10, 2018

In our use case we use each parser depending on the branch and analysis performed:
Topic branches use preview analysis so "Diff"
Main branches "master, maint..." use standard analysis and then retrieve the issues from the api, so "Issues"

I understand, but is it possible to detect which parser to use for which file? Then the user can just select "SonarQube". Otherwise the user needs to decide, which format to use...

@uhafner
Copy link
Member

uhafner commented Jul 10, 2018

Will there be any release of the old warnings plugin containing the last parsers?

I can make one if you can't wait for the new release...

uhafner added a commit that referenced this pull request Jul 10, 2018
uhafner added a commit to jenkinsci/warnings-plugin that referenced this pull request Jul 10, 2018
@CarlesCapdevila
Copy link
Contributor Author

In our use case we use each parser depending on the branch and analysis performed:
Topic branches use preview analysis so "Diff"
Main branches "master, maint..." use standard analysis and then retrieve the issues from the api, so "Issues"

I understand, but is it possible to detect which parser to use for which file? Then the user can just select "SonarQube". Otherwise the user needs to decide, which format to use...

It should be possible, by checking the most characteristic attributes of each report type. It feels kinda dirty to me though, it doesn't seem a 100% unmistakable thing because sometimes attributes are missing depending on the issue (like the line).

When I have some time I'll look into this, if that's OK with you :'D

@CarlesCapdevila
Copy link
Contributor Author

Will there be any release of the old warnings plugin containing the last parsers?

I can make one if you can't wait for the new release...

We only need the SQ parsers and we can build it locally for that, so I think there's no need to hurry for a new release. We'll survive ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants