chore: replace t.Errorf/t.Fatalf with assert/require in go/vt/servenv#19518
chore: replace t.Errorf/t.Fatalf with assert/require in go/vt/servenv#19518ManthanNimodiya wants to merge 1 commit intovitessio:mainfrom
Conversation
Part of vitessio#15182. Modernizes test assertions in the servenv package: - t.Fatalf → require.NoError for fatal checks in normal test flow - t.Errorf → assert.NoError inside goroutines (require unsafe in goroutines) - Range checks converted to assert.True with equivalent semantics
ManthanNimodiya
requested review from
dbussink and
timvaillancourt
as code owners
vitess-bot
bot
added
NeedsWebsiteDocsUpdate
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
|
|
I don't personally want to review a hundred of these kinds of PRs. I think that we should do it all in one PR, so that one PR resolves the issue. This is the kind of thing that OpenCode/Claude/Codex/Cursor etc could do very quickly and easily now — across all of the tests. |
|
Thanks for the feedback, @mattlord! Just to confirm: you'd prefer one PR that replaces all t.Errorf/t.Fatalf occurrences across the entire codebase at once? |
I would personally, yes. It should be something that e.g. Claude could kick out in literally a minute or so. Then we have one PR to review, one set of CI runs, etc. We can also ask AI to review it as well. :-D |
There was a problem hiding this comment.
Pull request overview
Modernizes go/vt/servenv tests by replacing t.Errorf/t.Fatalf usages with testify/assert and testify/require, aligning with the broader test-suite modernization effort in #15182.
Changes:
- Convert fatal error checks to
require.NoErrorinliveness_test.goandexporter_test.go. - Convert non-fatal validations to
assert.NoError/assert.Trueinmetrics_test.go. - Update imports accordingly to include
testifyhelpers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go/vt/servenv/metrics_test.go | Replaces manual error/range checks with assert-based validations for CPU/memory metric helpers. |
| go/vt/servenv/liveness_test.go | Replaces t.Fatalf error handling with require.NoError for HTTP request/response reads. |
| go/vt/servenv/exporter_test.go | Replaces listener setup fatal check with require.NoError and uses assert.NoError for the HTTP server goroutine. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| go func() { | ||
| err := HTTPServe(listener) | ||
| if err != nil { | ||
| t.Errorf("HTTPServe returned: %v", err) | ||
| } | ||
| assert.NoError(t, err) | ||
| }() |
There was a problem hiding this comment.
assert.NoError(t, err) is being called from a goroutine. Using *testing.T (and testify assertions that wrap it) from a goroutine without synchronization can race with the test finishing and can trigger "testing: ... after Test... has completed" panics/flakes. Consider sending the HTTPServe result on a channel and asserting in the main test goroutine (or wait for the goroutine to exit before TestHandleFunc returns).
Description
Replace
t.Errorfandt.Fatalfcalls with testifyassertandrequireequivalentsin the
go/vt/servenvpackage, as part of the test suite modernization tracked in #15182.t.Fatalf(...)→require.NoError(t, err)for fatal error checks in normal test flowt.Errorf(...)→assert.NoError(t, err)for non-fatal checks (including inside goroutineswhere
requirecannot be used safely)assert.True(t, condition, msg)with equivalent semanticsRelated Issue(s)
Fixes part of #15182
Checklist
Deployment Notes
No deployment impact — test-only changes.
AI Disclosure
This PR was written with assistance from AI