-
Notifications
You must be signed in to change notification settings - Fork 346
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
test: add cases for output functions #937
Conversation
1b960fb
to
debe3ad
Compare
This is ready for review but #938 should be merged first. Note that beyond the usual review of the technical implementation, reviewers should also review the test cases to make sure they're correct for the real world and are complete, and also review the snapshots for each outputs for any oddentities that we might want to change (for which dedicated issues should be created - they won't be addressed in this PR) |
3d16a73
to
75150a5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #937 +/- ##
==========================================
+ Coverage 63.71% 63.94% +0.22%
==========================================
Files 146 146
Lines 11958 11958
==========================================
+ Hits 7619 7646 +27
+ Misses 3875 3854 -21
+ Partials 464 458 -6 ☔ View full report in Codecov by Sentry. |
This stabilizes the order in the SARIF output so that it is deterministic, as #937 proved it was not.
fwiw the coverage output for this looks even better after #961 is landed, as it means we've got 100% coverage across most files in the package 😄 |
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.
LGTM, thanks!
Update test snapshots after merging #937 . Also seems to cleanup old remediation in_place_test snapshots
This stabilizes the order in the SARIF output so that it is deterministic, as google#937 proved it was not.
These got included in google#938 even though they're for google#937 and it seems that `go-snaps` does not error about this even though it will clean them up if run with `UPDATE_SNAPS=clean`.
This introduces a set of crafted scanner results that each supported `output` format is run through to showcase how they look across all the different results possible from a scanner run - it originally started life as the tests for google#889 but I realised they could base used more generally for testing and reviewing all the outputters, so here we are. ~It looks like this has also revealed the SARIF output is unstable in its ordering, which I'll aim to address in a dedicated PR~
Update test snapshots after merging google#937 . Also seems to cleanup old remediation in_place_test snapshots
This introduces a set of crafted scanner results that each supported
output
format is run through to showcase how they look across all the different results possible from a scanner run - it originally started life as the tests for #889 but I realised they could base used more generally for testing and reviewing all the outputters, so here we are.It looks like this has also revealed the SARIF output is unstable in its ordering, which I'll aim to address in a dedicated PR