-
Notifications
You must be signed in to change notification settings - Fork 7
Handle comma-separated filename inputs for Sonar issues JSON and Sonar hotspots JSON #365
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
Conversation
drdavella
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Even though --sonar-hotspots-json is just a pass-thru without implementation I think we should also update the help message to indicate a comma-separated input.
framework/codemodder-base/src/main/java/io/codemodder/CodemodLoader.java
Outdated
Show resolved
Hide resolved
| } catch (IOException e) { | ||
| throw new UncheckedIOException("Problem reading Sonar issues JSON file", e); | ||
| } | ||
| if (issuesFiles != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code makes sense to me but I also want to be careful that we don't introduce any regressions here. Would you be able to add a unit test or do you feel like this is sufficiently covered by existing tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drdavella
When we run a codemod test that is not a sonar codemod, this sonar module is being executed because all modules (defectdojo, semgrep, codeql, sonar, etc) are being configured at CodemodLoader.java. So, in when the non-sonar codemod test is being executed, this sonar module handles the empty/null issuesFiles because they're not required for that codemod (defectdojo, semgrep, etc).
So yes, this case is already covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drdavella In this commit be04cc0 I have updated our tests to support multiple sonar issues json files and refactor one existing sonar test
|
| .collect(Collectors.toList()); | ||
|
|
||
| if (sonarJsonsPaths.isEmpty()) { | ||
| Path defaultPath = testResourceDir.resolve("sonar-issues.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I thought I left this comment before but I don't see it. Shouldn't it be this now?
| Path defaultPath = testResourceDir.resolve("sonar-issues.json"); | |
| Path defaultPath = testResourceDir.resolve("sonar-issues_1.json"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drdavella no, default sonar issues filename remains the same as it previously was, otherwise, i will have to rename all other sonar codemod test json files

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, I'm sorry. I missed that the one you renamed was for a specific test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes




Issue: #364
Example of running a list of sonar findings files:
Sonar hotspot CLI input was the only update since there's no implementation for it yet