🧪 Enhanced MTA-831 test to validate crane validate report in YAML format for offline mode - #543
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe MTA-831 e2e test is refactored to support both JSON and YAML ChangesYAML Serialization and Offline Validate Test Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 |
Test Coverage ReportTotal: 48.6% Per-package coverage
Full function-level detailsPosted by CI |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go`:
- Line 246: In the YAML success block logging statement around line 246, change
the field reference from report.APIResourcesSource to
reportYAML.APIResourcesSource to ensure consistency when logging the YAML report
details. The reportYAML.Mode field is being logged in the same statement, so the
APIResourcesSource field should also come from the reportYAML object to
accurately reflect the YAML report source information.
- Line 231: The YAML validation phase in the runValidateAndParseReport call is
using a different input directory parameter (validateDirYAML) than the JSON
validation phase, which defeats the parity check between the two formats. To fix
this, identify what input directory parameter was used in the JSON validation
call and use that same parameter in the YAML validation call on line 231 instead
of validateDirYAML, ensuring both validation phases operate on identical inputs
for proper format comparison.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc10f8eb-5a3e-486a-a7bc-391e2e04c36f
📒 Files selected for processing (1)
e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go
istein1
reviewed
Jun 17, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go (1)
203-247: ⚡ Quick winPrefer table-driven subcases for JSON/YAML validation paths.
These two format phases are nearly identical; table-driving them will reduce duplication and keep assertions in sync as coverage grows.
Refactor sketch
- By("Run crane validate in offline mode with output in JSON format") - validateDir := filepath.Join(paths.TempDir, "validate") - report, err := runValidateAndParseReport(runner, paths.OutputDir, validateDir, apiSurfaceFile, "json") - ... - By("Run crane validate in offline mode with output in YAML format") - validateDirYAML := filepath.Join(paths.TempDir, "validate-yaml") - reportYAML, err := runValidateAndParseReport(runner, paths.OutputDir, validateDirYAML, apiSurfaceFile, "yaml") - ... + cases := []struct { + format string + label string + validateDir string + }{ + {format: "json", label: "JSON", validateDir: filepath.Join(paths.TempDir, "validate")}, + {format: "yaml", label: "YAML", validateDir: filepath.Join(paths.TempDir, "validate-yaml")}, + } + for _, tc := range cases { + tc := tc + By("Run crane validate in offline mode with output in " + tc.label + " format") + report, err := runValidateAndParseReport(runner, paths.OutputDir, tc.validateDir, apiSurfaceFile, tc.format) + Expect(err).NotTo(HaveOccurred()) + Expect(report.APIResourcesSource).To(Equal(apiSurfaceFile)) + verifyCompatibleResources(report, namespace, tc.validateDir, tc.label) + }As per coding guidelines, "Use table-driven tests for multiple scenarios in Go test code."
🤖 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 `@e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go` around lines 203 - 247, The JSON and YAML validation blocks in this test are nearly identical with only the format parameter, directory names, and report variable names differing. Refactor this code to use a table-driven approach by creating a slice of structs containing the format type (json/yaml), directory path, and expected output label, then iterate through this table to execute the common validation logic once for each format instead of duplicating the runValidateAndParseReport, verifyCompatibleResources, and log.Printf calls. This will eliminate duplication and ensure both JSON and YAML validation paths remain in sync as the test coverage evolves.Source: Coding guidelines
🤖 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.
Inline comments:
In `@e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go`:
- Around line 230-235: The YAML validation test case in this section is missing
a parity assertion for the APIResourcesSource field. After the reportYAML is
parsed from the runValidateAndParseReport call, add an assertion to verify that
the APIResourcesSource in the YAML report matches the apiSurfaceFile variable,
similar to what is being done for JSON validation elsewhere in the test suite.
This ensures that YAML-specific regressions in offline source propagation are
caught and maintains test parity between JSON and YAML output formats.
---
Nitpick comments:
In `@e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go`:
- Around line 203-247: The JSON and YAML validation blocks in this test are
nearly identical with only the format parameter, directory names, and report
variable names differing. Refactor this code to use a table-driven approach by
creating a slice of structs containing the format type (json/yaml), directory
path, and expected output label, then iterate through this table to execute the
common validation logic once for each format instead of duplicating the
runValidateAndParseReport, verifyCompatibleResources, and log.Printf calls. This
will eliminate duplication and ensure both JSON and YAML validation paths remain
in sync as the test coverage evolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 00dc8eff-a0ff-43f9-a86f-3ee38008979b
📒 Files selected for processing (1)
e2e-tests/tests/tier0/mta_831_validate_compatible_resources_offline_test.go
nachandr
force-pushed
the
offline_yaml
branch
2 times, most recently
from
June 18, 2026 03:32
3d13b1b to
cb2d9d2
Compare
Contributor
Author
Added table-driven subcases for JSON/YAML validation paths. |
Contributor
|
just a thought, maybe it will be good to move the helper functions into the test_helpers.go file? it will require some changes, but maybe it will be worth it if its likely that those helpers will be helpful in future tests? |
Contributor
Author
I may need the helper functions in another test. So, I will make the changes and move the functions out to the test_helpers.go file in a follow up PR. |
aufi
reviewed
Jun 18, 2026
Contributor
There was a problem hiding this comment.
Overall looks good, but QE eyes from @stillalearner or @msajidmansoori12 could be better, so just providing what I've seen or got from claude review.
- Namespace change seems unrelated (line 121)
- namespace := appName
+ namespace := "multi-resource-831-offline"
Question: Why change the namespace from appName (which would be "multi-resource-app") to a hardcoded "multi-resource-831-offline"? This seems unrelated to adding YAML support.
Potential issue: If this namespace change is intentional, it should be documented in the PR description. If it's accidental, it should be reverted to maintain consistency.
- Missing import cleanup
The new code adds "gopkg.in/yaml.v3" but I don't see any removed imports. This is fine, but worth verifying that no unused imports remain.
- Test description could be more accurate (line 118)
It("[MTA-831] Generate and validate crane validate report in JSON format",
This description says "JSON format" but the test actually validates both JSON and YAML formats now. Should be:
It("[MTA-831] Generate and validate crane validate report in JSON and YAML formats",
nachandr
force-pushed
the
offline_yaml
branch
2 times, most recently
from
June 19, 2026 04:36
10706c2 to
cd472ab
Compare
Contributor
Author
Verified again that there are no unused imports. |
nachandr
force-pushed
the
offline_yaml
branch
2 times, most recently
from
June 19, 2026 17:57
449f249 to
f9cc24d
Compare
| @@ -0,0 +1,134 @@ | |||
| // Package utils provides utilities for e2e tests. | |||
Contributor
There was a problem hiding this comment.
i dont think utils_validate.go is needed here, can we please use the same utils.go file..
Contributor
Author
There was a problem hiding this comment.
Hi @stillalearner , thanks for your feedback. I'd like to clarify that utils_validate.go imports Ginkgo/Gomega and has test assertion helpers that use Expect()/By() directly, while utils.go is framework-agnostic and returns error values instead.
The VerifyValidateResults() fn defined in utils_validate.go will also be used by Automate validation of YAML report for live mode
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
Signed-off-by: Nandini Chandra <nachandr@redhat.com>
nachandr
force-pushed
the
offline_yaml
branch
2 times, most recently
from
July 14, 2026 15:54
e58415d to
f6ca20d
Compare
nachandr
changed the title
nachandr
changed the title
…t for offline mode Signed-off-by: Nandini Chandra <nachandr@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #544