[test-improver] Improve tests for sanitize package#1230
Merged
Conversation
- Replace manual strings.Contains + t.Errorf patterns with testify assertions - Add requireContainsRedacted test helper with t.Helper() to deduplicate logic - Fix assert.False(secretCount < 3) bug -> assert.GreaterOrEqual - Fix broken format strings in assert.Equal and assert.False calls - Use assert.NotContains instead of manual negative strings.Contains checks - Use require.NoError/require.True instead of t.Fatalf for error checks Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors and stabilizes the internal/logger/sanitize test suite by deduplicating repeated redaction assertions and standardizing on Testify assertions for clearer failure output.
Changes:
- Introduces a shared redaction assertion helper to reduce duplication across tests.
- Replaces manual
strings.Containschecks and mixed assertion patterns withassert.Contains/assert.NotContainsandrequire.NoError. - Fixes previously broken failure messages where format verbs were provided without format arguments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+17
to
+19
| assert.Contains(t, result, "[REDACTED]", "Expected sanitized output to contain [REDACTED]") | ||
| if mustNotContain != "" { | ||
| assert.NotContains(t, result, mustNotContain, "Sanitized output still contains secret") |
There was a problem hiding this comment.
requireContainsRedacted is named like a require.* helper (implying it stops the test on failure), but it uses assert.Contains/assert.NotContains. This is misleading and can allow the test to continue after the helper fails. Either rename it (e.g., assertContainsRedacted) or switch the assertions inside to require.Contains/require.NotContains to match the name/intent.
Suggested change
| assert.Contains(t, result, "[REDACTED]", "Expected sanitized output to contain [REDACTED]") | |
| if mustNotContain != "" { | |
| assert.NotContains(t, result, mustNotContain, "Sanitized output still contains secret") | |
| require.Contains(t, result, "[REDACTED]", "Expected sanitized output to contain [REDACTED]") | |
| if mustNotContain != "" { | |
| require.NotContains(t, result, mustNotContain, "Sanitized output still contains secret") |
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.
File Analyzed
internal/logger/sanitize/sanitize_test.gointernal/logger/sanitizeImprovements Made
1. Better Testify Usage
requireContainsRedactedtest helper usingt.Helper()to deduplicate repeated assertion logic across six test functionsif !strings.Contains(result, "[REDACTED]") { t.Errorf(...) }patterns withassert.Contains(t, result, "[REDACTED]", ...)if strings.Contains(result, secret) { t.Errorf(...) }patterns withassert.NotContains(t, result, secret, ...)assert.True(t, strings.Contains(...))with directassert.Contains/assert.NotContainst.Fatalf("Failed to parse result: %v", err)withrequire.NoError(t, err, ...)inTestSanitizeJSONWithInvalidJSONfmt.Sprintf("%v", payloadObj["_raw"])+ string comparison with a properstringtype assertion andassert.Contains2. Bug Fixes in Test Assertions
assert.Falsemisuse inTestSanitizeStringMultipleSecretsInSameString:assert.False(t, secretCount < 3, "got %d in: %s")had format verbs but no format arguments → changed toassert.GreaterOrEqual(t, secretCount, 3, "...", secretCount, result)which is both semantically correct and provides useful output on failureTestSecretPatternsCount:assert.Equal(t, expectedPatternCount, actualCount, "%d secret patterns, got %d")had format verbs but no format arguments → now passesexpectedPatternCountandactualCountas format args3. Cleaner & More Stable Tests
Why These Changes?
The file had a clearly inconsistent style — some test functions used proper testify assertions while others used verbose
if !strings.Contains(...) { t.Errorf(...) }patterns. Testify'sassert.Contains/assert.NotContainsprovide equivalent semantics with less boilerplate and better failure output.Two tests contained real bugs where format strings with
%d/%sverbs had no format arguments, meaning test failure messages would print the literal format string"got %d in: %s"instead of showing actual values. Theassert.False(t, count < 3, ...)pattern was also semantically misleading —assert.GreaterOrEqualdirectly expresses the intent and is idiomatic testify.Generated by Test Improver Workflow
Focuses on better patterns, increased coverage, and more stable tests