Skip to content

[test-parallel] Add t.Parallel() to safe Go test subtests (batch 1) #53083

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
pelikhan merged 1 commit into from
Aug 16, 2026
Merged

[test-parallel] Add t.Parallel() to safe Go test subtests (batch 1) #53083

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
2 changes: 2 additions & 0 deletions cmd/gh-aw/help_sections_order_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
)

func TestUsageAppearsBeforeExamplesInHelpOutput(t *testing.T) {
t.Parallel()
Comment on lines 13 to +14
commands := []*cobra.Command{
compileCmd,
disableCmd,
Expand All @@ -23,6 +24,7 @@ func TestUsageAppearsBeforeExamplesInHelpOutput(t *testing.T) {

for _, cmd := range commands {
t.Run(cmd.CommandPath(), func(t *testing.T) {
t.Parallel()

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This subtest now runs in parallel while mutating the shared package-level Cobra command via SetOut/SetErr, so sibling subtests can race on the same command object and make help-output assertions flaky.

💡 Why this is blocking and how to fix it

compileCmd, disableCmd, and the other entries in this table are global command instances, not per-subtest copies. After adding t.Parallel(), two subtests can interleave cmd.SetOut, cmd.SetErr, cmd.Help(), and the cleanup that restores the old writers. That means one subtest can capture another subtest's output or restore the wrong writer, turning this test into timing-dependent noise.

A safe fix is to avoid parallelizing these subtests unless each one works on an isolated command instance. For example, build a fresh command per case instead of reusing globals:

cmd := newRootCmdOrCloneForTest(...)
cmd.SetOut(&out)
cmd.SetErr(&out)

If cloning the command tree is awkward, remove the nested t.Parallel() here and keep the cases serialized.

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[/tdd] Data race: these subtests are parallelized but mutate shared package-level cobra.Command globals (compileCmd, disableCmd, etc.) via cmd.SetOut() / cmd.SetErr(). When subtests run concurrently, multiple goroutines will race to set the output writers on the same shared command object.

💡 Fix suggestion

Remove t.Parallel() from the subtests (line 27), since the parent test already owns the global commands and the subtest loop captures the loop variable cmd — a shared pointer. Parallelizing here is unsafe without creating a fresh cobra.Command per subtest.

The safest fix is to remove the t.Parallel() added to the subtests while keeping the parent-level t.Parallel() intact.

@copilot please address this.

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Data race: parallel subtests mutate shared package-level cobra.Command objects

The subtests call cmd.SetOut(&out) and cmd.SetErr(&out) on package-level variables (compileCmd, disableCmd, etc.) that are shared across all test goroutines. Running these subtests with t.Parallel() will cause concurrent writes to the same *cobra.Command, triggering a data race detectable with go test -race.

To fix, either:

  1. Remove t.Parallel() from these subtests only (keeping the outer t.Parallel() is fine), or
  2. Clone each command before mutating it so each subtest has its own instance.

@copilot please address this.

var out bytes.Buffer
originalOut := cmd.OutOrStdout()
originalErr := cmd.ErrOrStderr()
Expand Down
1 change: 1 addition & 0 deletions cmd/gh-aw/short_description_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ func TestShortDescriptionConsistency(t *testing.T) {
t.Parallel()
for _, cmd := range collectCommandTree(rootCmd) {
t.Run("command "+cmd.Name()+" has no trailing punctuation", func(t *testing.T) {
t.Parallel()

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Parallelizing these subtests is unsafe because every case reads from the shared global rootCmd tree, and Cobra lazily initializes command metadata in ways that are not guaranteed to be concurrency-safe.

💡 Why this is blocking and how to fix it

collectCommandTree(rootCmd) returns pointers into the one global command graph. Once the subtests run concurrently, each case is traversing and inspecting the same mutable Cobra objects at the same time. This PR description explicitly avoided parallelizing another Cobra test for exactly that reason, so doing it here reintroduces the same class of flake through a different path.

The low-risk fix is to keep these subtests serial, or construct an isolated command tree per test before enabling t.Parallel(). Relying on read-only access is not enough when the library caches/normalizes fields lazily.

short := cmd.Short
if short == "" {
t.Skip("Command has no Short description")
Expand Down
1 change: 1 addition & 0 deletions pkg/actionpins/actionpins_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ func TestFindCompatiblePin_SemverFallback(t *testing.T) {
}

func TestFindVersionBySHA_ReturnsVersionForKnownSHA(t *testing.T) {
t.Parallel()
t.Run("returns version for a known SHA in embedded data", func(t *testing.T) {
pins := GetActionPinsByRepo("actions/checkout")
require.NotEmpty(t, pins, "prerequisite: embedded pins must exist for actions/checkout")
Expand Down
6 changes: 6 additions & 0 deletions pkg/actionpins/spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func TestSpec_PublicAPI_FormatPinnedActionReference(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
if tt.wantPanic != "" {
assert.Empty(t, tt.expected, "test case %q: wantPanic and expected are mutually exclusive", tt.name)
require.PanicsWithValue(t, tt.wantPanic, func() {
Expand Down Expand Up @@ -140,6 +141,7 @@ func TestSpec_PublicAPI_FormatCacheKey(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := actionpins.FormatCacheKey(tt.repo, tt.version)
assert.Equal(t, tt.expected, result, "FormatCacheKey(%q, %q) should match spec format", tt.repo, tt.version)
})
Expand Down Expand Up @@ -188,6 +190,7 @@ func TestSpec_PublicAPI_ExtractRepo(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := actionpins.ExtractRepo(tt.uses)
assert.Equal(t, tt.expected, result, "ExtractRepo(%q) should return repo part", tt.uses)
})
Expand Down Expand Up @@ -236,6 +239,7 @@ func TestSpec_PublicAPI_ExtractVersion(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := actionpins.ExtractVersion(tt.uses)
assert.Equal(t, tt.expected, result, "ExtractVersion(%q) should return version part", tt.uses)
})
Expand Down Expand Up @@ -359,6 +363,7 @@ func TestSpec_PublicAPI_ResolveActionPin_EnforcePinned(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
var failures []actionpins.ResolutionFailure
ctx := &actionpins.PinContext{
Resolver: tt.resolver,
Expand Down Expand Up @@ -703,6 +708,7 @@ func TestSpec_PublicAPI_RecordResolutionFailure(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
var failures []actionpins.ResolutionFailure
ctx := &actionpins.PinContext{
Resolver: tt.resolver,
Expand Down
1 change: 1 addition & 0 deletions pkg/agentdrain/anomaly_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,7 @@ func TestAnalyzeEvent(t *testing.T) {

// TestAnalyzeEvent_Variants covers edge-case event shapes: empty stage and nil/empty fields.
func TestAnalyzeEvent_Variants(t *testing.T) {
t.Parallel()
tests := []struct {
name string
evt AgentEvent
Expand Down
1 change: 1 addition & 0 deletions pkg/agentdrain/miner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
)

func TestNewMiner(t *testing.T) {
t.Parallel()
cfg := DefaultConfig()
m, err := NewMiner(cfg)
require.NoError(t, err, "NewMiner should not return an error")
Expand Down
4 changes: 4 additions & 0 deletions pkg/agentdrain/spec_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ func TestSpec_PublicAPI_Utility_Tokenize(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := agentdrain.Tokenize(tt.line)
assert.Equal(t, tt.expected, result, "Tokenize(%q) mismatch", tt.line)
})
Expand All @@ -197,6 +198,7 @@ func TestSpec_PublicAPI_Utility_Tokenize(t *testing.T) {
func TestSpec_PublicAPI_Utility_FlattenEvent(t *testing.T) {
t.Parallel()
t.Run("excludes listed fields", func(t *testing.T) {
t.Parallel()
evt := agentdrain.AgentEvent{
Stage: "plan",
Fields: map[string]string{
Expand All @@ -210,6 +212,7 @@ func TestSpec_PublicAPI_Utility_FlattenEvent(t *testing.T) {
})

t.Run("produces deterministic output for same input", func(t *testing.T) {
t.Parallel()
evt := agentdrain.AgentEvent{
Stage: "tool_call",
Fields: map[string]string{
Expand Down Expand Up @@ -257,6 +260,7 @@ func TestSpec_PublicAPI_Utility_StageSequence(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result := agentdrain.StageSequence(tt.events)
assert.Equal(t, tt.expected, result, "StageSequence mismatch for %q", tt.name)
})
Expand Down
3 changes: 3 additions & 0 deletions pkg/cli/access_log_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ func TestExtractDomainFromURL(t *testing.T) {

for _, tt := range tests {
t.Run(tt.url, func(t *testing.T) {
t.Parallel()
result := stringutil.ExtractDomainFromURL(tt.url)
assert.Equal(t, tt.expected, result, "should extract correct domain from URL")
})
Expand Down Expand Up @@ -225,6 +226,7 @@ func TestParseSquidLogLine(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
result, err := parseSquidLogLine(tt.line)

if tt.shouldErr {
Expand Down Expand Up @@ -296,6 +298,7 @@ func TestAddMetrics(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
tt.base.AddMetrics(tt.toAdd)
assert.Equal(t, tt.expected.TotalRequests, tt.base.TotalRequests, "total requests should match")
assert.Equal(t, tt.expected.AllowedRequests, tt.base.AllowedRequests, "allowed requests should match")
Expand Down
6 changes: 6 additions & 0 deletions pkg/cli/actions_build_command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ func TestActionsCleanCommand_NoActionsDir(t *testing.T) {
}

func TestGetActionDirectories(t *testing.T) {
t.Parallel()
tests := []struct {
name string
setup func(string) error
Expand Down Expand Up @@ -129,6 +130,7 @@ func TestGetActionDirectories(t *testing.T) {
}

func TestGetActionDirectories_SortedOutput(t *testing.T) {
t.Parallel()
tmpDir := t.TempDir()
actionsDir := filepath.Join(tmpDir, "actions")
// Create directories in reverse-alphabetical order to verify sorting
Expand All @@ -142,6 +144,7 @@ func TestGetActionDirectories_SortedOutput(t *testing.T) {
}

func TestValidateActionYml(t *testing.T) {
t.Parallel()
tests := []struct {
name string
actionYmlContent string
Expand Down Expand Up @@ -243,6 +246,7 @@ runs:
}

func TestGetActionDependencies(t *testing.T) {
t.Parallel()
tests := []struct {
name string
actionName string
Expand All @@ -262,6 +266,7 @@ func TestGetActionDependencies(t *testing.T) {

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
deps := getActionDependencies(tt.actionName)
assert.GreaterOrEqual(t, len(deps), tt.minDeps, "Should return at least minimum dependencies")
})
Expand Down Expand Up @@ -326,6 +331,7 @@ func TestActionsCleanCommand_EmptyActionsDir(t *testing.T) {
}

func TestIsCompositeAction(t *testing.T) {
t.Parallel()
tests := []struct {
name string
actionYmlContent string
Expand Down
2 changes: 2 additions & 0 deletions pkg/cli/add_command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ func validateEngineStub(engine string) error {
}

func TestNewAddCommand(t *testing.T) {
t.Parallel()
cmd := NewAddCommand(validateEngineStub)

require.NotNil(t, cmd, "NewAddCommand should not return nil")
Expand Down Expand Up @@ -84,6 +85,7 @@ func TestNewAddCommand(t *testing.T) {
}

func TestNewAddCommand_MentionsEnterpriseSourceResolution(t *testing.T) {
t.Parallel()
cmd := NewAddCommand(validateEngineStub)
require.NotNil(t, cmd)

Expand Down
Loading