Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new test file that verifies the root CLI registers core commands and persistent flags, and that extension commands described by manifest files can be discovered and executed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Pull request overview
Adds unit tests for the a6 root command to increase coverage around root-level wiring, including persistent flags, core subcommand registration, and extension command discovery/execution.
Changes:
- Introduce tests asserting root command metadata and presence of global persistent flags.
- Add coverage for core subcommand wiring.
- Add an integration-style test that discovers an extension from the configured extensions directory and executes it through the root command.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } { | ||
| found, _, err := rootCmd.Find([]string{name}) | ||
| require.NoError(t, err) | ||
| assert.NotNil(t, found) |
There was a problem hiding this comment.
The core-subcommand loop doesn’t actually assert that the resolved command matches the expected subcommand name. As written, the check can succeed even if the subcommand isn’t registered (e.g., if the lookup falls back to the root command). Please assert the found command’s name/use (and that it isn’t the root command) for each entry to make this test meaningful.
| assert.NotNil(t, found) | |
| require.NotNil(t, found) | |
| assert.NotSame(t, rootCmd, found) | |
| assert.Equal(t, name, found.Name()) | |
| assert.True(t, strings.HasPrefix(found.Use, name)) |
| cmd, _, err := rootCmd.Find([]string{"hello"}) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, "extension", cmd.GroupID) | ||
| assert.Equal(t, "test extension", cmd.Short) |
There was a problem hiding this comment.
Local variable cmd shadows the imported github.com/api7/a6/pkg/cmd package name (used earlier in the function). Renaming the variable (e.g., foundCmd/extCmd) will avoid confusion and prevent accidental misuse in future edits.
| cmd, _, err := rootCmd.Find([]string{"hello"}) | |
| require.NoError(t, err) | |
| assert.Equal(t, "extension", cmd.GroupID) | |
| assert.Equal(t, "test extension", cmd.Short) | |
| foundCmd, _, err := rootCmd.Find([]string{"hello"}) | |
| require.NoError(t, err) | |
| assert.Equal(t, "extension", foundCmd.GroupID) | |
| assert.Equal(t, "test extension", foundCmd.Short) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/cmd/root/root_test.go (1)
19-23: Make the core wiring test hermetic by pinning extension dir
NewCmdRootloads extensions during construction. Without isolatingA6_CONFIG_DIRin this test, local/CI extension state can influence outcomes.Suggested patch
func TestNewCmdRoot_RegistersCoreCommandsAndFlags(t *testing.T) { + t.Setenv("A6_CONFIG_DIR", t.TempDir()) ios, _, _, _ := iostreams.Test() rootCmd := NewCmdRoot(&cmd.Factory{IOStreams: ios})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cmd/root/root_test.go` around lines 19 - 23, TestNewCmdRoot_RegistersCoreCommandsAndFlags is non-hermetic because NewCmdRoot reads extensions from A6_CONFIG_DIR; pin A6_CONFIG_DIR to a temporary empty directory before calling NewCmdRoot to isolate extension state. In the test (within TestNewCmdRoot_RegistersCoreCommandsAndFlags) use os.MkdirTemp to create an empty dir, set the env var A6_CONFIG_DIR to that path via os.Setenv, defer cleanup (os.RemoveAll) and restore the original env value after the test, then construct rootCmd with NewCmdRoot so the test no longer depends on local/CI extension files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cmd/root/root_test.go`:
- Around line 48-50: The test currently calls rootCmd.Find([]string{name}) and
only checks err and that found is non-nil; add an explicit assertion that the
matched command's name equals the requested name to ensure the correct command
was returned. After the existing require.NoError(t, err) and assert.NotNil(t,
found) lines, assert (or require) that found.Name() (or found.Name) equals the
name variable so the test fails if a different command was matched; reference
the rootCmd.Find call and the found variable when making this assertion.
---
Nitpick comments:
In `@pkg/cmd/root/root_test.go`:
- Around line 19-23: TestNewCmdRoot_RegistersCoreCommandsAndFlags is
non-hermetic because NewCmdRoot reads extensions from A6_CONFIG_DIR; pin
A6_CONFIG_DIR to a temporary empty directory before calling NewCmdRoot to
isolate extension state. In the test (within
TestNewCmdRoot_RegistersCoreCommandsAndFlags) use os.MkdirTemp to create an
empty dir, set the env var A6_CONFIG_DIR to that path via os.Setenv, defer
cleanup (os.RemoveAll) and restore the original env value after the test, then
construct rootCmd with NewCmdRoot so the test no longer depends on local/CI
extension files.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da81c874-ceac-4789-af47-4b4dcf639b9d
📒 Files selected for processing (1)
pkg/cmd/root/root_test.go
|
Addressed the hermeticity and command-matching feedback in a follow-up commit. I am not adding separate E2E coverage to this PR: this branch is intentionally limited to root-command wiring, while scenario-level E2E coverage is handled separately in PR #18 so the review scope stays clean. |
Summary
Validation
Refs #14
Summary by CodeRabbit