Improve test quality: migrate tracker_id_integration_test.go to testify assertions - #51178
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Improves tracker ID integration tests with Testify, broader validation coverage, and safer temporary-file cleanup.
Changes:
- Migrates assertions to Testify.
- Adds invalid and multi-output tracker ID cases.
- Uses per-subtest temporary directories and extracts a shared assertion helper.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/tracker_id_integration_test.go |
Expands and refactors tracker ID integration tests. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Balanced
| // mode, not inline). | ||
| func assertScriptUsesRequire(t *testing.T, contentStr string) { | ||
| t.Helper() | ||
| assert.Contains(t, contentStr, "GH_AW_TRACKER_ID", "expected GH_AW_TRACKER_ID environment variable to be set") |
| if tt.shouldCompile { | ||
| require.NoError(t, err, "expected compilation to succeed") | ||
| } else { | ||
| require.Error(t, err, "expected compilation to fail") |
|
@copilot lint go |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in 77a723c. The referenced custom lint checks now pass. |
|
🎯 Excellent work on the test quality improvements! This PR tackles exactly what the testify-expert review flagged in #51152 — migrating the What stands out:
This PR is ready for review and merge. The changes directly improve maintainability and test signal quality, and align perfectly with the project's test-driven agentic workflow.
|
|
@copilot Quick triage nudge for this PR. Please refresh the branch, address any remaining review feedback, run the
Run: https://github.com/github/gh-aw/actions/runs/31218745745
|
…fy assertions (#51178) * Initial plan * Improve test quality: migrate tracker_id_integration_test.go to testify Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Fix engine definition custom lint findings Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> * Use consistent engine import empty checks Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> Co-authored-by: Peli de Halleux <pelikhan@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
pkg/workflow/tracker_id_integration_test.gorelied entirely on rawt.Fatalf/t.Errorf, lacked coverage for invalidtracker-idvalues despite documented validation constraints (min 8 chars, alphanumeric + hyphens/underscores), and used manualos.Removecleanup that could be skipped on early test failures.t.Fatalf/t.Errorfwithrequire.NoError/require.Errorfor compile-step assertions andassert.Contains/assert.NotContainsfor content checks, so a subtest can surface multiple independent failures instead of stopping at the first one.create-issue+create-pull-request) in the same workflow.tmpDirviatestutil.TempDir, which registerst.Cleanupinternally, removing the need for manualos.Removecalls at the end of the subtest and ensuring cleanup runs on early failures or panics. This also removes the shared-filename risk across subtests, making them safe for futuret.Parallel().assertScriptUsesRequire(t, contentStr)for the repeated "env var set + script loaded viarequire()" assertion pattern, intended to be reusable across sibling*_integration_test.gofiles.