Current State
- File:
pkg/workflow/activation_checkout_test.go (152 lines, build-tagged integration)
- Source pair: no dedicated
activation_checkout.go; behavior is spread across pkg/workflow/compiler_activation_job.go (683 lines) and pkg/workflow/compiler_activation_steps.go (393 lines), notably generateCheckoutGitHubFolderForActivation, buildActivationJob, and the various shouldInclude*Reactions/shouldInclude*StatusComments helpers.
- Test count: 1 top-level test function (
TestActivationJobNoCheckoutStep), table-driven with 3 subtests.
- Testify usage: none — the file uses raw
if ... t.Error/t.Fatal checks exclusively (assert/require are not imported).
Strengths
- Table-driven structure with descriptive
name/description fields is a good foundation.
- Uses
testutil.TempDir for isolated temp workflow files.
- Covers a meaningful invariant (no full-repo checkout for activation timestamp check) across a few permission/reaction variants.
Prioritized Improvements
1. Missing/high-value tests
The source file compiler_activation_job.go has many small, easily-testable pure functions that are not exercised at all in this test file or elsewhere nearby:
shouldIncludeIssueReactions, shouldIncludePullRequestReactions, shouldIncludeDiscussionReactionsshouldIncludeIssueStatusComments, shouldIncludePullRequestStatusComments, shouldIncludeDiscussionStatusCommentsactivationEventSet, isActivationMetadataTriggerFieldbuildCentralizedCommandOnSectionlocalSkillSparseCheckoutTopLevelDirs, resolveSymlinkExtraPathsaddSameRepoIfConditionToSteps, injectIfConditionAfterName
These are unit-testable without a full compile+lock-file round trip, and would run fast without the integration build tag. Recommend adding direct unit tests (no build tag) for at least activationEventSet, buildCentralizedCommandOnSection, and injectIfConditionAfterName, since they contain non-trivial string/logic branching that the current integration test doesn't touch.
Additionally, consider a negative case: a workflow with contents: write permission — verify checkout behavior differs appropriately (currently only "no contents permission" and "issues read" variants are tested; there's no case asserting what happens when checkout is expected).
2. Testify assertion upgrades
Before/after example
Before (current style, manual string search + t.Fatal/t.Error):
lockContent, err := os.ReadFile(lockFile)
if err != nil {
t.Fatalf("Failed to read lock file: %v", err)
}
...
if !strings.Contains(lockContentStr, "activation:") {
t.Error("Expected activation job to be present")
}After (testify, require for setup, assert for validations):
lockContent, err := os.ReadFile(lockFile)
require.NoError(t, err, "Failed to read lock file")
lockContentStr := string(lockContent)
require.Contains(t, lockContentStr, "activation:", "Expected activation job to be present")
And for the negative checks currently written as manual if strings.Contains(...) { t.Errorf(...) }:
assert.NotContains(t, activationJobSection, "Checkout workflows",
"%s: Should not have 'Checkout workflows' step - uses GitHub API for timestamp checking", tt.description)
assert.Contains(t, activationJobSection, "Check workflow lock file",
"%s: Should contain timestamp check step", tt.description)
assert.Contains(t, activationJobSection, "require(",
"%s: Should load scripts via require()", tt.description)This removes ~10 lines of manual conditionals and gives consistent failure messages with diff-friendly output.
3. Table-driven refactors
The manual "find the activation job boundaries" logic (lines ~113–142) is complex, brittle (relies on indentation heuristics), and duplicated per subtest. Recommend extracting it into a small testable helper, e.g. extractJobSection(lockContent, jobName string) (string, error), placed in a _test.go helper or testutil package, then unit-testing that helper directly with a few small YAML fixtures (job at end of file, job followed by another job, job with nested multi-level indentation). This both reduces duplication if more job-boundary tests are added later, and lets the boundary-detection logic itself be verified independently of the full compiler pipeline.
4. Organization/readability
- The file lacks any
assert/require imports — adding github.com/stretchr/testify/require and .../assert aligns it with the rest of the codebase's convention (see scratchpad/testing.md).
- The subtest
description field duplicates information already conveyed by t.Errorf("%s: ...", tt.description) — since t.Run(tt.name, ...) already scopes failures under tt.name, the repeated %s: prefix is redundant; assertion messages could drop tt.description in favor of just using tt.name from the subtest context, or keep it only where it clarifies why, not what.
- Consider renaming the file to
compiler_activation_checkout_test.go or moving/splitting it to sit next to compiler_activation_job.go for easier co-location (matches the "Tests are co-located with implementation files" convention in scratchpad/testing.md).
Acceptance Checklist
Generated by 🧪 Daily Testify Uber Super Expert · auto · 22.6 AIC · ⌖ 3.64 AIC · ⊞ 7.1K · ◷
Current State
pkg/workflow/activation_checkout_test.go(152 lines, build-taggedintegration)activation_checkout.go; behavior is spread acrosspkg/workflow/compiler_activation_job.go(683 lines) andpkg/workflow/compiler_activation_steps.go(393 lines), notablygenerateCheckoutGitHubFolderForActivation,buildActivationJob, and the variousshouldInclude*Reactions/shouldInclude*StatusCommentshelpers.TestActivationJobNoCheckoutStep), table-driven with 3 subtests.if ... t.Error/t.Fatalchecks exclusively (assert/requireare not imported).Strengths
name/descriptionfields is a good foundation.testutil.TempDirfor isolated temp workflow files.Prioritized Improvements
1. Missing/high-value tests
The source file
compiler_activation_job.gohas many small, easily-testable pure functions that are not exercised at all in this test file or elsewhere nearby:shouldIncludeIssueReactions,shouldIncludePullRequestReactions,shouldIncludeDiscussionReactionsshouldIncludeIssueStatusComments,shouldIncludePullRequestStatusComments,shouldIncludeDiscussionStatusCommentsactivationEventSet,isActivationMetadataTriggerFieldbuildCentralizedCommandOnSectionlocalSkillSparseCheckoutTopLevelDirs,resolveSymlinkExtraPathsaddSameRepoIfConditionToSteps,injectIfConditionAfterNameThese are unit-testable without a full compile+lock-file round trip, and would run fast without the
integrationbuild tag. Recommend adding direct unit tests (no build tag) for at leastactivationEventSet,buildCentralizedCommandOnSection, andinjectIfConditionAfterName, since they contain non-trivial string/logic branching that the current integration test doesn't touch.Additionally, consider a negative case: a workflow with
contents: writepermission — verify checkout behavior differs appropriately (currently only "no contents permission" and "issues read" variants are tested; there's no case asserting what happens when checkout is expected).2. Testify assertion upgrades
Before/after example
Before (current style, manual string search +
t.Fatal/t.Error):After (testify,
requirefor setup,assertfor validations):And for the negative checks currently written as manual
if strings.Contains(...) { t.Errorf(...) }:This removes ~10 lines of manual conditionals and gives consistent failure messages with diff-friendly output.
3. Table-driven refactors
The manual "find the activation job boundaries" logic (lines ~113–142) is complex, brittle (relies on indentation heuristics), and duplicated per subtest. Recommend extracting it into a small testable helper, e.g.
extractJobSection(lockContent, jobName string) (string, error), placed in a_test.gohelper ortestutilpackage, then unit-testing that helper directly with a few small YAML fixtures (job at end of file, job followed by another job, job with nested multi-level indentation). This both reduces duplication if more job-boundary tests are added later, and lets the boundary-detection logic itself be verified independently of the full compiler pipeline.4. Organization/readability
assert/requireimports — addinggithub.com/stretchr/testify/requireand.../assertaligns it with the rest of the codebase's convention (seescratchpad/testing.md).descriptionfield duplicates information already conveyed byt.Errorf("%s: ...", tt.description)— sincet.Run(tt.name, ...)already scopes failures undertt.name, the repeated%s:prefix is redundant; assertion messages could droptt.descriptionin favor of just usingtt.namefrom the subtest context, or keep it only where it clarifies why, not what.compiler_activation_checkout_test.goor moving/splitting it to sit next tocompiler_activation_job.gofor easier co-location (matches the "Tests are co-located with implementation files" convention inscratchpad/testing.md).Acceptance Checklist
testifyassert/requireimports and replace manualif err != nil { t.Fatal }/if !strings.Contains { t.Error }patternsintegrationbuild tag) for at leastactivationEventSet,buildCentralizedCommandOnSection,injectIfConditionAfterNamecontents: writepermission is present, asserting expected checkout behaviormake test-unitand confirm all tests passmake fmtafter any Go changes