Skip to content

Commit bac5ffa

Browse files
Copilotpelikhan
Copilot
and
pelikhan
authored
refactor: improve test assertions for safe-output-tools prompt tests
- Use assert.Equal (order-sensitive) instead of assert.ElementsMatch so sorting/determinism test cases actually validate emission order - Rewrite consistency test to parse the exact Tools: line into a set of identifiers and assert membership, using stringutil.NormalizeSafeOutputIdentifier instead of strings.ReplaceAll to avoid false positives from substring matches - Extract helper extractToolNamesFromSections to avoid duplication Agent-Logs-Url: https://github.com/github/gh-aw/sessions/88d2ab26-99ab-40e0-8b1f-04dd925ae6a1 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent 5b0aed3 commit bac5ffa

File tree

3 files changed

+60
-51
lines changed

3 files changed

+60
-51
lines changed

‎.github/workflows/daily-choice-test.lock.yml‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎.github/workflows/notion-issue-summary.lock.yml‎

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎pkg/workflow/safe_outputs_prompt_tools_test.go‎

Lines changed: 58 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"strings"
77
"testing"
88

9+
"github.com/github/gh-aw/pkg/stringutil"
910
"github.com/stretchr/testify/assert"
1011
"github.com/stretchr/testify/require"
1112
)
@@ -136,36 +137,10 @@ func TestBuildSafeOutputsSectionsCustomTools(t *testing.T) {
136137

137138
require.NotNil(t, sections, "Expected non-nil sections")
138139

139-
// Find the opening section with the Tools: list
140-
var toolsLine string
141-
for _, section := range sections {
142-
if !section.IsFile && strings.HasPrefix(section.Content, "<safe-output-tools>") {
143-
toolsLine = section.Content
144-
break
145-
}
146-
}
147-
148-
require.NotEmpty(t, toolsLine, "Expected to find <safe-output-tools> opening section")
149-
150-
// Extract tool names from "Tools: tool1, tool2, ..." line
151-
lines := strings.Split(toolsLine, "\n")
152-
require.GreaterOrEqual(t, len(lines), 2, "Expected at least two lines in tools section")
140+
actualToolNames := extractToolNamesFromSections(t, sections)
153141

154-
toolsListLine := lines[1]
155-
assert.True(t, strings.HasPrefix(toolsListLine, "Tools: "), "Second line should start with 'Tools: '")
156-
157-
toolsList := strings.TrimPrefix(toolsListLine, "Tools: ")
158-
actualToolNames := strings.Split(toolsList, ", ")
159-
160-
// Strip any max budget annotations like "noop(max:5)" → "noop"
161-
for i, t := range actualToolNames {
162-
if name, _, found := strings.Cut(t, "("); found {
163-
actualToolNames[i] = name
164-
}
165-
}
166-
167-
assert.ElementsMatch(t, tt.expectedTools, actualToolNames,
168-
"Tool names in <safe-output-tools> should match expected set")
142+
assert.Equal(t, tt.expectedTools, actualToolNames,
143+
"Tool names in <safe-output-tools> should match expected order and set")
169144
})
170145
}
171146
}
@@ -191,34 +166,68 @@ func TestBuildSafeOutputsSectionsCustomToolsConsistency(t *testing.T) {
191166
sections := buildSafeOutputsSections(config)
192167
require.NotNil(t, sections, "Expected non-nil sections")
193168

194-
// Concatenate all non-file section content to find the tools block
195-
var toolsBuilder strings.Builder
196-
for _, section := range sections {
197-
if !section.IsFile {
198-
toolsBuilder.WriteString(section.Content)
199-
toolsBuilder.WriteString("\n")
200-
}
169+
actualToolNames := extractToolNamesFromSections(t, sections)
170+
actualToolSet := make(map[string]bool, len(actualToolNames))
171+
for _, name := range actualToolNames {
172+
actualToolSet[name] = true
201173
}
202-
toolsContent := toolsBuilder.String()
203174

204-
// Every custom job name (normalized) must appear in the tools list
175+
// Every custom job name (normalized) must appear as an exact tool identifier.
205176
for jobName := range config.Jobs {
206-
normalizedName := strings.ReplaceAll(jobName, "-", "_")
207-
assert.Contains(t, toolsContent, normalizedName,
208-
"Custom job %q (normalized: %q) should appear in <safe-output-tools>", jobName, normalizedName)
177+
normalized := stringutil.NormalizeSafeOutputIdentifier(jobName)
178+
assert.True(t, actualToolSet[normalized],
179+
"Custom job %q (normalized: %q) should appear as an exact tool identifier in <safe-output-tools>", jobName, normalized)
209180
}
210181

211-
// Every custom script name (normalized) must appear in the tools list
182+
// Every custom script name (normalized) must appear as an exact tool identifier.
212183
for scriptName := range config.Scripts {
213-
normalizedName := strings.ReplaceAll(scriptName, "-", "_")
214-
assert.Contains(t, toolsContent, normalizedName,
215-
"Custom script %q (normalized: %q) should appear in <safe-output-tools>", scriptName, normalizedName)
184+
normalized := stringutil.NormalizeSafeOutputIdentifier(scriptName)
185+
assert.True(t, actualToolSet[normalized],
186+
"Custom script %q (normalized: %q) should appear as an exact tool identifier in <safe-output-tools>", scriptName, normalized)
216187
}
217188

218-
// Every custom action name (normalized) must appear in the tools list
189+
// Every custom action name (normalized) must appear as an exact tool identifier.
219190
for actionName := range config.Actions {
220-
normalizedName := strings.ReplaceAll(actionName, "-", "_")
221-
assert.Contains(t, toolsContent, normalizedName,
222-
"Custom action %q (normalized: %q) should appear in <safe-output-tools>", actionName, normalizedName)
191+
normalized := stringutil.NormalizeSafeOutputIdentifier(actionName)
192+
assert.True(t, actualToolSet[normalized],
193+
"Custom action %q (normalized: %q) should appear as an exact tool identifier in <safe-output-tools>", actionName, normalized)
194+
}
195+
}
196+
197+
// extractToolNamesFromSections parses the <safe-output-tools> opening section and returns
198+
// the list of tool names in the order they appear, stripping any max-budget annotations
199+
// (e.g. "noop(max:5)" → "noop").
200+
func extractToolNamesFromSections(t *testing.T, sections []PromptSection) []string {
201+
t.Helper()
202+
203+
var toolsLine string
204+
for _, section := range sections {
205+
if !section.IsFile && strings.HasPrefix(section.Content, "<safe-output-tools>") {
206+
toolsLine = section.Content
207+
break
208+
}
209+
}
210+
211+
require.NotEmpty(t, toolsLine, "Expected to find <safe-output-tools> opening section")
212+
213+
lines := strings.Split(toolsLine, "\n")
214+
require.GreaterOrEqual(t, len(lines), 2, "Expected at least two lines in tools section")
215+
216+
toolsListLine := lines[1]
217+
require.True(t, strings.HasPrefix(toolsListLine, "Tools: "),
218+
"Second line should start with 'Tools: ', got: %q", toolsListLine)
219+
220+
toolsList := strings.TrimPrefix(toolsListLine, "Tools: ")
221+
toolEntries := strings.Split(toolsList, ", ")
222+
223+
names := make([]string, 0, len(toolEntries))
224+
for _, entry := range toolEntries {
225+
// Strip optional budget annotation: "noop(max:5)" → "noop"
226+
if name, _, found := strings.Cut(entry, "("); found {
227+
names = append(names, name)
228+
} else {
229+
names = append(names, entry)
230+
}
223231
}
232+
return names
224233
}

0 commit comments

Comments
 (0)