deduplicate sarif outputs for GitHub #2333
Conversation
Co-authored-by: brabster <[email protected]>
Co-authored-by: brabster <[email protected]>
Co-authored-by: brabster <[email protected]>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
brabster
changed the title
|
Hmm not sure what to do about "Copilot" not having signed the CLA? I guess if this solution looks good I can go rewrite the history back to me |
| "github.com/google/osv-scanner/v2/pkg/models" | ||
| ) | ||
|
|
||
| func TestCreateSARIFFingerprint(t *testing.T) { |
There was a problem hiding this comment.
| func TestCreateSARIFFingerprint(t *testing.T) { | |
| func Test_createSARIFFingerprint(t *testing.T) { |
There was a problem hiding this comment.
also I think this test could do with some rework - rather than have a table test with the same inputs, and then a second (sub?) test that tests a bunch of other packages, I think we should have a single collection of table tests, and within each test we can run the function twice to do the deterministic test
Co-authored-by: Gareth Jones <[email protected]>
another-rex
left a comment
another-rex
left a comment
There was a problem hiding this comment.
Looks good to me, thanks for the contribution!
I think the current tests are mostly fine, as the second test is covering something completely separate from the first. Though can you move the second test into a separate Test_ function?
|
To make the snapshot tests pass, you need to run with |
- Moved "different inputs" test to separate Test_ function - Removed duplicate test case with same inputs - Added diverse test cases covering different scenarios - Each test verifies determinism by calling function twice - All tests still pass Co-authored-by: brabster <[email protected]>
- Uses nested loops to generate all combinations of test dimensions - Tests 2 vulnIDs × 2 artifactPaths × 4 packages = 16 combinations - More maintainable: easy to add new test values without manual enumeration - Better error messages showing both inputs when fingerprints collide - Verifies all combinations produce unique fingerprints Co-authored-by: brabster <[email protected]>
- Extracted common test parameters (vulnIDs, artifactPaths, packages) to package-level variables - Both tests now use the same parameter sets for consistency - Test_createSARIFFingerprint uses matrix approach to generate all 16 combinations - Test_createSARIFFingerprint_DifferentInputs uses same parameters to verify uniqueness - Easy to extend by modifying common parameter arrays - Ensures both tests cover the exact same input space Co-authored-by: brabster <[email protected]>
|
Thanks @another-rex!
I'm not sure what to do about Copilot's authorship and the CLA - if I need to rewrite the history and force push or something I can do that, let me know. |
3 participants
Fixes #2331
I did use Copilot coding agent to help me with this but I've tweaked and sense checked the output. I hope it's acceptable, let me know if there are any problems 🙏