Testify assertion hygiene sweep in Go tests (require/assert helper alignment)

[Copilot]

✨ Enhancement

The issue identified localized testify anti-patterns in tests (boolean/len equality assertions, strings.Contains wrapped in assert.True, and assert.NoError used where failure should short-circuit). This PR applies a mechanical cleanup to those spots while keeping behavior and dependency versions unchanged (testify stays at v1.11.1).

• What does this improve?

  • Replaces fail-open precondition checks with fail-fast checks in listed deref-sensitive cases:
    • assert.NoError(...) → require.NoError(...)
  • Replaces assertion forms with idiomatic testify helpers for clearer diagnostics:
    • assert.Equal(t, true|false, x) → assert.True/False(t, x)
    • assert.Equal(t, len(x), y) → assert.Len(t, x, y)
    • assert.NotEqual(t, 0, exitCode) → assert.NotZero(t, exitCode)
    • assert.True(t, strings.Contains(...)) patterns → assert.Contains(...) / assert.Regexp(...) where dual-format matching is required

• Why is this valuable?

  • Makes failures more actionable (better assertion output, less generic boolean failures).
  • Removes nil-panic foot-guns in test paths that should stop immediately on error.
  • Aligns test style with current testify best practices without broad refactoring.

• Implementation approach:

  • Scoped changes to targeted test files in cmd/gh-aw, pkg/agentdrain, pkg/cli, pkg/console, pkg/parser, and pkg/workflow.
  • No production/runtime code changes; no module/version changes.

// before
assert.Equal(t, true, mentions["enabled"])
assert.True(t, strings.Contains(err.Error(), "issue_ur") && strings.Contains(err.Error(), "issue_url"))
assert.NoError(t, err)

// after
assert.True(t, mentions["enabled"].(bool))
assert.Contains(t, err.Error(), "issue_ur")
assert.Contains(t, err.Error(), "issue_url")
require.NoError(t, err)

[Copilot]

Pull request overview

Mechanical cleanup of Go tests to align with testify assertion best practices (more idiomatic assert.* helpers and more fail-fast require.* usage) without changing runtime code or dependency versions.

Changes:

• Replaced boolean/len/equality anti-patterns with assert.True/False, assert.Len, assert.NotZero, assert.Contains, and assert.Regexp where appropriate.
• Switched several precondition-style assert.NoError calls to require.NoError to short-circuit tests on failure.
• Removed now-unused imports after assertion refactors (e.g., strings in a couple of tests).

Show a summary per file

File	Description
pkg/workflow/safe_outputs_config_generation_test.go	Updates multiple assertions to assert.True/False (with direct bool casts) and other helper alignment.
pkg/workflow/safe_outputs_call_workflow_test.go	Replaces strings.Contains OR checks with assert.Regexp for dual-quote matching.
pkg/workflow/mcp_gateway_entrypoint_mounts_e2e_test.go	Replaces Contains OR checks with a regex to tolerate escaped/unescaped formats.
pkg/workflow/lock_schema_test.go	Uses require.NoError instead of assert.NoError in the non-error branch.
pkg/workflow/compiler_test.go	Uses assert.Regexp instead of boolean Contains combinations for error text matching.
pkg/workflow/codex_logs_test.go	Replaces len(...) equality checks with assert.Len for clearer intent.
pkg/parser/import_conflict_test.go	Uses require.NoError where subsequent assertions depend on successful setup.
pkg/parser/import_cache_test.go	Uses require.NoError in the success path for table-driven validation.
pkg/parser/frontmatter_helpers_test.go	Refactors a boolean equality assertion to assert.True (with bool cast).
pkg/console/verbose_test.go	Tightens assertion to explicitly require the verbose icon when verbose output is expected.
pkg/console/progress_test.go	Replaces strings.Contains OR checks with assert.Regexp and removes unused strings import.
pkg/cli/workflows_count_test.go	Replaces assert.Equal(len(...), ...) with assert.Len.
pkg/cli/run_workflow_validation_test.go	Splits compound Contains checks into two assert.Contains and removes unused strings import.
pkg/cli/health_metrics_test.go	Replaces len(...) equality check with assert.Len.
pkg/cli/audit_expanded_test.go	Replaces len(...) equality check with assert.Len.
pkg/agentdrain/miner_test.go	Uses require.NoError inside concurrent training loop to fail fast.
cmd/gh-aw/main_entry_test.go	Replaces assert.NotEqual(0, ...) with assert.NotZero(...) for exit code.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (8)

pkg/workflow/safe_outputs_config_generation_test.go:186

