[testify-expert] Improve Test Quality: pkg/workflow/tracker_id_integration_test.go

[github-actions]

Current State

• File: pkg/workflow/tracker_id_integration_test.go (build-tagged (go/redacted):build integration)
• Paired source: no single dedicated source file; logic spans pkg/workflow/frontmatter_types.go, pkg/workflow/workflow_data.go, pkg/workflow/safe_outputs_env.go, pkg/workflow/compiler_safe_outputs_job.go (tracker-id frontmatter field → GH_AW_TRACKER_ID env var)
• Tests: 1 test function (TestTrackerIDIntegration), 3 table-driven subtests
• LOC: 151

Strengths

• Uses a table-driven structure for the 3 scenarios (with tracker-id, without, PR context).
• Covers both presence and absence of GH_AW_TRACKER_ID in the compiled lock file.
• Cleans up generated lock/workflow files after each subtest.

Prioritized Improvements

1. Missing/high-value tests

• No case for an invalid tracker-id (source comment on workflow_data.go:52 states "min 8 chars, alphanumeric + hyphens/underscores" — there's no test asserting compilation fails or the value is rejected for e.g. "short", "has spaces", or "bad!chars").
• No case verifying tracker-id propagates correctly when multiple safe-outputs are configured together (e.g., both create-issue and create-pull-request in the same workflow).
• No assertion on the exact placement/format of GH_AW_TRACKER_ID beyond substring checks — e.g., no check that the value doesn't leak into unrelated output steps.

2. Testify assertion upgrades

The file uses raw t.Fatalf/t.Errorf throughout with 0 testify usage. Given the rest of the pkg/workflow suite relies on testify/assert and testify/require, migrating improves failure messages and readability.

Before / after example

Before:

err = compiler.CompileWorkflow(workflowFile)

if tt.shouldCompile && err != nil {
    t.Fatalf("Expected compilation to succeed, got error: %v", err)
}
if !tt.shouldCompile && err == nil {
    t.Fatal("Expected compilation to fail, but it succeeded")
}

After:

err = compiler.CompileWorkflow(workflowFile)

if tt.shouldCompile {
    require.NoError(t, err, "expected compilation to succeed")
} else {
    require.Error(t, err, "expected compilation to fail")
}

Before:

if tt.shouldHaveEnvVar {
    envVarLine := "GH_AW_TRACKER_ID: \"" + tt.expectedTrackerID + "\""
    if !strings.Contains(contentStr, envVarLine) {
        t.Errorf("Expected lock file to contain env var '%s', but it didn't", envVarLine)
    }
} else {
    envVarLine := "GH_AW_TRACKER_ID: \""
    if strings.Contains(contentStr, envVarLine) {
        t.Error("Expected lock file to NOT set GH_AW_TRACKER_ID env var, but it did")
    }
}

After:

if tt.shouldHaveEnvVar {
    envVarLine := "GH_AW_TRACKER_ID: \"" + tt.expectedTrackerID + "\""
    assert.Contains(t, contentStr, envVarLine, "expected lock file to declare tracker-id env var")
} else {
    assert.NotContains(t, contentStr, "GH_AW_TRACKER_ID: \"", "lock file should not set tracker-id env var")
}

3. Table-driven refactors

• The subtest body mixes require-style (fatal) and assert-style (non-fatal) checks manually. Using require.NoError/require.Error for the compile step and assert.Contains/assert.NotContains for content checks would let a single subtest report multiple independent assertion failures instead of stopping at the first t.Fatalf.
• Consider extracting the "shouldHaveInScript" checks (env var + require() into a small helper assertScriptUsesRequire(t, contentStr) shared across similar integration tests, since this pattern likely repeats in sibling *_integration_test.go files.

4. Organization/readability

• File cleanup uses manual os.Remove(lockFile) / os.Remove(workflowFile) at the end of the subtest body; prefer t.Cleanup(func() { ... }) right after each file is created, so cleanup still runs on early t.Fatalf/panic paths.
• tmpDir is created once outside the subtests via testutil.TempDir, but all subtests write to test.md in the same directory — a t.Run could clash if tests were parallelized in the future; consider a per-subtest unique filename or subdirectory for safety and to support future t.Parallel().

Acceptance Checklist

• Add invalid tracker-id test case(s) validating min-length/character constraints
• Migrate assertions to testify/require (fatal) and testify/assert (non-fatal) per guidance above
• Add t.Cleanup for generated files instead of manual os.Remove at end of subtest
• (Optional) Add case for tracker-id with multiple safe-outputs configured together
• make test-unit passes after changes (note: this file is (go/redacted):build integration tagged — also verify with the integration build tag if applicable)

Generated by 🧪 Daily Testify Uber Super Expert · auto · 21.2 AIC · ⊞ 7.2K · ◷

• expires on Aug 9, 2026, 10:15 AM UTC-08:00

[github-actions]

🍪 Issue Monster selected this for Copilot

I have identified this issue as a good candidate for automated resolution and requested assignment to the Copilot coding agent.

If assignment succeeds, the Copilot coding agent will analyze the issue and create a pull request with the fix.

Om nom nom! 🍪

🍪 Om nom nom by Issue Monster · gpt54 · 4.7 AIC · ⊞ 10.2K · ◷