testifylint: enable the go-require linter and fix all findings#20397
Open
arthurschreiber wants to merge 4 commits into
Open
testifylint: enable the go-require linter and fix all findings#20397arthurschreiber wants to merge 4 commits into
arthurschreiber wants to merge 4 commits into
Conversation
Enables testifylint with only the go-require checker turned on. go-require flags require.* calls made from a non-test goroutine: require calls t.FailNow() -> runtime.Goexit(), which only terminates the test correctly on the test goroutine. Off-goroutine it kills just that goroutine, so the test can hang on a never-closed channel or report a false pass. The remaining testifylint checkers are being adopted separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
The go-require checker flags require.* calls made from a non-test
goroutine. require calls t.FailNow() -> runtime.Goexit(), which only
terminates the test correctly on the main test goroutine; off-goroutine
it kills just that goroutine, so the test can hang on a channel that
never closes or report a false pass.
Fixes the 9 findings in packages that run in unit CI:
- topo/test/{shard,trylock}.go: replace require.Fail with assert.Fail +
return in the lock-test goroutines, preserving the existing
timeout-on-failure behavior.
- vtctl/workflow/utils_test.go: the goroutine-only update() helper now
uses assert + return.
- mysql/conn_test.go: hoist createSocketPair (a require-using helper
shared by 18 callers, all but one on the main goroutine) out of the
goroutine.
- vttablet/tabletmanager/tm_init_test.go: grantAllPrivilegesToUser now
returns an error, so the main caller requires it while the
delayed-grant goroutine asserts, guarded by ctx.Err() to avoid
asserting after the test completes.
- vttablet/tabletserver/vstreamer/vstreamer_test.go: hoist
startFullyThrottledStream setup out of the goroutine.
go-require stays disabled in config until the remaining endtoend
findings are fixed in a follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
execVtgateQuery and insertLargeTransactionForChunkTesting (vreplication endtoend test helpers) asserted internally via require, which is unsafe when called from a goroutine: require's FailNow only terminates the test correctly on the test goroutine. Rather than carry a parallel error-returning "Err" twin, make the helpers themselves return an error and have every caller require/assert on it. - execVtgateQuery(conn, db, query) (*sqltypes.Result, error): dropped the *testing.T parameter; callers own the assertion. - insertLargeTransactionForChunkTesting(conn, ks, startID) error: likewise. - ~108 call sites updated: require.NoError on the test goroutine, assert.NoError in the vstream inserter goroutines (with the existing mutex-unlock-before-return preserved). Pure refactor with no behavior change for existing test-goroutine callers. Prep for enabling the testifylint go-require checker. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Fixes the remaining go-require findings across go/test/endtoend. To make the vdiff helpers safe to call from goroutines (rather than carry an error-returning "WithErr" twin), two shared helpers now return an error: - waitForWorkflowState(vc, ksWorkflow, wantState, ...) error: dropped the *testing.T param and returns an error instead of calling require; its callers now wrap it in require.NoError. This keeps it require-free so it can run inside a goroutine. - performVDiff2Action(...) (uuid, output string, err error): returns an error and waits for the workflow internally via the now-require-free waitForWorkflowState, so its goroutine callers can use it with assert.NoError. Other go-require fixes: - Goroutine-only helpers (verifyDisableEnableRedoLogs, runSingleConnection, runInTransaction, vtgateExec, runFuzzerThread and its compare helpers, populate): require -> assert with explicit return/continue to preserve the abort behavior require's Goexit provided. - checkTabletType (void helper): body converted to assert, making it safe to call from the backup goroutines without weakening its test-goroutine callers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Arthur Schreiber <arthur@planetscale.com>
Contributor
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20397 +/- ##
===========================================
- Coverage 69.67% 69.33% -0.34%
===========================================
Files 1614 279 -1335
Lines 216793 46990 -169803
===========================================
- Hits 151044 32580 -118464
+ Misses 65749 14410 -51339
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
mattlord
approved these changes
Jun 25, 2026
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
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.
Description
This enables
testifylint'sgo-requirechecker and fixes every finding across the codebase.go-requireflagsrequire.*calls made from a non-test goroutine.requirecallst.FailNow()→runtime.Goexit(), which only terminates the test correctly on the goroutine running the test function. Off that goroutine it kills just the spawned goroutine, so the test can hang on a channel that never closes, or report a false pass — a real correctness problem, not a style nit.The PR is structured so each piece is easy to review:
.golangci.ymlwith onlygo-requireturned on (disable-all: true+enable: [go-require]). The other testifylint checkers are being adopted separately.conn_test,tm_init_test,vstreamer_test, workflowutils_test):require→assertinside the spawned goroutines, with explicitreturn/continueso the abort behaviourrequire'sGoexitprovided is preserved. Shared helpers used from both the test goroutine and a child goroutine (createSocketPair,grantAllPrivilegesToUser,startFullyThrottledStream) are fixed at the goroutine call site so their main-goroutine callers stay fail-fast.execVtgateQuery/insertLargeTransactionForChunkTestingnow return an error instead of asserting internally (the*testing.Tparameter is dropped); ~108 call sites updated torequire.NoErroron the test goroutine andassert.NoErrorin the inserter goroutines. This avoids a parallel error-returning "twin" helper.waitForWorkflowStateandperformVDiff2Actionreturn errors so they are goroutine-safe (again, instead of carrying a twin).performVDiff2Actionkeeps waiting for the workflow internally via the now-require-freewaitForWorkflowState.go vetpasses across every touched package, andgolangci-lint run --max-same-issues 0 --max-issues-per-linter 0 ./go/...reports 0go-requirefindings. The unit-test fixes were verified locally withgo test -race; the endtoend changes rely on CI for behavioural validation.Related Issue(s)
Checklist
Deployment Notes
None — this only touches test code and the linter config.
AI Disclosure
This PR was written primarily by Claude Code — I provided direction and reviewed the changes.