• mentions["enabled"].(bool) / mentions["allowTeamMembers"].(bool) will panic if either key is missing or not a boolean. Consider checking key presence + type (val, ok := ....(bool)) with require.True(t, ok) before asserting the boolean value, so failures remain assertion-based and more diagnostic.

	mentions, ok := parsed["mentions"].(map[string]any)
	require.True(t, ok, "Expected mentions key in config")
	assert.True(t, mentions["enabled"].(bool), "enabled should be true")
	assert.False(t, mentions["allowTeamMembers"].(bool), "allowTeamMembers should be false")
	assert.InDelta(t, float64(5), mentions["max"], 0.0001, "max should be 5")

pkg/workflow/safe_outputs_config_generation_test.go:267

• Casting schema["additionalProperties"].(bool) can panic if the key is absent or not a boolean, which makes failures harder to interpret than an assertion diff. Consider asserting presence/type (require.Contains + require.IsType / safe type assertion) before assert.False.

				schema, ok := result["inputSchema"].(map[string]any)
				require.True(t, ok, "inputSchema should be a map")
				assert.Equal(t, "object", schema["type"], "schema type should be object")
				assert.False(t, schema["additionalProperties"].(bool), "additionalProperties should be false")
				props, ok := schema["properties"].(map[string]any)

pkg/workflow/safe_outputs_config_generation_test.go:478

• prConfig["draft"].(bool) will panic if the field is missing or not a boolean (e.g., if it becomes an expression string in the future). Consider a safe type assertion with require.True(t, ok) so test failures stay actionable.

	assert.Equal(t, "dev", prConfig["base_branch"], "base_branch should be set")
	assert.True(t, prConfig["draft"].(bool), "draft should be true")

pkg/workflow/safe_outputs_config_generation_test.go:490

• prConfig["fallback_as_issue"].(bool) will panic if the field is missing or not a boolean. Prefer asserting presence/type (val, ok := ....(bool) + require.True(t, ok)) before assert.False to avoid panic-style failures.

	assert.Equal(t, "[refactor] ", prConfig["title_prefix"], "title_prefix should be set")
	assert.False(t, prConfig["fallback_as_issue"].(bool), "fallback_as_issue should be false")

pkg/workflow/safe_outputs_config_generation_test.go:520

• Both allow_empty and auto_merge are read via direct .(bool) type assertions; a missing key or non-bool value will panic. Consider safe type assertions (with require.True(t, ok)) to preserve clear assertion failures.

	assert.InDelta(t, float64(2), prConfig["max"], 0.0001, "max should be 2")
	assert.True(t, prConfig["allow_empty"].(bool), "allow_empty should be true")
	assert.True(t, prConfig["auto_merge"].(bool), "auto_merge should be true")

pkg/workflow/safe_outputs_config_generation_test.go:615

• prConfig["auto_close_issue"].(bool) will panic if the serialized value ever becomes non-boolean (e.g., expression string). Consider asserting the type first (or doing a safe .(bool) with an ok check) before assert.False.

	prConfig, ok := parsed["create_pull_request"].(map[string]any)
	require.True(t, ok, "Expected create_pull_request key in config")

	assert.False(t, prConfig["auto_close_issue"].(bool), "auto_close_issue should be false")
}

pkg/workflow/safe_outputs_config_generation_test.go:829

• replyConfig["footer"].(bool) will panic if footer is missing or not a boolean. Consider a safe type assertion (val, ok := ....(bool) with require.True(t, ok)) to keep failures assertion-based.

	replyConfig, ok := parsed["reply_to_pull_request_review_comment"].(map[string]any)
	require.True(t, ok, "Expected reply_to_pull_request_review_comment key in config.json")
	assert.InDelta(t, float64(10), replyConfig["max"], 0.0001, "max should be 10")
	assert.Equal(t, "pull_request", replyConfig["target"], "target should be set")
	assert.Equal(t, "org/other-repo", replyConfig["target-repo"], "target-repo should be set")

	allowedRepos, ok := replyConfig["allowed_repos"].([]any)
	require.True(t, ok, "allowed_repos should be an array")
	assert.Len(t, allowedRepos, 1, "Should have 1 allowed repo")
	assert.Equal(t, "org/other-repo", allowedRepos[0], "allowed_repos entry should match")

	assert.True(t, replyConfig["footer"].(bool), "footer should be true")
}

pkg/workflow/safe_outputs_config_generation_test.go:905

• closePRConfig["staged"].(bool) will panic if the field is absent or not boolean. Consider asserting presence/type before assert.True so the test fails with an assertion message rather than a panic.

	closePRConfig, ok := parsed["close_pull_request"].(map[string]any)
	require.True(t, ok, "Expected close_pull_request key in config.json")

	assert.True(t, closePRConfig["staged"].(bool), "staged should be true")
	assert.Nil(t, closePRConfig["github-token"], "github-token should not be set when empty")

• Files reviewed: 17/17 changed files
• Comments generated: 1