Harden acceptance-test mode guards - #3602
Open
rilical wants to merge 17 commits into
Open
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…egression harness Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…lper Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…e tests Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The static completeness guard's extractCalls/scanBlock only inspected
direct ExprStmt/AssignStmt statements and only recursed into direct
t.Run(...) closures. This let org-scoped must*/skip-precedence
violations nested inside RangeStmt/ForStmt/IfStmt/SwitchStmt bodies,
or inside func literals embedded in composite-literal or call
arguments (e.g. resource.TestCase{CheckDestroy: func() error {...}}),
evade detection entirely.
Replace extractCalls/scanBlock with a full recursive walk:
- scanExpr: examines CallExpr/FuncLit/CompositeLit/KeyValueExpr/
UnaryExpr/ParenExpr/BinaryExpr, recognizing t.Run subtests,
guard-func calls, org-scoped violation calls, and recursing into
any nested func literal reachable through call args or
composite-literal element values.
- scanStmt: walks every statement kind (ExprStmt, AssignStmt,
GoStmt, DeferStmt, ReturnStmt, SendStmt, LabeledStmt, BlockStmt,
IfStmt, ForStmt, RangeStmt, SwitchStmt, TypeSwitchStmt), threading
a guarded bool through in lexical order. Unconditionally executed
constructs (Init clauses) propagate a guard forward;
conditionally/repeatedly executed bodies (if/for/range/switch
case bodies) do not leak a guard found inside them back out to
the enclosing scope, since those bodies are not guaranteed to run
before, or even run at all relative to, later sibling statements.
- scanStmts: threads guarded sequentially across a statement list.
Added two failing-first fixture regression tests proving the gap and
then the fix:
- TestFindModeGuardViolations_DetectsRangeLoopNestedTRun: an
unguarded mustCreateTestTeam inside a t.Run nested in a for-range
loop.
- TestFindModeGuardViolations_DetectsFuncLitInCompositeLiteral: an
unguarded mustCreateTestTeam inside a func literal assigned as a
composite-literal field value.
Both were confirmed RED (0 violations detected) against the old
scanner and GREEN after the rewrite. The original fixture test
(TestFindModeGuardViolations_DetectsUnguardedOrgSetup) and the
completeness guard against the real github/*_test.go tree
(TestModeGuardPrecedesOrgScopedSetup_Static) both continue to pass
with no new false positives or newly-discovered real violations.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Independent re-review requested explicit fixture coverage for five
unguarded forms: range-loop-nested t.Run, composite-literal func
closure, if-body, switch-case, and var-declaration initializer.
Adding fixture tests for all five against the current scanner showed
four already pass (range/composite-literal were fixed previously;
if-body/switch-case were already covered by the existing IfStmt/
SwitchStmt handling in scanStmt). The fifth genuinely failed:
TestFindModeGuardViolations_DetectsVarDeclInitializer
acc_mode_guard_static_test.go:633: expected exactly 1 violation
(from fixture_vardecl_bad_test.go), got 0: []
scanStmt had no case for *ast.DeclStmt, so a call like
`var team = mustCreateTestTeam(t)` inside a function body fell
through to the default case and was silently skipped.
Fix: add a DeclStmt case to scanStmt that walks each GenDecl's
ValueSpec.Values and scans them as expressions, propagating the
guarded state forward exactly like AssignStmt (a var declaration
executes unconditionally in its enclosing block, same as any other
direct statement).
All five fixture tests now pass, plus the original two fixture tests
and the completeness guard against the real github/*_test.go tree
(still 0 violations — no new false positives).
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Extend the lexical AST scanner to distinguish general acceptance configuration guards from mode/capability guards while detecting testAccConf-rooted reads. Guard every surfaced acceptance-test site and cover the 37 affected TestAcc functions with hermetic subprocess regressions. Co-authored-by: GitHub Copilot <copilot@github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
👋 Hi, and thank you for this contribution! This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can. You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions. 🤖 This is an automated message. |
Contributor
Author
|
@copilot review |
Remove the agent scratch plan committed under docs/superpowers/ and add the directory to .gitignore so planning artifacts stay out of the repo. Make the acc mode-guard allowlist comment self-contained now that the referenced plan file is gone. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Acceptance tests now stop safely before touching credentials, organization setup, or provider metadata that the selected test mode cannot use.
Two concrete provider bugs are fixed:
getTestMetaand friends) requested a token / GitHub App even whenGH_TEST_AUTH_MODE=anonymous, so credential-free tests likeTestAccGithubIpRangesDataSourceexited before running.must*setup helpers and directtestAccConfreads execute beforeresource.Test, i.e. beforePreCheck. WithTF_ACCunset that panics on a niltestAccConf; with an individual-account PAT it fails against the wrong account. These now fail closed with an inline mode guard.User-visible result: anonymous acceptance tests run without credentials, and org/team/enterprise tests can no longer panic or create resources in the wrong account when run outside their mode.
How this was found
We ran this provider's acceptance suite through that harness in anonymous mode. The run failed before any test executed because setup still demanded credentials (bug 1). Auditing that path surfaced the pre-
resource.Testguard gap (bug 2) across a set of setup helpers andtestAccConfreads.The fix is enforced two ways:
TestModeGuardPrecedesOrgScopedSetup_Static) that rejects unsafe setup added by future contributors, including nested control flow and closures, so this can't silently regressValidation
make testgo vet ./...CGO_ENABLED=0 go build ./...gofmt -l .is emptygit diff --check origin/main...HEADTestAccGithubIpRangesDataSourcein anonymous mode (no credentials), which now runs instead of exiting earlyFiles
github/acc_test.go: nil-safe acceptance and mode guardsgithub/acc_mode_guard_static_test.go: recursive static enforcement of the guard-before-setup rulegithub/acc_mode_guard_test.go: hermetic subprocess regressionsgithub/**/*_test.gocall sites: inline mode checks before org/team/enterprise setup andtestAccConfreads.github/instructions/tests.instructions.md: documents the rule for contributors and reviewersScope and limitations
mustCreateTestRepository) can still panic if a contributor directly invokes a matchingTestAcc...withTF_ACCunset. Normal CI excludes acceptance tests (-skip '^TestAcc') and the new scanner covers the mode-sensitive helpers; the narrower direct-invocation case can be handled separately if wanted.Supersedes closed draft #3595 with the complete guard set and current
mainmerged.