test: add unit tests for OADP toolset#327
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds OADP/Velero CRDs to envtest and introduces OADP-focused unit and integration tests: a Toolset unit test suite and an MCP OADPSuite that enables CRDs, creates the test namespace, and validates the oadp-troubleshoot prompt, including dynamic-client Backup/Restore scenarios. ChangesOADP Toolset Test Coverage
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: shubham-pampattiwar The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/toolsets/oadp/toolset_test.go (1)
47-54: ⚡ Quick winSplit this subtest into single-behavior assertions.
This block verifies both argument count and per-argument optionality in one test case; split it into focused
s.Run()cases for clearer failures.Suggested refactor
- s.Run("prompt has three optional arguments", func() { + s.Run("prompt has three arguments", func() { t := &Toolset{} prompts := t.GetPrompts() args := prompts[0].Prompt.Arguments s.Len(args, 3) + }) + + s.Run("prompt arguments are optional", func() { + t := &Toolset{} + prompts := t.GetPrompts() + args := prompts[0].Prompt.Arguments for _, arg := range args { s.False(arg.Required) } })As per coding guidelines, "One assertion per test case - each
s.Run()block should ideally test one specific behavior using testify assertion methods."🤖 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/toolsets/oadp/toolset_test.go` around lines 47 - 54, The test combines two behaviors: verifying the number of arguments and verifying each argument is optional; split into two focused subtests using s.Run — one like "prompt has three arguments" that constructs t := &Toolset{}, calls prompts := t.GetPrompts(), and asserts s.Len(prompts[0].Prompt.Arguments, 3), and a separate subtest like "all prompt arguments are optional" that obtains args := prompts[0].Prompt.Arguments and iterates asserting s.False(arg.Required) for each; keep using the same Toolset and GetPrompts symbols and ensure each s.Run contains only the single focused assertion behavior.pkg/mcp/oadp_test.go (2)
87-87: ⚡ Quick winUse
s.Require().NoError(err)for must-succeed test stepsThese are success-path operations; prefer fail-fast assertions to stop the subtest immediately on error.
Suggested change
- s.NoError(err) + s.Require().NoError(err)As per coding guidelines, "
**/*.go: ... in tests uses.Require().NoError(err)for operations that must succeed ands.Error(err)ors.NoError(err)for testing error conditions".Also applies to: 117-117, 152-152, 162-162, 174-174
🤖 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/mcp/oadp_test.go` at line 87, Replace assertions that must fail-fast by changing s.NoError(err) to s.Require().NoError(err) for the success-path steps in this test; specifically update the s.NoError(err) calls (the one at the shown diff and the other occurrences referenced at lines 117, 152, 162, 174) so the test stops immediately on unexpected errors. Locate these calls in the oadp_test (look for s.NoError usages) and switch each to s.Require().NoError(err).
53-53: ⚡ Quick winResolve the toolset name at runtime instead of hardcoding it
Line 53 hardcodes
"oadp"when extendings.Cfg.Toolsets. Please resolve the name dynamically from the toolset/config source to avoid override/rebrand drift in tests.As per coding guidelines, "Never hardcode override-prone defaults (
ReadOnly,Toolsets,ToolOverrides,ListOutput, toolset names) - read them froms.Cfgor resolve them at runtime".🤖 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/mcp/oadp_test.go` at line 53, Replace the hardcoded "oadp" when appending to s.Cfg.Toolsets with a runtime lookup from the test config—call the config accessor that returns the toolset name (for example use s.Cfg.ToolsetName(), s.Cfg.Defaults.Toolset, or a provided ResolveToolset/Lookup method) and append that result to s.Cfg.Toolsets instead of the literal "oadp"; update the test in oadp_test.go to derive the toolset name from s.Cfg (use the existing config getter on s.Cfg rather than hardcoding).
🤖 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/mcp/oadp_test.go`:
- Line 87: Replace assertions that must fail-fast by changing s.NoError(err) to
s.Require().NoError(err) for the success-path steps in this test; specifically
update the s.NoError(err) calls (the one at the shown diff and the other
occurrences referenced at lines 117, 152, 162, 174) so the test stops
immediately on unexpected errors. Locate these calls in the oadp_test (look for
s.NoError usages) and switch each to s.Require().NoError(err).
- Line 53: Replace the hardcoded "oadp" when appending to s.Cfg.Toolsets with a
runtime lookup from the test config—call the config accessor that returns the
toolset name (for example use s.Cfg.ToolsetName(), s.Cfg.Defaults.Toolset, or a
provided ResolveToolset/Lookup method) and append that result to s.Cfg.Toolsets
instead of the literal "oadp"; update the test in oadp_test.go to derive the
toolset name from s.Cfg (use the existing config getter on s.Cfg rather than
hardcoding).
In `@pkg/toolsets/oadp/toolset_test.go`:
- Around line 47-54: The test combines two behaviors: verifying the number of
arguments and verifying each argument is optional; split into two focused
subtests using s.Run — one like "prompt has three arguments" that constructs t
:= &Toolset{}, calls prompts := t.GetPrompts(), and asserts
s.Len(prompts[0].Prompt.Arguments, 3), and a separate subtest like "all prompt
arguments are optional" that obtains args := prompts[0].Prompt.Arguments and
iterates asserting s.False(arg.Required) for each; keep using the same Toolset
and GetPrompts symbols and ensure each s.Run contains only the single focused
assertion behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 113020db-a9a9-4295-bf1d-7c844ef3ccfd
📒 Files selected for processing (3)
pkg/mcp/common_test.gopkg/mcp/oadp_test.gopkg/toolsets/oadp/toolset_test.go
shubham-pampattiwar
force-pushed
the
feature/oadp-toolset-tests
branch
from
a4fb74d to
6bee000
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/toolsets/oadp/toolset_test.go`:
- Around line 41-67: The tests access prompts[0] directly in multiple subtests
which can panic if GetPrompts() returns an empty slice; update each affected
subtest (those using prompts[0] in the "prompt is named oadp-troubleshoot",
"prompt has three arguments", and "prompt has a handler" cases) to first assert
the slice is non-empty (e.g., s.NotEmpty(prompts) or s.Len(prompts, X)) before
referencing prompts[0], so failures are reported as assertions instead of
panics; keep the checks around the same variable names (prompts, args) and the
Toolset.GetPrompts() call.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 828b0f45-d05a-4096-a041-2dae825528e3
📒 Files selected for processing (3)
pkg/mcp/common_test.gopkg/mcp/oadp_test.gopkg/toolsets/oadp/toolset_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/mcp/common_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/mcp/oadp_test.go
Add tests for the OADP toolset covering toolset registration (no tools, prompt exposed) and troubleshoot prompt handler (default/custom namespace, existing/non-existent backup and restore scenarios). Register OADP/Velero CRDs in envtest setup. Follow-up to PR openshift#122 per reviewer request.
shubham-pampattiwar
force-pushed
the
feature/oadp-toolset-tests
branch
from
6bee000 to
16888b2
Compare
|
@shubham-pampattiwar: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Summary
Adds unit tests for the OADP toolset as requested in #122.
Unit tests (pkg/toolsets/oadp/toolset_test.go)
Integration tests (pkg/mcp/oadp_test.go)
Following the kubevirt/kiali testing pattern:
Other changes
Test plan
go test -run TestToolsetSuite ./pkg/toolsets/oadp/-- 8/8 passgo test -run TestOADPSuite ./pkg/mcp/-- 9/9 passSummary by CodeRabbit