From 8d63dd28715458f690a250a6d9132e3c344daf35 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 15 Mar 2026 15:13:29 +0000 Subject: [PATCH 1/3] Initial plan From 858244765e422b8a603c49278c548293fb9a679e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 15 Mar 2026 15:21:29 +0000 Subject: [PATCH 2/3] improve test quality in pkg/parser/frontmatter_helpers_test.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/frontmatter_helpers_test.go | 107 +++++++++---------------- 1 file changed, 39 insertions(+), 68 deletions(-) diff --git a/pkg/parser/frontmatter_helpers_test.go b/pkg/parser/frontmatter_helpers_test.go index c32905708f2..98fa1a9acc7 100644 --- a/pkg/parser/frontmatter_helpers_test.go +++ b/pkg/parser/frontmatter_helpers_test.go @@ -6,10 +6,11 @@ import ( "errors" "os" "path/filepath" - "strings" "testing" "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestUpdateWorkflowFrontmatter(t *testing.T) { @@ -22,6 +23,7 @@ func TestUpdateWorkflowFrontmatter(t *testing.T) { updateFunc func(frontmatter map[string]any) error expectedContent string expectError bool + verbose bool }{ { name: "Add tool to existing tools section", @@ -121,50 +123,51 @@ tools: {} expectedContent: "", expectError: true, }, + { + name: "Verbose mode does not affect output", + initialContent: `--- +engine: claude +--- +# Test`, + updateFunc: func(frontmatter map[string]any) error { + frontmatter["engine"] = "copilot" + return nil + }, + expectError: false, + verbose: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { // Create test file testFile := filepath.Join(tempDir, "test-workflow.md") - if err := os.WriteFile(testFile, []byte(tt.initialContent), 0644); err != nil { - t.Fatalf("Failed to create test file: %v", err) - } + err := os.WriteFile(testFile, []byte(tt.initialContent), 0644) + require.NoError(t, err, "test file setup should succeed") // Run the update function - err := UpdateWorkflowFrontmatter(testFile, tt.updateFunc, false) + err = UpdateWorkflowFrontmatter(testFile, tt.updateFunc, tt.verbose) if tt.expectError { - if err == nil { - t.Errorf("Expected error but got none") - } + assert.Error(t, err, "UpdateWorkflowFrontmatter should return an error") return } - if err != nil { - t.Errorf("Unexpected error: %v", err) - return - } + require.NoError(t, err, "UpdateWorkflowFrontmatter should succeed") // Read the updated content updatedContent, err := os.ReadFile(testFile) - if err != nil { - t.Fatalf("Failed to read updated file: %v", err) - } + require.NoError(t, err, "reading updated file should succeed") - // For tools section tests, just verify the tools were added correctly - // Skip exact content comparison since YAML marshaling order may vary - if strings.Contains(tt.name, "tool") { - content := string(updatedContent) - if !strings.Contains(content, "new-tool:") { - t.Errorf("Expected 'new-tool:' in updated content, got: %s", content) - } - if !strings.Contains(content, "type: test") { - t.Errorf("Expected 'type: test' in updated content, got: %s", content) - } - if !strings.Contains(content, "---") { - t.Errorf("Expected frontmatter delimiters '---' in updated content, got: %s", content) - } + content := string(updatedContent) + if tt.verbose { + // Verbose mode: verify the update was applied correctly (engine changed to copilot) + assert.Contains(t, content, "engine: copilot", "verbose mode should still update frontmatter correctly") + assert.Contains(t, content, "---", "verbose mode should preserve frontmatter delimiters") + } else { + assert.Contains(t, content, "new-tool:", "updated file should contain the new tool key") + assert.Contains(t, content, "type: test", "updated file should contain the tool type") + assert.Contains(t, content, "---", "updated file should preserve frontmatter delimiters") } }) } @@ -205,41 +208,15 @@ func TestEnsureToolsSection(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tools := EnsureToolsSection(tt.frontmatter) - // Verify tools section is a map - if tools == nil { - t.Errorf("Expected tools to be a map, got nil") - return - } + require.NotNil(t, tools, "EnsureToolsSection should never return nil") - // Verify frontmatter was updated + // Verify frontmatter was updated to contain the tools map frontmatterTools, ok := tt.frontmatter["tools"].(map[string]any) - if !ok { - t.Errorf("Expected frontmatter['tools'] to be a map") - return - } - - // Verify returned tools is the same reference as in frontmatter - if len(tools) != len(frontmatterTools) { - t.Errorf("Expected returned tools to have same length as frontmatter['tools']") - } + require.True(t, ok, "frontmatter['tools'] should be a map[string]any") - // For existing tools, verify content - if len(tt.expectedTools) > 0 { - for key, expectedValue := range tt.expectedTools { - if actualValue, exists := tools[key]; !exists { - t.Errorf("Expected tool '%s' not found", key) - } else { - // Simple comparison for test values - if expectedMap, ok := expectedValue.(map[string]any); ok { - if actualMap, ok := actualValue.(map[string]any); ok { - if expectedMap["type"] != actualMap["type"] { - t.Errorf("Expected tool '%s' type '%v', got '%v'", key, expectedMap["type"], actualMap["type"]) - } - } - } - } - } - } + // Verify returned tools matches the frontmatter entry and expected content + assert.Equal(t, frontmatterTools, tools, "returned tools should be the same reference as frontmatter['tools']") + assert.Equal(t, tt.expectedTools, tools, "tools section should match expected state") }) } } @@ -280,14 +257,8 @@ func TestReconstructWorkflowFile(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { result, err := reconstructWorkflowFile(tt.frontmatterYAML, tt.markdownContent) - if err != nil { - t.Errorf("Unexpected error: %v", err) - return - } - - if result != tt.expectedResult { - t.Errorf("Expected:\n%s\n\nGot:\n%s", tt.expectedResult, result) - } + require.NoError(t, err, "reconstructWorkflowFile should succeed") + assert.Equal(t, tt.expectedResult, result, "reconstructed file content should match") }) } } From fa6f75ab1d53521eed26dd8370e262d84ad35d74 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 15 Mar 2026 19:43:29 +0000 Subject: [PATCH 3/3] address review: remove dead expectedContent, capture stderr in verbose test, prove reference identity by mutation Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/frontmatter_helpers_test.go | 72 +++++++++----------------- 1 file changed, 25 insertions(+), 47 deletions(-) diff --git a/pkg/parser/frontmatter_helpers_test.go b/pkg/parser/frontmatter_helpers_test.go index 98fa1a9acc7..5ad75521d08 100644 --- a/pkg/parser/frontmatter_helpers_test.go +++ b/pkg/parser/frontmatter_helpers_test.go @@ -18,12 +18,11 @@ func TestUpdateWorkflowFrontmatter(t *testing.T) { tempDir := testutil.TempDir(t, "test-*") tests := []struct { - name string - initialContent string - updateFunc func(frontmatter map[string]any) error - expectedContent string - expectError bool - verbose bool + name string + initialContent string + updateFunc func(frontmatter map[string]any) error + expectError bool + verbose bool }{ { name: "Add tool to existing tools section", @@ -38,17 +37,6 @@ Some content`, tools["new-tool"] = map[string]any{"type": "test"} return nil }, - expectedContent: `--- -existing: {} -new-tool: - type: test -tools: - existing: {} - new-tool: - type: test ---- -# Test Workflow -Some content`, expectError: false, }, { @@ -63,14 +51,6 @@ Some content`, tools["new-tool"] = map[string]any{"type": "test"} return nil }, - expectedContent: `--- -engine: claude -tools: - new-tool: - type: test ---- -# Test Workflow -Some content`, expectError: false, }, { @@ -84,13 +64,6 @@ Some content`, tools["new-tool"] = map[string]any{"type": "test"} return nil }, - expectedContent: `--- -tools: - new-tool: - type: test ---- -# Test Workflow -Some content`, expectError: false, }, { @@ -102,13 +75,6 @@ Some content without frontmatter`, tools["new-tool"] = map[string]any{"type": "test"} return nil }, - expectedContent: `--- -tools: - new-tool: - type: test ---- -# Test Workflow -Some content without frontmatter`, expectError: false, }, { @@ -120,11 +86,10 @@ tools: {} updateFunc: func(frontmatter map[string]any) error { return errors.New("test error") }, - expectedContent: "", - expectError: true, + expectError: true, }, { - name: "Verbose mode does not affect output", + name: "Verbose mode emits info message to stderr", initialContent: `--- engine: claude --- @@ -145,8 +110,15 @@ engine: claude err := os.WriteFile(testFile, []byte(tt.initialContent), 0644) require.NoError(t, err, "test file setup should succeed") - // Run the update function - err = UpdateWorkflowFrontmatter(testFile, tt.updateFunc, tt.verbose) + // Run the update function, capturing stderr to detect verbose output + var stderr string + if tt.verbose { + stderr = testutil.CaptureStderr(t, func() { + err = UpdateWorkflowFrontmatter(testFile, tt.updateFunc, true) + }) + } else { + err = UpdateWorkflowFrontmatter(testFile, tt.updateFunc, false) + } if tt.expectError { assert.Error(t, err, "UpdateWorkflowFrontmatter should return an error") @@ -161,7 +133,9 @@ engine: claude content := string(updatedContent) if tt.verbose { - // Verbose mode: verify the update was applied correctly (engine changed to copilot) + // Verbose mode: verify the info message was emitted to stderr + assert.Contains(t, stderr, "Updated workflow file", "verbose mode should emit an info message to stderr") + // Verify the update was still applied correctly assert.Contains(t, content, "engine: copilot", "verbose mode should still update frontmatter correctly") assert.Contains(t, content, "---", "verbose mode should preserve frontmatter delimiters") } else { @@ -214,8 +188,12 @@ func TestEnsureToolsSection(t *testing.T) { frontmatterTools, ok := tt.frontmatter["tools"].(map[string]any) require.True(t, ok, "frontmatter['tools'] should be a map[string]any") - // Verify returned tools matches the frontmatter entry and expected content - assert.Equal(t, frontmatterTools, tools, "returned tools should be the same reference as frontmatter['tools']") + // Verify reference identity: mutating the returned map must be visible via frontmatter + tools["__probe__"] = true + assert.Equal(t, true, frontmatterTools["__probe__"], "returned tools should be the same map stored in frontmatter['tools']") + delete(tools, "__probe__") + + // Verify returned tools matches the expected content assert.Equal(t, tt.expectedTools, tools, "tools section should match expected state") }) }