validate: use golden yaml for expected status - #500
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/*.go📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Files:
**/*_test.go📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Files:
🧠 Learnings (7)📓 Common learnings📚 Learning: 2026-05-12T22:11:16.268ZApplied to files:
📚 Learning: 2026-05-20T20:00:29.810ZApplied to files:
📚 Learning: 2026-05-20T23:18:04.966ZApplied to files:
📚 Learning: 2026-05-20T23:17:29.721ZApplied to files:
📚 Learning: 2026-05-20T18:45:08.052ZApplied to files:
📚 Learning: 2026-05-12T19:36:08.290ZApplied to files:
🔇 Additional comments (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe validation tests replace large inline expected status structures with YAML golden fixtures. Test helpers load and unmarshal application and cluster reports. Unused imports, variables, and constants are removed. ChangesGolden YAML validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 (2)
pkg/validate/application/application_test.go (1)
168-179: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fixture loaders omit the fixture name in failure messages. Both helpers accept a fixture name but report
ReadFile() errorandUnmarshal() errorwithout it. A failing test does not identify which fixture is broken, andloadClustersStatusserves two fixtures.
pkg/validate/application/application_test.go#L168-L179: bind the joined path to a variable and include it in botht.Fatalfmessages.pkg/validate/clusters/clusters_test.go#L152-L163: apply the same change sok8s_status.yamlandocp_status.yamlfailures are distinguishable.🤖 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 `@pkg/validate/application/application_test.go` around lines 168 - 179, Update loadApplicationStatus in pkg/validate/application/application_test.go at lines 168-179 to store the joined fixture path and include it in both ReadFile and Unmarshal failure messages. Apply the same change to loadClustersStatus in pkg/validate/clusters/clusters_test.go at lines 152-163 so failures identify whether k8s_status.yaml or ocp_status.yaml is affected.pkg/validate/application/testdata/passed_status.yaml (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe golden fixtures carry no provenance comment. All three files are generated from resources in
pkg/testdata/and must be regenerated when those resources change. Nothing in the files states this. Issue#422names this maintenance requirement. YAML comments cost nothing at load time.
pkg/validate/application/testdata/passed_status.yaml#L1-L1: add a header comment namingTestValidateApplicationPassedand the source resources.pkg/validate/clusters/testdata/k8s_status.yaml#L1-L1: add a header comment namingTestValidateClustersK8sandpkg/testdata/clusters/k8s.pkg/validate/clusters/testdata/ocp_status.yaml#L1-L1: add a header comment namingTestValidateClustersOcpandpkg/testdata/clusters/ocp.🤖 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 `@pkg/validate/application/testdata/passed_status.yaml` at line 1, Add provenance header comments to pkg/validate/application/testdata/passed_status.yaml at lines 1-1 naming TestValidateApplicationPassed and its source resources; add a header to pkg/validate/clusters/testdata/k8s_status.yaml at lines 1-1 naming TestValidateClustersK8s and pkg/testdata/clusters/k8s; and add a header to pkg/validate/clusters/testdata/ocp_status.yaml at lines 1-1 naming TestValidateClustersOcp and pkg/testdata/clusters/ocp.
🤖 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 `@pkg/validate/application/application_test.go`:
- Around line 168-179: Update loadApplicationStatus in
pkg/validate/application/application_test.go at lines 168-179 to store the
joined fixture path and include it in both ReadFile and Unmarshal failure
messages. Apply the same change to loadClustersStatus in
pkg/validate/clusters/clusters_test.go at lines 152-163 so failures identify
whether k8s_status.yaml or ocp_status.yaml is affected.
In `@pkg/validate/application/testdata/passed_status.yaml`:
- Line 1: Add provenance header comments to
pkg/validate/application/testdata/passed_status.yaml at lines 1-1 naming
TestValidateApplicationPassed and its source resources; add a header to
pkg/validate/clusters/testdata/k8s_status.yaml at lines 1-1 naming
TestValidateClustersK8s and pkg/testdata/clusters/k8s; and add a header to
pkg/validate/clusters/testdata/ocp_status.yaml at lines 1-1 naming
TestValidateClustersOcp and pkg/testdata/clusters/ocp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 90be598f-fb4c-4c30-9195-23fac7c4c8bc
📒 Files selected for processing (7)
pkg/validate/application/application_test.gopkg/validate/application/command_test.gopkg/validate/application/testdata/passed_status.yamlpkg/validate/clusters/clusters_test.gopkg/validate/clusters/command_test.gopkg/validate/clusters/testdata/k8s_status.yamlpkg/validate/clusters/testdata/ocp_status.yaml
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
**/*.go: Use proper punctuation in comments - end sentences with periods
All Go files need SPDX license headers - check existing files for the format
Check existing code for error formatting conventions in Go
Files:
pkg/validate/application/application_test.gopkg/validate/clusters/clusters_test.gopkg/validate/application/command_test.gopkg/validate/clusters/command_test.go
**/*_test.go
📄 CodeRabbit inference engine (.cursor/rules/project.mdc)
Use
helpers.FakeTime(t)for time-dependent tests to ensure reproducibility in Go tests
Files:
pkg/validate/application/application_test.gopkg/validate/clusters/clusters_test.gopkg/validate/application/command_test.gopkg/validate/clusters/command_test.go
🔇 Additional comments (6)
pkg/validate/application/application_test.go (1)
13-14: LGTM!pkg/validate/application/command_test.go (1)
127-129: LGTM!pkg/validate/clusters/clusters_test.go (1)
13-14: LGTM!pkg/validate/clusters/testdata/ocp_status.yaml (1)
279-283: LGTM!pkg/validate/clusters/command_test.go (1)
71-72: LGTM!Also applies to: 117-118
pkg/validate/application/testdata/passed_status.yaml (1)
93-94: 📐 Maintainability & Code QualityKeep
secondaryCluster.vrg.lastGroupSyncTimewithoutvalue— the previous inline expectation also set onlystate: ok, so the fixture does not drop data.> Likely an incorrect or invalid review comment.
| t.Helper() | ||
| data, err := os.ReadFile(filepath.Join("testdata", name)) | ||
| if err != nil { | ||
| t.Fatalf("ReadFile() error: %v", err) |
There was a problem hiding this comment.
Check if the error includes the file name. If not we want to include it in the error message. Can be tested by modifying the name (e.g. name+".missing").
| @@ -0,0 +1,102 @@ | |||
| hub: | |||
There was a problem hiding this comment.
Add a comment to the file explaining how it is generated and when we need to update it.
| } | ||
| // TEMPORARY — delete after generating the file | ||
| dumped, _ := yaml.Marshal(expectedStatus) | ||
| os.WriteFile("testdata/passed_status.yaml", dumped, 0644) |
There was a problem hiding this comment.
This is a good way to generate the golden files, but we cannot leave this code like this. Every commit must be correct and clean - no temporary code we going to remove in the next commit.
You can share this code in the commit messge, explaining how the golden file was geneated.
For this commit, removing the code for generating the golden file is enough.
There was a problem hiding this comment.
Let name this after the application tested - appset-deploy-rbd - like the testdata directory. In the future we will have more testdata and golden files.
| }, | ||
| }, | ||
| } | ||
| expected := loadClustersStatus(t, "ocp_status.yaml") |
There was a problem hiding this comment.
Replacing the huge struct with the yaml seems like the right way for this commit. Let's do the same for the application test.
| // TEMPORARY — delete after generating the file | ||
| dumped, _ := yaml.Marshal(expectedStatus) | ||
| os.WriteFile("testdata/passed_status.yaml", dumped, 0644) | ||
| expectedStatus := loadApplicationStatus(t, "passed_status.yaml") |
There was a problem hiding this comment.
Lets squash this commit into the first commit - this is the right way to replace the literal struct with golden file - no need for a partial commit adding a golden file without removing the literal struct.
|
Thanks for the detailed review, @nirs! I'll address all of these , will push updates shortly!! |
6b97a92 to
35bf2c3
Compare
Replace the large inline report.ApplicationStatus and
report.ClustersStatus struct literals in
validate/application/command_test.go and
validate/clusters/command_test.go with golden yaml fixtures loaded
from testdata/, via new loadApplicationStatus/loadClustersStatus
test helpers. This matches the pattern already used by html_test.go
in both packages.
Golden files were generated by temporarily adding, right after each
struct literal:
dumped, err := yaml.Marshal(expected)
if err != nil {
t.Fatalf("Marshal() error: %v", err)
}
if err := os.WriteFile("testdata/<name>.yaml", dumped, 0644); err != nil {
t.Fatalf("WriteFile() error: %v", err)
}
running the relevant test once to produce the file, then removing
this code and replacing the struct literal with a call to the
loader helper.
Fixes RamenDR#422
35bf2c3 to
d7ba5db
Compare
|
@nirs Addressed all review comments: filename in error messages, golden file comments, removed temp generator code, renamed files to match convention (appset-deploy-rbd.yaml, k8s-status.yaml, ocp-status.yaml), and squashed everything into a single clean commit. Also fixed 6 lint issues (import ordering + unused vars left over from the removed literals) that were causing the Lint check to fail. All green locally: go build, go test ./..., and golangci-lint run ./.... , |
| # TestValidateApplicationPassed in command_test.go. | ||
| # | ||
| # Generated from the report.ApplicationStatus struct that was previously | ||
| # hardcoded in the test, by temporarily marshaling it to yaml and writing |
There was a problem hiding this comment.
No need to detail how it used to work ("previously hardcoded in the test")
| # TestValidateClustersK8s in command_test.go. | ||
| # | ||
| # Generated from the report.ClustersStatus struct that was previously | ||
| # hardcoded in the test, by temporarily marshaling it to yaml and writing |
| # TestValidateClustersOcp in command_test.go. | ||
| # | ||
| # Generated from the report.ClustersStatus struct that was previously | ||
| # hardcoded in the test, by temporarily marshaling it to yaml and writing |
| } | ||
| s := &report.ApplicationStatus{} | ||
| if err := yaml.Unmarshal(data, s); err != nil { | ||
| t.Fatalf("Unmarshal() error: %v", err) |
There was a problem hiding this comment.
| t.Fatalf("Unmarshal() error: %v", err) | |
| t.Fatalf("Unmarshal(%q) error: %v", name, err) |
| } | ||
| s := &report.ClustersStatus{} | ||
| if err := yaml.Unmarshal(data, s); err != nil { | ||
| t.Fatalf("Unmarshal() error: %v", err) |
There was a problem hiding this comment.
| t.Fatalf("Unmarshal() error: %v", err) | |
| t.Fatalf("Unmarshal(%q) error: %v", name, err) |
| # Golden file for the expected report.ApplicationStatus produced by | ||
| # TestValidateApplicationPassed in command_test.go. | ||
| # | ||
| # Generated from the report.ApplicationStatus struct that was previously | ||
| # hardcoded in the test, by temporarily marshaling it to yaml and writing | ||
| # it to this path. See the commit message for the exact steps used. | ||
| # | ||
| # If the test fixtures under pkg/testdata/appset-deploy-rbd/ change, | ||
| # this file must be regenerated to match. |
There was a problem hiding this comment.
The useful parts are only:
- which test uses the file
- which pkg/testdata/... tree it tracks
- regenerate when that tree changes
| # Golden file for the expected report.ClustersStatus produced by | ||
| # TestValidateClustersK8s in command_test.go. | ||
| # | ||
| # Generated from the report.ClustersStatus struct that was previously | ||
| # hardcoded in the test, by temporarily marshaling it to yaml and writing | ||
| # it to this path. See the commit message for the exact steps used. | ||
| # | ||
| # If the test fixtures under pkg/testdata/clusters/k8s/ change, this | ||
| # file must be regenerated to match. |
There was a problem hiding this comment.
The useful parts are only:
- which test uses the file
- which pkg/testdata/... tree it tracks
- regenerate when that tree changes
| # Golden file for the expected report.ClustersStatus produced by | ||
| # TestValidateClustersOcp in command_test.go. | ||
| # | ||
| # Generated from the report.ClustersStatus struct that was previously | ||
| # hardcoded in the test, by temporarily marshaling it to yaml and writing | ||
| # it to this path. See the commit message for the exact steps used. | ||
| # | ||
| # If the test fixtures under pkg/testdata/clusters/ocp/ change, this | ||
| # file must be regenerated to match. |
There was a problem hiding this comment.
The useful parts are only:
- which test uses the file
- which pkg/testdata/... tree it tracks
- regenerate when that tree changes
4 participants
What
Converts the large inline
report.ApplicationStatusandreport.ClustersStatusstruct literals used inpkg/validate/application/command_test.goandpkg/validate/clusters/command_test.gointo golden yaml fixturesunder
testdata/, loaded via newloadApplicationStatus/loadClustersStatustest helpers.This follows the same golden-file pattern already used by
html_test.goin both packages.Why
The inline struct literals were ~180–570 lines each, making the tests hard to read and maintain. The yaml equivalents are
significantly shorter and easier to edit by hand.
Testing
go build ./...go test ./...Fixes #422