tests: replace most t.Fatalf calls#1883
Merged
AndrewChubatiuk merged 1 commit intomasterfrom Feb 25, 2026
Merged
Conversation
Contributor
There was a problem hiding this comment.
6 issues found across 29 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/controller/operator/factory/vmagent/podscrape_test.go">
<violation number="1" location="internal/controller/operator/factory/vmagent/podscrape_test.go:32">
P2: This assert does not stop the test on error, unlike the previous return path. If generatePodScrapeConfig fails, the test continues and may panic or produce confusing failures. Use a fatal assertion or return on failure.</violation>
<violation number="2" location="internal/controller/operator/factory/vmagent/podscrape_test.go:34">
P2: This assert doesn't halt the test on marshal failure, unlike the previous early return. Consider returning on failure (or use require.NoError) to keep test failures focused on the real error.</violation>
</file>
<file name="internal/controller/operator/factory/vmalertmanager/config_test.go">
<violation number="1" location="internal/controller/operator/factory/vmalertmanager/config_test.go:58">
P3: Non-fatal assert lets the test continue after an unexpected error, which can dereference `got` and hide the real failure. Guard the assertion to return early on failure.</violation>
</file>
<file name="internal/controller/operator/factory/vmagent/nodescrape_test.go">
<violation number="1" location="internal/controller/operator/factory/vmagent/nodescrape_test.go:33">
P2: `assert.NoError` doesn’t stop the test. If `generateNodeScrapeConfig` returns an error, the test continues and uses `got`, which can cause misleading failures. Add an early return (or use require.NoError) to match the previous behavior.</violation>
<violation number="2" location="internal/controller/operator/factory/vmagent/nodescrape_test.go:35">
P2: `assert.NoError` here doesn’t stop execution, so the test can still compare `gotBytes` after a marshal failure. Add an early return (or use require.NoError) to avoid cascading failures.</violation>
</file>
<file name="internal/controller/operator/factory/vlsingle/vlogs_test.go">
<violation number="1" location="internal/controller/operator/factory/vlsingle/vlogs_test.go:33">
P2: This assertion block no longer short-circuits the test on expected/actual errors, so the test continues to build and fetch services even when an error path is triggered. Add a return for the error case (or use require.*) to avoid executing the rest of the test on failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
internal/controller/operator/factory/vmagent/nodescrape_test.go
Outdated
Show resolved
Hide resolved
internal/controller/operator/factory/vmagent/nodescrape_test.go
Outdated
Show resolved
Hide resolved
Comment on lines
+33
to
37
| if o.wantErr { | ||
| assert.Error(t, err) | ||
| } else { | ||
| assert.NoError(t, err) | ||
| } |
Contributor
There was a problem hiding this comment.
P2: This assertion block no longer short-circuits the test on expected/actual errors, so the test continues to build and fetch services even when an error path is triggered. Add a return for the error case (or use require.*) to avoid executing the rest of the test on failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/vlsingle/vlogs_test.go, line 33:
<comment>This assertion block no longer short-circuits the test on expected/actual errors, so the test continues to build and fetch services even when an error path is triggered. Add a return for the error case (or use require.*) to avoid executing the rest of the test on failure.</comment>
<file context>
@@ -30,9 +30,10 @@ func TestCreateOrUpdateVLogs(t *testing.T) {
- if (err != nil) != o.wantErr {
- t.Errorf("CreateOrUpdateVLogs() error = %v, wantErr %v", err, o.wantErr)
- return
+ if o.wantErr {
+ assert.Error(t, err)
+ } else {
</file context>
Suggested change
| if o.wantErr { | |
| assert.Error(t, err) | |
| } else { | |
| assert.NoError(t, err) | |
| } | |
| if o.wantErr { | |
| assert.Error(t, err) | |
| return | |
| } | |
| assert.NoError(t, err) |
8c265b4 to
443e620
Compare
| t.Errorf("CreateOrUpdateService() error = %v, wantErr %v", err, o.wantErr) | ||
| return | ||
| if o.wantErr { | ||
| assert.Error(t, err) |
Contributor
There was a problem hiding this comment.
why not just return after error check instead of placing a whole successful path into else?
Collaborator
Author
There was a problem hiding this comment.
Yeah, this is even worse.
I was attempting to find the middle ground between
if o.wantErr {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
and
if o.wantErr {
assert.Error(t, err)
return
}
assert.NoError(t, err)
svc := build.Service(o.cr, o.cr.Spec.Port, nil)
Testify asserts produce a significantly better output
443e620 to
e9909ed
Compare
AndrewChubatiuk
approved these changes
Feb 25, 2026
vrutkovs
added a commit
that referenced
this pull request
Mar 3, 2026
Testify asserts produce a significantly better output
AndrewChubatiuk
pushed a commit
that referenced
this pull request
Mar 3, 2026
Testify asserts produce a significantly better output
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
2 participants
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.
Testify asserts produce a significantly better output
Summary by cubic
Replaced most t.Fatalf/t.Errorf in tests with testify assertions for clearer failures and cleaner test code. No functional changes; tests only.
Written for commit e9909ed. Summary will update on new commits.