Skip to content

test(cmd): fix preRun tests to call preRun directly #60

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
feloy merged 1 commit into from
Mar 12, 2026
Merged

test(cmd): fix preRun tests to call preRun directly #60

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 60 additions & 5 deletions AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
})
}
```
Comment on lines +268 to +335
Copy link

@coderabbitai coderabbitai bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.


**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

Expand Down
93 changes: 60 additions & 33 deletions pkg/cmd/workspace_list_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -49,82 +50,108 @@ 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")
}
})

t.Run("accepts no output flag", func(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)
}
})

t.Run("accepts valid output flag with json", func(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)
}
})

t.Run("rejects invalid output format", func(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") {
t.Errorf("Expected error to contain 'unsupported output format', got: %v", err)
}
})

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") {
Expand Down
69 changes: 30 additions & 39 deletions pkg/cmd/workspace_remove_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
"testing"

"github.com/kortex-hub/kortex-cli/pkg/instances"
"github.com/spf13/cobra"
)

func TestWorkspaceRemoveCmd(t *testing.T) {
Expand All @@ -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)
Comment on lines +50 to +58
Copy link

@coderabbitai coderabbitai bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

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()

Expand Down Expand Up @@ -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()

Expand Down
Loading