test(cmd): fix preRun tests to call preRun directly#60
Merged
feloy merged 1 commit intokortex-hub:mainfrom Mar 12, 2026
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughRefactors command tests to favor direct invocation of preRun handlers in unit tests and expands AGENTS.md with explicit examples and guidance for both preRun unit tests and E2E tests. Adjusts workspace list/remove tests to wire Cobra flags locally and call preRun(cmd, args) directly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 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. 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 |
benoitf
approved these changes
Mar 11, 2026
jeffmaury
approved these changes
Mar 11, 2026
The preRun tests in workspace_list_test.go and workspace_remove_test.go were incorrectly calling rootCmd.Execute() instead of testing the preRun method directly. This mixed Cobra wiring with unit test logic. Updated tests to follow the pattern from init_test.go by creating command struct instances and calling c.preRun() directly. Moved Cobra argument validation tests to E2E test sections where they belong. Also enhanced AGENTS.md with explicit examples showing the correct pattern for both unit tests (call preRun directly) and E2E tests (call Execute). Closes kortex-hub#45 Co-Authored-By: Claude Code (Claude Sonnet 4.5) <noreply@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
![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 the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 268-335: Update the examples and guidance to call t.Parallel() at
the parent test function level as the first line (unless using t.Setenv()),
e.g., add t.Parallel() at the top of TestMyCmd_PreRun and TestMyCmd_E2E before
any t.Run subtests; keep the inner subtests optionally parallel but ensure the
parent test functions begin with t.Parallel() and document the exception when
t.Setenv() is used; references: TestMyCmd_PreRun, TestMyCmd_E2E, c.preRun,
rootCmd.Execute.
In `@pkg/cmd/workspace_remove_test.go`:
- Around line 50-58: The test currently passes an absolute path from t.TempDir()
into cmd.Flags().String("storage", ...) so workspaceRemoveCmd.preRun() never
exercises filepath.Abs() normalization; change the test to pass a relative
storage path instead (e.g., create a temporary directory, derive a relative path
to it from the current working directory or chdir to the temp parent and use the
basename) when setting cmd.Flags().String("storage", ...), then call
workspaceRemoveCmd.preRun(cmd, args) and assert it still returns nil and that
c.storage (or the field set by preRun) contains the expected absolute path after
normalization by preRun.
🪄 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: 7d103c8b-1604-4278-9aeb-50e57401cdcc
📒 Files selected for processing (3)
AGENTS.mdpkg/cmd/workspace_list_test.gopkg/cmd/workspace_remove_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/cmd/workspace_list_test.go
Comment on lines
+268
to
+335
| 1. **Unit Tests** - Test the `preRun` method directly by calling it on the command struct: | ||
| - **IMPORTANT**: Create an instance of the command struct (e.g., `c := &initCmd{}`) | ||
| - **IMPORTANT**: Create a mock `*cobra.Command` and set up required flags | ||
| - **IMPORTANT**: Call `c.preRun(cmd, args)` directly - DO NOT call `rootCmd.Execute()` | ||
| - Use `t.Run()` for subtests within a parent test function | ||
| - Test with different argument/flag combinations | ||
| - Verify struct fields are set correctly | ||
| - Verify struct fields are set correctly after `preRun()` executes | ||
| - Use `t.TempDir()` for temporary directories (automatic cleanup) | ||
|
|
||
| 2. **E2E Tests** - Test the full command execution: | ||
| - Execute via `rootCmd.Execute()` | ||
| **Example:** | ||
| ```go | ||
| func TestMyCmd_PreRun(t *testing.T) { | ||
| t.Run("sets fields correctly", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| storageDir := t.TempDir() | ||
|
|
||
| c := &myCmd{} // Create command struct instance | ||
| cmd := &cobra.Command{} // Create mock cobra command | ||
| cmd.Flags().String("storage", storageDir, "test storage flag") | ||
|
|
||
| args := []string{"arg1"} | ||
|
|
||
| err := c.preRun(cmd, args) // Call preRun directly | ||
| if err != nil { | ||
| t.Fatalf("preRun() failed: %v", err) | ||
| } | ||
|
|
||
| // Assert on struct fields | ||
| if c.manager == nil { | ||
| t.Error("Expected manager to be created") | ||
| } | ||
| }) | ||
| } | ||
| ``` | ||
|
|
||
| 2. **E2E Tests** - Test the full command execution including Cobra wiring: | ||
| - Execute via `rootCmd.Execute()` to test the complete flow | ||
| - Use real temp directories with `t.TempDir()` | ||
| - Verify output messages | ||
| - Verify persistence (check storage/database) | ||
| - Verify all field values from `manager.List()` or similar | ||
| - Test multiple scenarios (default args, custom args, edge cases) | ||
| - Test Cobra's argument validation (e.g., required args, arg counts) | ||
|
|
||
| **Example:** | ||
| ```go | ||
| func TestMyCmd_E2E(t *testing.T) { | ||
| t.Run("executes successfully", func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| storageDir := t.TempDir() | ||
|
|
||
| rootCmd := NewRootCmd() // Use full command construction | ||
| rootCmd.SetArgs([]string{"mycommand", "arg1", "--storage", storageDir}) | ||
|
|
||
| err := rootCmd.Execute() // Execute the full command | ||
| if err != nil { | ||
| t.Fatalf("Execute() failed: %v", err) | ||
| } | ||
|
|
||
| // Verify results in storage | ||
| manager, _ := instances.NewManager(storageDir) | ||
| instancesList, _ := manager.List() | ||
| // ... assert on results | ||
| }) | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Make the test examples include parent-level t.Parallel().
Both new examples show t.Parallel() only inside the subtest, but the repo rule requires it as the first line of each test function unless t.Setenv() is involved. Since this file is the canonical guidance, the examples and bullet list should show that explicitly to avoid teaching the wrong pattern.
As per coding guidelines, "All Go test functions must call t.Parallel() as the first line of the test function, except for tests using t.Setenv() which cannot use t.Parallel() on the parent test function due to Go framework restrictions".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 268 - 335, Update the examples and guidance to call
t.Parallel() at the parent test function level as the first line (unless using
t.Setenv()), e.g., add t.Parallel() at the top of TestMyCmd_PreRun and
TestMyCmd_E2E before any t.Run subtests; keep the inner subtests optionally
parallel but ensure the parent test functions begin with t.Parallel() and
document the exception when t.Setenv() is used; references: TestMyCmd_PreRun,
TestMyCmd_E2E, c.preRun, rootCmd.Execute.
Comment on lines
+50
to
+58
| storageDir := t.TempDir() | ||
|
|
||
| c := &workspaceRemoveCmd{} | ||
| cmd := &cobra.Command{} | ||
| cmd.Flags().String("storage", storageDir, "test storage flag") | ||
|
|
||
| args := []string{"test-workspace-id"} | ||
|
|
||
| err := c.preRun(cmd, args) |
There was a problem hiding this comment.
Exercise storage-path normalization with a relative path.
This subtest passes an already-absolute t.TempDir() value into --storage, so it never actually validates the filepath.Abs() step in workspaceRemoveCmd.preRun(). A regression in path normalization would still pass here. Consider feeding a relative storage path in this unit test and asserting the preRun setup still succeeds.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/cmd/workspace_remove_test.go` around lines 50 - 58, The test currently
passes an absolute path from t.TempDir() into cmd.Flags().String("storage", ...)
so workspaceRemoveCmd.preRun() never exercises filepath.Abs() normalization;
change the test to pass a relative storage path instead (e.g., create a
temporary directory, derive a relative path to it from the current working
directory or chdir to the temp parent and use the basename) when setting
cmd.Flags().String("storage", ...), then call workspaceRemoveCmd.preRun(cmd,
args) and assert it still returns nil and that c.storage (or the field set by
preRun) contains the expected absolute path after normalization by preRun.
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
4 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.
The preRun tests in workspace_list_test.go and workspace_remove_test.go were incorrectly calling rootCmd.Execute() instead of testing the preRun method directly. This mixed Cobra wiring with unit test logic.
Updated tests to follow the pattern from init_test.go by creating command struct instances and calling c.preRun() directly. Moved Cobra argument validation tests to E2E test sections where they belong.
Also enhanced AGENTS.md with explicit examples showing the correct pattern for both unit tests (call preRun directly) and E2E tests (call Execute).
Closes #45