-
Notifications
You must be signed in to change notification settings - Fork 578
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
(SARIF) move exit code description and change behaviour of success flag #2740
(SARIF) move exit code description and change behaviour of success flag #2740
Conversation
Added a small test that verifies a successful execution even when bugs were found. 🙂 |
@@ -416,6 +416,25 @@ void testCweTaxonomy() throws IOException { | |||
is("Deserialization of Untrusted Data")); | |||
} | |||
|
|||
@Test | |||
void testSuccessfulExecution() { |
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.
Could you please rename this test so it's better describes what it tests exactly?
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.
I made the test name more expressive in my newest commit, please check it again
@@ -34,7 +35,7 @@ class Invocation { | |||
JsonObject toJsonObject() { | |||
JsonObject result = new JsonObject(); | |||
result.addProperty("exitCode", exitCode); | |||
result.addProperty("exitSignalName", exitSignalName); | |||
result.addProperty("exitSignalName", exitCodeDescription); |
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.
@CodingDepot I'm confused here, why change part of the property name but leave it as exitSignalName? I'm not seeing the purpose of the name change as a result here. Its a bit confusing to me now. Can you explain?
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.
No, you are completely right... I was being sloppy with the tacked on fix, I don't know how I missed that.
Upon review I found two more places where I failed to use the new property. :/
Thanks for having a closer look.
# Conflicts: # CHANGELOG.md
spotbugs/src/main/java/edu/umd/cs/findbugs/sarif/SarifBugReporter.java
Outdated
Show resolved
Hide resolved
@CodingDepot Can you fix merge conflicts here? |
# Conflicts: # spotbugs-tests/src/test/java/edu/umd/cs/findbugs/sarif/SarifBugReporterTest.java
This PR makes changes to the
SarifBugReporter
in order to get it more in line with the official SARIF specification.This includes the following Issues:
exitSignalName
. This property SHALL be absent when the process did not exit due to a signal. A better place for this information would beexitCodeDescription
.Closes (SARIF)
exitSignalName
information should be moved toexitCodeDescription
#2739 .This goes against the intent of the
executionSuccessful
property as specified by the format.This should close Sarif report and SuppressFBWarnings #2116 .
I considered adding the idea from #1918 (setting
executionSuccessful
to false when bugs were found AND thefailOnError
flag is set) but I could not find an easy link from the Reporter to the Task containing the flag value.Make sure these boxes are checked before submitting your PR -- thank you!
CHANGELOG.md
if you have changed SpotBugs code