-
Notifications
You must be signed in to change notification settings - Fork 688
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
[No Jira] Improve Autoscan testing #4520
Conversation
knownDiffs.put(diff.ruleKey, diff); | ||
} | ||
|
||
// store new unexpected diffs in JSON files - serializable |
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.
For easier debugging, we only output diffs that are different from the expected. There is no value in outputting diffs that are expected that way.
String differences = Files.readString(pathFor(TARGET_ACTUAL + PROJECT_KEY + "-no-binaries_differences")); | ||
softly.assertThat(differences).isEqualTo("Issues differences: 3473"); | ||
softly.assertThat(differences).isEqualTo("Issues differences: " + expectedDiffs); |
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'm fine with removing this assertion entirely (I don't see much value here that goes beyond the comparison of the actual and new diffs collections), however the modification is okay as well, as it doesn't need to be edited when diffs change.
softly.assertThat(newTotal).isEqualTo(knownTotal); | ||
softly.assertThat(rulesCausingFPs).hasSize(6); | ||
softly.assertThat(rulesNotReporting).hasSize(7); | ||
softly.assertThat(rulesSilenced).hasSize(74); |
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.
The vast majority of rules relying on semantics will be silenced by the absence of binaries. Nowadays, it seems to me that most of our rules that we write rely on semantics. I don't think having this check is very valuable in addition to the diff data, that already tells us if a rule is silenced.
softly.assertThat(rulesCausingFPs).hasSize(6); | ||
softly.assertThat(rulesNotReporting).hasSize(7); |
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 think it's okay to leave these two assertions because this should actually be unexpected in most cases and require some additional verification. It's changed very rarely.
e205853
to
b2a749b
Compare
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 to me. Thanks for the effort.
Let's merge this PR after the release
b2a749b
to
4e48757
Compare
|
* Split autoscan test results into files per rule, to avoid merge conflicts * Remove / optimize redundant assertions
* Split autoscan test results into files per rule, to avoid merge conflicts * Remove / optimize redundant assertions
* Split autoscan test results into files per rule, to avoid merge conflicts * Remove / optimize redundant assertions
Please ensure your pull request adheres to the following guidelines: