From d7175ebd2342c1550dff0c4a2cfeafb179dc8dd8 Mon Sep 17 00:00:00 2001 From: Philippe Martin Date: Wed, 11 Mar 2026 15:59:24 +0100 Subject: [PATCH] test(cmd): fix preRun tests to call preRun directly 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 Co-Authored-By: Claude Code (Claude Sonnet 4.5) Signed-off-by: Philippe Martin --- AGENTS.md | 65 ++++++++++++++++++++-- pkg/cmd/workspace_list_test.go | 93 ++++++++++++++++++++------------ pkg/cmd/workspace_remove_test.go | 69 +++++++++++------------- 3 files changed, 150 insertions(+), 77 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0e93973..974e047 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -265,21 +265,76 @@ Commands should follow a consistent structure for maintainability and testabilit Commands should have two types of tests following the pattern in `pkg/cmd/init_test.go`: -1. **Unit Tests** - Test the `preRun` method directly: +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 + }) + } + ``` -**Reference:** See `pkg/cmd/init_test.go` for complete examples of both `preRun` unit tests (in `TestInitCmd_PreRun`) and E2E tests (in `TestInitCmd_E2E`). +**Reference:** See `pkg/cmd/init_test.go`, `pkg/cmd/workspace_list_test.go`, and `pkg/cmd/workspace_remove_test.go` for complete examples of both `preRun` unit tests (in `Test*Cmd_PreRun`) and E2E tests (in `Test*Cmd_E2E`). ### Working with the Instances Manager diff --git a/pkg/cmd/workspace_list_test.go b/pkg/cmd/workspace_list_test.go index c2f6f0f..c5f7f46 100644 --- a/pkg/cmd/workspace_list_test.go +++ b/pkg/cmd/workspace_list_test.go @@ -27,6 +27,7 @@ import ( api "github.com/kortex-hub/kortex-cli-api/cli/go" "github.com/kortex-hub/kortex-cli/pkg/instances" + "github.com/spf13/cobra" ) func TestWorkspaceListCmd(t *testing.T) { @@ -49,13 +50,21 @@ func TestWorkspaceListCmd_PreRun(t *testing.T) { t.Parallel() storageDir := t.TempDir() - rootCmd := NewRootCmd() - rootCmd.SetArgs([]string{"workspace", "list", "--storage", storageDir}) - // Execute to trigger preRun - err := rootCmd.Execute() + c := &workspaceListCmd{} + cmd := &cobra.Command{} + cmd.Flags().String("storage", storageDir, "test storage flag") + cmd.Flags().String("output", "", "test output flag") + + args := []string{} + + err := c.preRun(cmd, args) if err != nil { - t.Fatalf("Expected no error, got %v", err) + t.Fatalf("preRun() failed: %v", err) + } + + if c.manager == nil { + t.Error("Expected manager to be created") } }) @@ -63,12 +72,21 @@ func TestWorkspaceListCmd_PreRun(t *testing.T) { t.Parallel() storageDir := t.TempDir() - rootCmd := NewRootCmd() - rootCmd.SetArgs([]string{"workspace", "list", "--storage", storageDir}) - err := rootCmd.Execute() + c := &workspaceListCmd{} + cmd := &cobra.Command{} + cmd.Flags().String("storage", storageDir, "test storage flag") + cmd.Flags().String("output", "", "test output flag") + + args := []string{} + + err := c.preRun(cmd, args) if err != nil { - t.Fatalf("Expected no error with no output flag, got %v", err) + t.Fatalf("preRun() failed: %v", err) + } + + if c.output != "" { + t.Errorf("Expected output to be empty, got %s", c.output) } }) @@ -76,25 +94,22 @@ func TestWorkspaceListCmd_PreRun(t *testing.T) { t.Parallel() storageDir := t.TempDir() - rootCmd := NewRootCmd() - rootCmd.SetArgs([]string{"workspace", "list", "--storage", storageDir, "--output", "json"}) - - err := rootCmd.Execute() - if err != nil { - t.Fatalf("Expected no error with --output json, got %v", err) - } - }) - t.Run("accepts valid output flag with -o json", func(t *testing.T) { - t.Parallel() + c := &workspaceListCmd{} + cmd := &cobra.Command{} + cmd.Flags().String("storage", storageDir, "test storage flag") + cmd.Flags().String("output", "", "test output flag") + cmd.Flags().Set("output", "json") - storageDir := t.TempDir() - rootCmd := NewRootCmd() - rootCmd.SetArgs([]string{"workspace", "list", "--storage", storageDir, "-o", "json"}) + args := []string{} - err := rootCmd.Execute() + err := c.preRun(cmd, args) if err != nil { - t.Fatalf("Expected no error with -o json, got %v", err) + t.Fatalf("preRun() failed: %v", err) + } + + if c.output != "json" { + t.Errorf("Expected output to be 'json', got %s", c.output) } }) @@ -102,12 +117,18 @@ func TestWorkspaceListCmd_PreRun(t *testing.T) { t.Parallel() storageDir := t.TempDir() - rootCmd := NewRootCmd() - rootCmd.SetArgs([]string{"workspace", "list", "--storage", storageDir, "--output", "xml"}) - err := rootCmd.Execute() + c := &workspaceListCmd{} + cmd := &cobra.Command{} + cmd.Flags().String("storage", storageDir, "test storage flag") + cmd.Flags().String("output", "", "test output flag") + cmd.Flags().Set("output", "xml") + + args := []string{} + + err := c.preRun(cmd, args) if err == nil { - t.Fatal("Expected error with invalid output format, got nil") + t.Fatal("Expected preRun() to fail with invalid output format") } if !strings.Contains(err.Error(), "unsupported output format") { @@ -115,16 +136,22 @@ func TestWorkspaceListCmd_PreRun(t *testing.T) { } }) - t.Run("rejects invalid output format with short flag", func(t *testing.T) { + t.Run("rejects invalid output format yaml", func(t *testing.T) { t.Parallel() storageDir := t.TempDir() - rootCmd := NewRootCmd() - rootCmd.SetArgs([]string{"workspace", "list", "--storage", storageDir, "-o", "yaml"}) - err := rootCmd.Execute() + c := &workspaceListCmd{} + cmd := &cobra.Command{} + cmd.Flags().String("storage", storageDir, "test storage flag") + cmd.Flags().String("output", "", "test output flag") + cmd.Flags().Set("output", "yaml") + + args := []string{} + + err := c.preRun(cmd, args) if err == nil { - t.Fatal("Expected error with invalid output format, got nil") + t.Fatal("Expected preRun() to fail with invalid output format") } if !strings.Contains(err.Error(), "unsupported output format") { diff --git a/pkg/cmd/workspace_remove_test.go b/pkg/cmd/workspace_remove_test.go index 284a491..de4edc3 100644 --- a/pkg/cmd/workspace_remove_test.go +++ b/pkg/cmd/workspace_remove_test.go @@ -25,6 +25,7 @@ import ( "testing" "github.com/kortex-hub/kortex-cli/pkg/instances" + "github.com/spf13/cobra" ) func TestWorkspaceRemoveCmd(t *testing.T) { @@ -43,6 +44,35 @@ func TestWorkspaceRemoveCmd(t *testing.T) { func TestWorkspaceRemoveCmd_PreRun(t *testing.T) { t.Parallel() + t.Run("extracts id from args and creates manager", func(t *testing.T) { + t.Parallel() + + 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) + if err != nil { + t.Fatalf("preRun() failed: %v", err) + } + + if c.manager == nil { + t.Error("Expected manager to be created") + } + + if c.id != "test-workspace-id" { + t.Errorf("Expected id to be 'test-workspace-id', got %s", c.id) + } + }) +} + +func TestWorkspaceRemoveCmd_E2E(t *testing.T) { + t.Parallel() + t.Run("requires ID argument", func(t *testing.T) { t.Parallel() @@ -77,45 +107,6 @@ func TestWorkspaceRemoveCmd_PreRun(t *testing.T) { } }) - t.Run("creates manager from storage flag", func(t *testing.T) { - t.Parallel() - - storageDir := t.TempDir() - sourcesDir := t.TempDir() - - // Create a workspace first - manager, err := instances.NewManager(storageDir) - if err != nil { - t.Fatalf("Failed to create manager: %v", err) - } - - instance, err := instances.NewInstance(instances.NewInstanceParams{ - SourceDir: sourcesDir, - ConfigDir: filepath.Join(sourcesDir, ".kortex"), - }) - if err != nil { - t.Fatalf("Failed to create instance: %v", err) - } - - addedInstance, err := manager.Add(instance) - if err != nil { - t.Fatalf("Failed to add instance: %v", err) - } - - // Now remove it - rootCmd := NewRootCmd() - rootCmd.SetArgs([]string{"workspace", "remove", addedInstance.GetID(), "--storage", storageDir}) - - err = rootCmd.Execute() - if err != nil { - t.Fatalf("Expected no error, got %v", err) - } - }) -} - -func TestWorkspaceRemoveCmd_E2E(t *testing.T) { - t.Parallel() - t.Run("removes existing workspace by ID", func(t *testing.T) { t.Parallel()