test(graphql): Add mocked tests for ImageComponent resolver#21542
test(graphql): Add mocked tests for ImageComponent resolver#21542rhybrillou wants to merge 3 commits into
Conversation
|
Skipping CI for Draft Pull Request. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds a new Go test file for GraphQL image component vulnerability queries. It introduces shared fixtures and mock helpers, then exercises fixable-CVE resolution, filter handling, invalid-component errors, and optional variable behavior. ChangesImage Component Vulnerability GraphQL Tests
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
central/graphql/resolvers/image_components_mocked_test.go (1)
210-292: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant repeated mock setup across subtests.
setupImageComponentCVEMocksis invoked in everyt.Runsubtest (lines 212, 229, 246, 262), re-registering identicalAnyTimes()expectations on the same controller each time. Functionally harmless but adds unnecessary boilerplate; consider calling once inSetupTest/before the subtests instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@central/graphql/resolvers/image_components_mocked_test.go` around lines 210 - 292, The `setupImageComponentCVEMocks` setup is being repeated in each `t.Run` subtest, which re-registers the same `AnyTimes()` expectations on the shared mock container. Move that common mock initialization out of the individual subtests in `image_components_mocked_test.go`—ideally into `SetupTest` or once before the subtests—so the `query without filters`, `query with vulnQuery filter`, `query with fixable filter`, and `query with combined filters` cases only focus on query-specific inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@central/graphql/resolvers/image_components_mocked_test.go`:
- Around line 210-292: The `setupImageComponentCVEMocks` setup is being repeated
in each `t.Run` subtest, which re-registers the same `AnyTimes()` expectations
on the shared mock container. Move that common mock initialization out of the
individual subtests in `image_components_mocked_test.go`—ideally into
`SetupTest` or once before the subtests—so the `query without filters`, `query
with vulnQuery filter`, `query with fixable filter`, and `query with combined
filters` cases only focus on query-specific inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 43f51f17-5421-4bac-98d2-a0cdff8b8225
📒 Files selected for processing (1)
central/graphql/resolvers/image_components_mocked_test.go
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #21542 +/- ##
==========================================
- Coverage 50.36% 50.32% -0.05%
==========================================
Files 2844 2844
Lines 218429 218429
==========================================
- Hits 110004 109916 -88
- Misses 100445 100515 +70
- Partials 7980 7998 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 Build Images ReadyImages are ready for commit 475c4c2. To use with deploy scripts: export MAIN_IMAGE_TAG=4.12.x-395-g475c4c2040 |
vikin91
left a comment
vikin91
left a comment
There was a problem hiding this comment.
I am not a graphql expert, but gave it a shot anyway. If you need a deeper review, then I would suggest to involve an additional reviewer.
Solid addition of working test coverage. I have few observations in the comments, but none of them are blocking.
|
|
||
| // region helper mock types | ||
|
|
||
| type flatComponentV2 struct { |
There was a problem hiding this comment.
AI observation:
The file hand-rolls flatComponentV2 and flatCVEV2 structs to implement the imagecomponentflat.ComponentFlat and imagecveflat.CveFlat interfaces. But gomock-generated imageComponentFlatViewMocks.NewMockComponentFlat and imageCVEFlatViewMocks.NewMockCveFlat already exist and are used for exactly this purpose in the same package.
| // TestGraphQLVariableTypes validates that the GraphQL engine properly handles different variable types | ||
| func (s *ImageComponentVulnerabilitiesGraphQLTestSuite) TestGraphQLVariableTypes() { |
There was a problem hiding this comment.
nit: this test name suggests that it validates that the GraphQL engine properly handles different variable types, but there are only two subtests with rather focused scope.
Okay to keep as it is if you have future plans for this test.
| type cveResponse struct { | ||
| CVE string `json:"cve"` | ||
| CVSS float64 `json:"cvss"` | ||
| Severity string `json:"severity"` | ||
| FixedByVersion string `json:"fixedByVersion"` | ||
| } | ||
|
|
||
| type imageComponentResponse struct { | ||
| Vulnerabilities []cveResponse `json:"vulnerabilities"` | ||
| } | ||
|
|
||
| type queryResponse struct { | ||
| Result imageComponentResponse `json:"result"` | ||
| } |
There was a problem hiding this comment.
nit: we are in package resolvers and those structs will be also visible to the non-test code in the same package. Their names suggest nothing about them being test helpers. There are no conflicts currently, but using names that don't suggest test application may be a bit confusing to developers (actually no one now uses IDE to manually pick types from the auto-suggest lists, so this can be degraded to a super-nit comment).
| assert.NotEmpty(t, response.Errors, "Query with invalid ID should produce errors") | ||
|
|
||
| if len(response.Errors) > 0 { | ||
| t.Logf("Expected error: %s", response.Errors[0].Error()) |
There was a problem hiding this comment.
Just log, no assertion here?
rhybrillou
force-pushed
the
master-yann/graphql/image_component_tests
branch
from
3c9dd40 to
475c4c2
Compare
2 participants
Description
Multiple dependabot pull-requests to upgrade the github.com/graph-gophers/graphql-go failed after the dependency introduced a breaking behaviour change ( see graph-gophers/graphql-go#763 ).
This PR introduces a set of unit tests that helped with the investigation of the upgrade failures.
The pull request is part of a stack:
User-facing documentation
is updated ORupdate is not neededis created and is linked above ORis not neededTesting and quality
Automated testing
How I validated my change
Test-only -> CI run