🧪 crane validate offline mode : Automate test for malformed API surface file handling#592
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:
📝 WalkthroughWalkthroughAdds a tier1 Ginkgo e2e test for offline crane validation against malformed API surface JSON. It prepares a migration scenario, runs export/transform/apply, then validates eight malformed inputs and checks for failures without generating ChangesMTA-860 e2e test
Estimated code review effort: 2 (Simple) | ~15 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 |
Test Coverage ReportTotal: 47.8% 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/tier1/mta_860_validate_malformed_api_surface_test.go`:
- Around line 93-360: The eight malformed-JSON scenarios in this test are
implemented with repeated write-file, runner.Validate, and error assertions;
refactor them into a table-driven test loop. Use a slice of case structs (for
example with name, content, validateDir suffix, and expected error substrings)
or Ginkgo DescribeTable/Entry around the existing runner.Validate and
ValidateOptions usage, so each scenario only supplies its unique payload and
expectations. Keep the shared setup and assertions in one place and preserve the
per-case checks for report.json absence and substring matching on err.Error().
- Around line 162-192: Test Cases 3 and 6 only log the result and never assert
the expected behavior, so they pass unconditionally. Update the malformed JSON
scenarios around runner.Validate in the tier1 validate test to explicitly define
the intended outcome and add Expect assertions for it, rather than branching on
err and printing logs. Use the existing validation setup in the test cases to
verify either a required error or a required successful report, and make the
assertions consistent with the other cases.
🪄 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: 6fe770a1-2cfb-4cca-83c3-883166126133
📒 Files selected for processing (1)
e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go
…rface file handling Signed-off-by: Nandini Chandra <nachandr@redhat.com>
nachandr
force-pushed
the
860_offline_malformed_json
branch
from
13a9f28 to
7d249ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go (1)
181-218: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInner loop variable shadows outer
i.
for i, substr := range tc.errorSubstrings(line 215) shadows the outerifrom line 181. Harmless here sincetestNumis precomputed, but confusing to read.♻️ Proposed rename
- for i, substr := range tc.errorSubstrings { - matchers[i] = ContainSubstring(substr) + for j, substr := range tc.errorSubstrings { + matchers[j] = ContainSubstring(substr) }🤖 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/tier1/mta_860_validate_malformed_api_surface_test.go` around lines 181 - 218, The inner loop in the malformed API surface test is shadowing the outer loop index, which makes the validation block harder to read. In mta_860_validate_malformed_api_surface_test.go, rename the index variable used in the tc.errorSubstrings range loop inside the test case loop so it does not reuse the outer loop’s identifier from the same test case iteration. Keep the behavior the same and update the matcher construction in that block accordingly.
🤖 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/tier1/mta_860_validate_malformed_api_surface_test.go`:
- Around line 31-36: Remove the empty-context Skip guards in the tier1 malformed
API surface test so RUN_AS_ADMIN can execute the scenario. Update the validation
logic in mta_860_validate_malformed_api_surface_test to stop skipping when
scenario.SrcAppNonAdmin.Context or scenario.TgtAppNonAdmin.Context is empty, and
let the test rely on the admin override path instead.
- Around line 38-57: This test setup is using
SetupNamespaceAdminUsersForScenario, which is flagged by the run-as-admin
compliance check and still leaves the non-admin app contexts empty. Switch to
SetupActiveKubectlRunners to initialize the source and target kubectl runners,
and remove the empty-context skip guards on scenario.SrcAppNonAdmin and
scenario.TgtAppNonAdmin so the RUN_AS_ADMIN override can be applied
consistently.
---
Nitpick comments:
In `@e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go`:
- Around line 181-218: The inner loop in the malformed API surface test is
shadowing the outer loop index, which makes the validation block harder to read.
In mta_860_validate_malformed_api_surface_test.go, rename the index variable
used in the tc.errorSubstrings range loop inside the test case loop so it does
not reuse the outer loop’s identifier from the same test case iteration. Keep
the behavior the same and update the matcher construction in that block
accordingly.
🪄 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: 161d0417-f0c6-4908-b5a4-e6ca5da73db5
📒 Files selected for processing (1)
e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go
nachandr
force-pushed
the
860_offline_malformed_json
branch
from
7d249ab to
1a1f191
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go (2)
207-210: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInner loop variable shadows outer
i.The
for i, substr := range tc.errorSubstringsat Line 208 shadows the outerfor i, tc := range testCasesvariable (line 174). Harmless here sincetestNumis computed before the shadow, but confusing on read.♻️ Proposed rename
- matchers := make([]types.GomegaMatcher, len(tc.errorSubstrings)) - for i, substr := range tc.errorSubstrings { - matchers[i] = ContainSubstring(substr) - } + matchers := make([]types.GomegaMatcher, len(tc.errorSubstrings)) + for j, substr := range tc.errorSubstrings { + matchers[j] = ContainSubstring(substr) + }🤖 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/tier1/mta_860_validate_malformed_api_surface_test.go` around lines 207 - 210, The inner range variable in the matcher-building loop shadows the outer test-case index, making the nested loops harder to read. Rename the inner `i` used in the `for i, substr := range tc.errorSubstrings` loop in `Test...` to a distinct name like `j` or `idx`, and keep the rest of the matcher construction unchanged.
199-211: 🎯 Functional Correctness | 🔵 Trivial | ⚖️ Poor tradeoffSubstring assertions are largely tautological for unmarshal-failure cases.
Every case that reaches the
json.Unmarshalfailure path is wrapped as"parsing api-resources JSON from %q: %w"byParseAPIResourcesJSON. That wrapper text alone always contains the literal substrings"JSON"and"parsing"(which contains"parse"), so the per-caseerrorSubstringslists (e.g.,"invalid","syntax","EOF","character") are effectively redundant — theOr(...)matcher will pass via the wrapper text regardless of whether the specific inner error text changes or regresses. Consider asserting on the un-wrapped root error text (e.g., viaerrors.Unwrap/errors.As) or checking that a case-specific substring is present, to make these assertions actually discriminating.🤖 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/tier1/mta_860_validate_malformed_api_surface_test.go` around lines 199 - 211, The error-substring check in this test is too weak because ParseAPIResourcesJSON wraps all json.Unmarshal failures with a generic message, so the current Or(matchers...) can pass on wrapper text alone. Update the validation logic around tc.errorSubstrings in this test to inspect the underlying/root error from err (for example by unwrapping or using errors.As) and assert on a case-specific substring from that inner error instead of the wrapper, using the existing tc.errorSubstrings and errMsg/err handling.
🤖 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/tier1/mta_860_validate_malformed_api_surface_test.go`:
- Around line 116-124: The "Valid JSON but incorrect structure" case in the
malformed API surface test is asserting the wrong failure path: the fixture in
the ParseAPIResourcesJSON flow is still valid JSON, so it should hit the
empty-list error rather than an unmarshal error. Update this case to either
expect the wrapped "loading api-resources" plus "contains no API resource lists"
message, or change the fileContent in this test case so it actually produces a
JSON parse failure.
---
Nitpick comments:
In `@e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go`:
- Around line 207-210: The inner range variable in the matcher-building loop
shadows the outer test-case index, making the nested loops harder to read.
Rename the inner `i` used in the `for i, substr := range tc.errorSubstrings`
loop in `Test...` to a distinct name like `j` or `idx`, and keep the rest of the
matcher construction unchanged.
- Around line 199-211: The error-substring check in this test is too weak
because ParseAPIResourcesJSON wraps all json.Unmarshal failures with a generic
message, so the current Or(matchers...) can pass on wrapper text alone. Update
the validation logic around tc.errorSubstrings in this test to inspect the
underlying/root error from err (for example by unwrapping or using errors.As)
and assert on a case-specific substring from that inner error instead of the
wrapper, using the existing tc.errorSubstrings and errMsg/err handling.
🪄 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: e06a44ee-512c-4f96-b852-1879afccc5b0
📒 Files selected for processing (1)
e2e-tests/tests/tier1/mta_860_validate_malformed_api_surface_test.go
…rface file handling Signed-off-by: Nandini Chandra <nachandr@redhat.com>
nachandr
force-pushed
the
860_offline_malformed_json
branch
2 times, most recently
from
e654706 to
ea51f06
Compare
…rface file handling Signed-off-by: Nandini Chandra <nachandr@redhat.com>
nachandr
force-pushed
the
860_offline_malformed_json
branch
from
ea51f06 to
7339fa6
Compare
Contributor
Author
|
/ready-for-review |
msajidmansoori12
requested changes
Jul 2, 2026
| // Verify validation report was not created | ||
| reportPath := filepath.Join(validateDir, "report.json") | ||
| Expect(reportPath).NotTo(BeAnExistingFile(), "report.json should not be created with malformed JSON") | ||
| } else { |
Contributor
There was a problem hiding this comment.
Hi @nachandr ,
One small question on test intent: in the current cases, every entry sets expectError: true, so the success branch (else) is never exercised. Would you like this spec to stay strictly negative-path coverage? If yes, we could simplify by removing expectError/else to make intent explicit. If you want mixed coverage, maybe add one valid API-surface case so the success path is also tested.
Either direction works — mostly suggesting this for clarity and future maintenance.
Contributor
Author
There was a problem hiding this comment.
Hi @nachandr ,
One small question on test intent: in the current cases, every entry sets expectError: true, so the success branch (else) is never exercised. Would you like this spec to stay strictly negative-path coverage? If yes, we could simplify by removing expectError/else to make intent explicit. If you want mixed coverage, maybe add one valid API-surface case so the success path is also tested.
Either direction works — mostly suggesting this for clarity and future maintenance.
Hi @msajidmansoori12 ,
This test is strictly for malformed scenarios since the valid API surface scenarios are being covered by other offline tests. I have removed the success branch else block.
…rface file handling Signed-off-by: Nandini Chandra <nachandr@redhat.com>
nachandr
changed the title
nachandr
changed the title
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
2 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 #380
[MTA-860]offline validation when the API surface input file contains malformed or mis-shaped JSON..