🎯 Areas for Improvement
1. No Testify Assertions — Add require and assert
// ❌ CURRENT (throughout the file)
if len(result) != 2 {
t.Errorf("Expected 2 args, got %d", len(result))
}
if err != nil {
t.Fatalf("Error parsing with args field: %v", err)
}
if len(configs) == 0 {
t.Fatal("No configs returned")
}
// ✅ IMPROVED
require.NoError(t, err, "parsing args field should succeed")
require.NotEmpty(t, configs, "parser should return at least one MCP config")
assert.Len(t, result, 2, "should return exactly 2 custom args")
assert.Equal(t, []string{"--verbose", "--debug"}, result)
assert.Nil(t, result, "no args field should return nil")
Also add the imports:
import (
"testing"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
2. Replace Manual for-Loop Contains Checks
// ❌ CURRENT — 12 lines per containment check
foundVerbose := false
for _, arg := range configs[0].Args {
if arg == "--verbose" { foundVerbose = true }
}
if !foundVerbose {
t.Errorf("Expected --verbose in args, got: %v", configs[0].Args)
}
// ✅ IMPROVED — 1 line
assert.Contains(t, configs[0].Args, "--verbose", "custom flag should be appended")
assert.Contains(t, strings.Join(configs[0].Args, " "), "ghcr.io/github/github-mcp-server:",
"Docker image should be present")
3. Refactor Duplicate Patterns to Table-Driven Tests
The GitHub and Playwright unit subtests follow the same three-case pattern ([]any / []string / nil). Extract each into a dedicated table-driven test:
func TestGetGitHubCustomArgs(t *testing.T) {
tests := []struct {
name string
input map[string]any
expected []string
}{
{"args as []any", map[string]any{"args": []any{"--verbose", "--debug"}}, []string{"--verbose", "--debug"}},
{"args as []string", map[string]any{"args": []string{"--flag"}}, []string{"--flag"}},
{"no args field returns nil", map[string]any{}, nil},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.expected, getGitHubCustomArgs(tt.input))
})
}
}
4. Add Missing Tests for extractCustomArgs and writeArgsToYAML
Both functions in args.go lack direct unit tests.
func TestExtractCustomArgs(t *testing.T) {
tests := []struct {
name string
input map[string]any
expected []string
}{
{"no key returns nil", map[string]any{}, nil},
{"[]any values", map[string]any{"args": []any{"a", "b"}}, []string{"a", "b"}},
{"[]string values", map[string]any{"args": []string{"a"}}, []string{"a"}},
{"[]any skips non-strings", map[string]any{"args": []any{"a", 42, "b"}}, []string{"a", "b"}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert.Equal(t, tt.expected, extractCustomArgs(tt.input))
})
}
}
func TestWriteArgsToYAML(t *testing.T) {
tests := []struct {
name string
args []string
indent string
expected string
}{
{"nil args produces no output", nil, " ", ""},
{"single arg", []string{"--verbose"}, " ", ",\n \"--verbose\""},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
var sb strings.Builder
writeArgsToYAML(&sb, tt.args, tt.indent)
assert.Equal(t, tt.expected, sb.String())
})
}
}
5. Add Assertion Messages
Add the optional message parameter to every assert / require call so failures explain context immediately.
Overview
The test file
pkg/workflow/args_field_test.gohas been selected by the Testify Uber Super Expert. It testsgetGitHubCustomArgs,getPlaywrightCustomArgs, andparser.ExtractMCPConfigurationsbut does not import testify — relying entirely on raw Goif/t.Errorfpatterns. This issue provides specific, actionable improvements.Current State
pkg/workflow/args_field_test.gopkg/workflow/args.goTestArgsField) with 5t.Run()subtestsStrengths ✅
t.Run()for all subtests with descriptive names[]anyand[]stringarg variants🎯 Areas for Improvement
1. No Testify Assertions — Add
requireandassertAlso add the imports:
2. Replace Manual
for-Loop Contains Checks3. Refactor Duplicate Patterns to Table-Driven Tests
The GitHub and Playwright unit subtests follow the same three-case pattern (
[]any/[]string/ nil). Extract each into a dedicated table-driven test:4. Add Missing Tests for
extractCustomArgsandwriteArgsToYAMLBoth functions in
args.golack direct unit tests.5. Add Assertion Messages
Add the optional message parameter to every
assert/requirecall so failures explain context immediately.📋 Implementation Guidelines
Priority Order
t.Fatal/t.Errorfwithrequire/assertfor-loop contains checks withassert.ContainsextractCustomArgsandwriteArgsToYAMLBest Practices from
scratchpad/testing.mdrequire.*for setup steps (stops test on failure)assert.*for validations (continues checking)t.Run()and descriptive namesCommands
go test -v ./pkg/workflow/... -run TestArgsField make test-unit make lintAcceptance Criteria
assertandrequireimported from testifyt.Fatal/t.Errorfraw checks replaced with testify assertionsfor-loop contains checks replaced withassert.ContainsextractCustomArgsandwriteArgsToYAMLmake test-unitpassesmake lintpassesPriority: Medium | Effort: Small | Expected Impact: Consistent style, better failure messages, increased coverage of
args.goFiles Involved:
pkg/workflow/args_field_test.go,pkg/workflow/args.goReferences: