test: unified converter tests#1085
Conversation
And a cheeky fix to ensure we don't panic if we recieve malformed data with a shorter than expected dependency path.
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Summary of ChangesHello @rrama, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the robustness and test coverage of the unified converter logic. It introduces a new, extensive suite of AI-generated unit tests that validate the converter's functionality for building upgrade paths, generating remediation advice, and handling various dependency-related scenarios. Additionally, a critical defensive fix has been implemented to prevent application panics when encountering malformed dependency path data, ensuring more graceful error handling. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces a crucial fix in unified_converter.go to prevent panics when handling malformed dependency paths, significantly improving the robustness of the converter. Additionally, a comprehensive suite of unit tests has been added in unified_converter_test.go, which is excellent for validating the conversion logic and covering various edge cases. However, the new tests also highlight some existing "FIXME"s and logical "quirks" within the production code, particularly concerning the extractDependencyPath function's behavior and the IsUpgradable flag. Addressing these identified areas in future work would further enhance the accuracy and reliability of the unified converter.
| } | ||
|
|
||
| if len(dependencyPath) == 0 { | ||
| if len(dependencyPath) < 2 { |
There was a problem hiding this comment.
The change from len(dependencyPath) == 0 to len(dependencyPath) < 2 is a critical improvement. Accessing dependencyPath[1] when the slice has fewer than two elements would lead to a runtime panic. This fix correctly handles cases where the dependency path is empty or contains only a single element, preventing potential crashes.
| name: "Wrong behavior: multiple dependency paths - returns first one only (FIXME in prod code)", | ||
| // TODO: Delete this test and enable the test above when the fix has been implemented in the prod code. | ||
| finding: createFindingWithMultipleDependencyPaths(t, "goof@1.0.0", []string{"lodash@4.17.20"}, []string{"other-pkg@1.0.0"}), | ||
| expected: []string{"goof@1.0.0", "lodash@4.17.20"}, |
There was a problem hiding this comment.
This test case correctly identifies a "Wrong behavior" in the extractDependencyPath function, which currently only returns the first dependency path found, rather than all of them. The FIXME comment in the production code (unified_converter.go, line 333) also points to this. This limitation means that if a vulnerability is introduced through multiple dependency paths, the system might not fully represent all relevant information. It's important to address this in the extractDependencyPath function to ensure comprehensive vulnerability reporting.
| // Note: IsUpgradable is based on len(upgradePath) > 0, so [false] makes it true | ||
| // This is a quirk of the current implementation - might want to change to len(upgradePath) > 1 | ||
| assert.True(t, additionalData.IsUpgradable, "Current behavior: marked as upgradable when upgradePath=[false]") |
There was a problem hiding this comment.
The comment highlights a logical inconsistency where IsUpgradable is determined by len(upgradePath) > 0. If upgradePath contains only [false], it implies no actual upgrade is available, yet len(upgradePath) would be 1, making IsUpgradable true. This could lead to misleading information being presented to the user. Consider adjusting the logic to len(upgradePath) > 1 to accurately reflect when a true upgrade path exists.
Plus reduce `createFindingWithFixRelationship` and `createFindingWithMultipleUpgradePaths` into one function.
Update an incorrect comment about issues with fixed versions but without update paths in the code.
Remove the edge cases that were there just for the sake of padding out the tests. Remove the duplicate truncation checking test.
Also removed the test which lied in the name about testing a finding with no evidence. Added a skipped test for when the prod code is updated to return multiple findings.
Ensure clarity on scenarios and ensure we cover all expected scenarios.
These tests can not be enabled until the prod code is fixed.
This comment has been minimized.
This comment has been minimized.
|
No stale, just de-prioritised and no one has touched the code since for me to push it onto them. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
andrewrobinsonhodges-snyk
temporarily deployed
to
MT1
GitHub Actions
Inactive
andrewrobinsonhodges-snyk
temporarily deployed
to
MT3
GitHub Actions
Inactive
andrewrobinsonhodges-snyk
temporarily deployed
to
Standard
GitHub Actions
Inactive
PR Reviewer Guide 🔍
|
|
|
||
| // Create deps with WRONG types (strings instead of proper types) | ||
| deps := map[string]any{ | ||
| ctx2.DepConfig: "not-a-config", // Should be *config.Config |
There was a problem hiding this comment.
[Critical] This test file does not compile against main — it was written against an older API and not recompiled after merge. go vet ./infrastructure/oss/ fails, and because Go builds one test binary per package, this breaks the entire infrastructure/oss test suite: every test in the package fails to run, so the coverage this PR adds is currently zero. Confirmed undefined symbols:
ctx2.DepConfig(lines 705, 854) — the context package definesDepConfigResolver/DepConfiguration/DepEngine/…, there is noDepConfig.c.SetSnykOSSQuickFixCodeActionsEnabled(line 833) — exists nowhere in the repo.error_reporting.NewTestErrorReporter()(line 846) — signature isNewTestErrorReporter(workflow.Engine), called with 0 args.- The anonymous
Relationshipsstruct literals (lines ~988, ~1192) no longer matchtestapi.FindingData(Id fields areoapi-codegen/runtime/types.UUID, and aProjectrelationship was added).
Fix: rebuild the test against current APIs — use the real DI keys (DepEngine + ConfigResolverFromContext + FolderConfigFromContext), the real config-flag mechanism, NewTestErrorReporter(engine), and current generated types. Add go test ./infrastructure/oss/ to CI as a build gate — a green CI here would mean the package's tests aren't being compiled.
— AI review
|
|
||
| if len(dependencyPath) == 0 { | ||
| if len(dependencyPath) < 2 { | ||
| return nil |
There was a problem hiding this comment.
[Should Fix] Good fix here — but the identical bug class remains a few lines below at line 412:
if len(path) > 0 && path[1].Name == depPathPackageName {path[1] requires length ≥ 2, but the guard only checks len(path) > 0, so a single-element UpgradePath.DependencyPath (API data) panics with index-out-of-range — exactly what you just fixed for dependencyPath. Tighten to len(path) > 1, and add a test feeding a length-1 upgrade path (mirroring the single-element case you already added for the input side). 3 of 4 reviewers flagged this; no current test exercises it.
— AI review
|
Context for this review: the only prior review on this PR was from The source change is correct: Non-blocking follow-ups once the compile is fixed:
— AI review |
|
|
||
| // Create deps with WRONG types (strings instead of proper types) | ||
| deps := map[string]any{ | ||
| ctx2.DepConfig: "not-a-config", // Should be *config.Config |
There was a problem hiding this comment.
Critical — this test file does not compile against HEAD, so the entire infrastructure/oss test package fails to build. go test ./infrastructure/oss/ fails (this is also why CI lint is red and the unit/race/integration jobs are skipped — none of the ~1300 new lines of assertions have ever run). The file references an API that no longer exists at HEAD:
ctx2.DepConfig(here at line 705, and line 854) — undefined. The context package hasDepEngine,DepConfigResolver,DepConfiguration,DepLearnService,DepErrorReporter, but noDepConfig.config.Config(line ~818) — no such exported type inapplication/config.c.SetSnykOSSQuickFixCodeActionsEnabled(...)(line ~833) —cis theworkflow.Enginereturned bytestutil.UnitTestWithCtx, which has no such method.error_reporting.NewTestErrorReporter()(line ~846) — now requires aworkflow.Engineargument; see the sibling call invulnerability_count_test.go.- The hand-rolled anonymous
Relationshipsstruct literals (lines ~988, ~1192) no longer match the generatedtestapitype — it gained aProjectfield and usesoapi-codegen/runtime/types.UUIDforId, while the literals usegithub.com/google/uuid.UUIDand omitProject.
This needs a rebase onto current main and an update to the live API: obtain the engine/reporter the way the package's existing tests do, key deps by the real Dep* constants, and build Relationships from the generated testapi types rather than re-declaring the anonymous struct (the duplicated struct shape is the root cause of the last breakage and will keep breaking on every API change). Confirm go test ./infrastructure/oss/ passes before merge.
— AI review
| } | ||
|
|
||
| if len(dependencyPath) == 0 { | ||
| if len(dependencyPath) < 2 { |
There was a problem hiding this comment.
This guard fix is correct — dependencyPath[1] at line 407 would panic on a length-1 path, and < 2 cleanly rejects both empty and single-element inputs while preserving behavior for valid (len ≥ 2) inputs.
Should fix — the structurally identical twin panic just below is still unguarded. At line 412, path[1].Name is reached after only len(path) > 0, so an upgrade DependencyPath with exactly one package panics with index-out-of-range. That path slice comes from the API (upgradeAction.UpgradePaths[…].DependencyPath) and has no length invariant. Since this PR's theme is hardening this exact access pattern, tighten line 412 to len(path) > 1 to match, and add a test case with a single-element upgrade path — none of the new tests currently feed one, so the "avoid panic" coverage they imply doesn't actually exist.
— AI review
Reviewed Merge blocker (inline on the test file): Should fix (inline on Positives: the production change at line 404 is the correct root-cause panic fix and is behavior-preserving for valid inputs; the two comment corrections are accurate. Where the tests do target the production change ( Suggestions: a couple of skipped TODO tests (~120 lines) stage unbuilt "return all dependency paths" behavior — better tracked in the issue tracker and landed with that change; one Security: no findings introduced (the change is comment + bounds-guard; security-neutral-to-positive). — AI review |
5 participants
Description
AI took the tests from #1042 and updated them to work with #1054 which went into main. It took 2 days of the AI spinning in the background, I then took what it did and made the tests proper.
There is also a fix to ensure we don't panic if we receive malformed data with a shorter than expected dependency path.
Checklist
make generate)make lint-fix)