From e5ed2351bd2992cb4c3b0bfe28e4a373bba8b727 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 13:21:15 +0000 Subject: [PATCH 1/3] Initial plan From 4526a70aee867cace29982aec21df888e66800f7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 13:31:42 +0000 Subject: [PATCH 2/3] docs: add scratchpad/testing.md with testing conventions --- scratchpad/testing.md | 230 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 230 insertions(+) create mode 100644 scratchpad/testing.md diff --git a/scratchpad/testing.md b/scratchpad/testing.md new file mode 100644 index 0000000..eab529c --- /dev/null +++ b/scratchpad/testing.md @@ -0,0 +1,230 @@ +# Testing Patterns & Best Practices + +This document describes the testing conventions used in this repository. It serves as the reference for test quality analysis and improvements. + +## Testing Philosophy + +- **No mocks** — test real component interactions, not mocked interfaces +- **No test suites** — use plain `TestXxx` functions with `t.Run()` for subtests +- **Table-driven tests** — preferred for functions with multiple input/output combinations +- **Real filesystem** — use `t.TempDir()` for isolated filesystem tests +- **Descriptive names** — test names should explain what's being tested and the scenario + +## Assertion Libraries + +The repository uses [testify](https://github.com/stretchr/testify) for assertions. All new tests should use testify. + +### `require.*` — stop on failure + +Use `require` for critical setup steps. If the assertion fails, the test stops immediately. + +```go +import "github.com/stretchr/testify/require" + +config, err := LoadConfig() +require.NoError(t, err, "config loading should succeed") +require.NotNil(t, config, "config should not be nil") +``` + +### `assert.*` — continue after failure + +Use `assert` for validation checks where multiple failures can be reported together. + +```go +import "github.com/stretchr/testify/assert" + +result := ProcessData(input) +assert.Equal(t, expected, result, "should process data correctly") +assert.True(t, result.IsValid(), "result should be valid") +assert.Len(t, items, 3, "should have exactly 3 items") +``` + +### Common anti-patterns → testify replacements + +| Anti-pattern | Testify equivalent | +|---|---| +| `if err != nil { t.Fatal(err) }` | `require.NoError(t, err)` | +| `if err == nil { t.Fatal("expected error") }` | `require.Error(t, err)` | +| `if got != want { t.Errorf("got %v, want %v", got, want) }` | `assert.Equal(t, want, got)` | +| `if got == nil { t.Fatal("expected non-nil") }` | `require.NotNil(t, got)` | +| `if len(got) != 3 { t.Errorf("...") }` | `assert.Len(t, got, 3)` | +| `if !strings.Contains(got, sub) { t.Errorf("...") }` | `assert.Contains(t, got, sub)` | + +## Table-Driven Tests + +Preferred for any function tested with multiple inputs. + +```go +func TestContextEvaluate(t *testing.T) { + tests := []struct { + name string + expr string + want interface{} + wantErr bool + }{ + {name: "literal true", expr: "true", want: true}, + {name: "literal false", expr: "false", want: false}, + {name: "event property", expr: "event.cwd", want: "/test/path"}, + {name: "invalid expr", expr: "!!!bad", wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := Evaluate(tt.expr) + + if tt.wantErr { + require.Error(t, err) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} +``` + +See `internal/expression/evaluator_test.go` for real examples of this pattern in the codebase. + +## Unit Tests + +Unit tests live in the same package as the code they test (e.g. `internal/runner/runner_test.go`). + +### Before (plain Go) + +```go +func TestStepWithoutTimeout(t *testing.T) { + // ... + results, err := runner.Run(ctx) + if err != nil { + t.Fatalf("Expected no error, got %v", err) + } + if len(results) != 1 { + t.Fatalf("Expected 1 result, got %d", len(results)) + } + if !results[0].Success { + t.Errorf("Expected success, got error: %v", results[0].Error) + } +} +``` + +### After (testify) + +```go +func TestStepWithoutTimeout(t *testing.T) { + // ... + results, err := runner.Run(ctx) + require.NoError(t, err, "runner should execute without error") + require.Len(t, results, 1, "should produce exactly one result") + assert.True(t, results[0].Success, "step should succeed") +} +``` + +## End-to-End Tests + +E2E tests live in `tests/e2e/` and test the full `hookflow` binary. + +They rely on helpers defined in the package: + +- `setupWorkspaceWithHookflows(t, map[string]string)` — creates a temp workspace with hookflow YAML files +- `runHookflow(t, workspace, eventJSON, lifecycle, env)` — runs the binary and returns `(exitCode, output)` +- `assertAllow(t, result, output)` — asserts the hookflow allowed the action +- `assertDeny(t, result, output, msg)` — asserts the hookflow denied the action +- `buildEventJSON(action, data, cwd)` — constructs a JSON event payload + +Example: + +```go +func TestMyWorkflow(t *testing.T) { + workspace := setupWorkspaceWithHookflows(t, map[string]string{ + "my-workflow.yml": `name: My Workflow +on: + file: + paths: ['**/*.go'] +blocking: true +steps: + - name: check + run: exit 1 +`, + }) + + eventJSON := buildEventJSON("create", map[string]interface{}{ + "path": filepath.Join(workspace, "main.go"), + "file_text": "package main", + }, workspace) + + result, output := runHookflow(t, workspace, eventJSON, "preToolUse", nil) + assertDeny(t, result, output, "") +} +``` + +## Naming Conventions + +- `Test` — tests a single function +- `Test_` — tests a specific scenario for a function +- `t.Run("scenario name", ...)` — subtests within table-driven tests use readable names + +### Examples + +```go +func TestEvaluate(t *testing.T) { ... } +func TestEvaluate_WithEnvContext(t *testing.T) { ... } +func TestEvaluate_ReturnsErrorOnInvalidExpr(t *testing.T) { ... } +``` + +## Assertion Messages + +Always include a helpful message as the final argument so test failures are easier to diagnose: + +```go +// ❌ Without message — hard to debug +assert.Equal(t, expected, got) + +// ✅ With message — immediately tells you what failed +assert.Equal(t, expected, got, "response body should match expected output") +require.NoError(t, err, "parsing config file should not fail") +``` + +## Test Setup & Cleanup + +- Use `t.TempDir()` for filesystem tests — automatically cleaned up +- Use `t.Setenv(key, val)` to set environment variables — automatically restored +- Avoid global state; each test should be independent + +```go +func TestAppendEntry(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOOKFLOW_SESSION_DIR", dir) + // ... +} +``` + +## Running Tests + +```bash +# Run all tests +go test ./... + +# Run a specific package +go test ./internal/runner/... + +# Run a specific test +go test ./internal/expression/... -run TestContextEvaluate + +# Run with verbose output +go test -v ./... + +# Run with race detection +go test -race ./... + +# Run with coverage +go test -cover ./... +go test -coverprofile=coverage.out ./... +go tool cover -html=coverage.out +``` + +## Coverage Expectations + +- Unit tests should cover all exported functions and error paths +- Table-driven tests should cover success, error, and edge cases +- E2E tests focus on integration behaviour, not exhaustive coverage From c7f39c8ea14b67034a2c20c3631642aedcb45944 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 29 May 2026 13:32:51 +0000 Subject: [PATCH 3/3] docs: refine scratchpad/testing.md based on review feedback --- scratchpad/testing.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scratchpad/testing.md b/scratchpad/testing.md index eab529c..0bd6986 100644 --- a/scratchpad/testing.md +++ b/scratchpad/testing.md @@ -4,7 +4,7 @@ This document describes the testing conventions used in this repository. It serv ## Testing Philosophy -- **No mocks** — test real component interactions, not mocked interfaces +- **No mocks** — prefer real component interactions over mocked interfaces; mocks are only acceptable for unavoidable external dependencies (e.g., third-party APIs with no local equivalent) - **No test suites** — use plain `TestXxx` functions with `t.Run()` for subtests - **Table-driven tests** — preferred for functions with multiple input/output combinations - **Real filesystem** — use `t.TempDir()` for isolated filesystem tests @@ -84,7 +84,7 @@ func TestContextEvaluate(t *testing.T) { } ``` -See `internal/expression/evaluator_test.go` for real examples of this pattern in the codebase. +See the expression package tests (`internal/expression/`) for real examples of this pattern in the codebase. ## Unit Tests @@ -124,7 +124,7 @@ func TestStepWithoutTimeout(t *testing.T) { E2E tests live in `tests/e2e/` and test the full `hookflow` binary. -They rely on helpers defined in the package: +They rely on helpers defined in the package (refer to `tests/e2e/helpers_test.go` or similar for the current signatures): - `setupWorkspaceWithHookflows(t, map[string]string)` — creates a temp workspace with hookflow YAML files - `runHookflow(t, workspace, eventJSON, lifecycle, env)` — runs the binary and returns `(exitCode, output)`