Conversation
- Replace ad‑hoc teardown with t.Cleanup and assert removal via require.NoError across io/auth/mfa/converters/memory tests for more reliable cleanup. - Use secure file permissions in io tests (0600 instead of 0644). - Capture/log errors from SecureWriteFromReader and similar calls; standardize error verification in tests. - Introduce constants (e.g., JWT issuer, error messages) to de‑duplicate assertions and clarify expectations. - Wrap selected test errors with ewrap for consistent types. - Add targeted nolint directives to silence gosec false positives on test secrets. - Update .golangci.yaml allow-strs to include alice to reduce lint noise. No production code changes; test robustness and lint signal improved.
• Replace inline t.Fatalf strings with shared constants (e.g., errMsgExpectedSigner/Token, errMsgUnexpected, errMsgDetector, errMsgExpectedRedactor) across jwt, paseto, converters, encoding, io, memory, mfa, password, sanitize, tokens, validate, and secrets packages. • Normalize test data (testValue, testValueInt64, int8Value, testEmail) and apply minor cleanup. • Refactor helper usage (createTempFile/createTempDir) for consistency. • Chore: expand .golangci.yaml allow-strs/allow-ints and add “unparam” to cspell ignore list. No production code changes; improves test consistency and prepares for stricter linting.
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving test code quality through linting fixes and standardization. The changes extract repeated error messages into constants, improve error handling in test cleanup functions, and add appropriate linter suppression directives.
Changes:
- Extracted repeated error message strings into constants across test files to reduce duplication and improve maintainability
- Improved error handling in test cleanup functions by checking and reporting errors instead of silently ignoring them
- Added appropriate
//nolintdirectives for legitimate linter warnings (gosec for test secrets, revive for test setup variables) - Updated linter configuration to allow commonly used test strings and constants
- Changed
t.Errorftot.Errorandt.Fatalftot.Fatalwhere no format arguments are needed
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/validate/url_test.go | Extracted error message constant |
| pkg/validate/email_test.go | Extracted error message and test data constants |
| pkg/tokens/tokens_test.go | Extracted error message constant |
| pkg/secrets/secrets_test.go | Extracted error message constants and added nolint directives |
| pkg/sanitize/sql_test.go | Uses error message constant (missing definition - critical issue) |
| pkg/sanitize/nosql_detect_test.go | Extracted error message constant |
| pkg/password/bcrypt_test.go | Changed t.Fatalf to t.Fatal for non-formatted messages |
| pkg/password/argon2id_test.go | Changed t.Fatalf to t.Fatal for non-formatted messages |
| pkg/mfa/totp_test.go | Uses constants and added nolint directives for test setup |
| pkg/mfa/rate_limit_test.go | Extracted constants and simplified function call |
| pkg/mfa/hotp_test.go | Extracted constants and added nolint directives |
| pkg/mfa/backup_test.go | Added nolint directive |
| pkg/memory/secure_buffer_test.go | Changed t.Errorf to t.Error and added nolint directives |
| pkg/memory/reader_test.go | Extracted constant and standardized on ewrap for errors |
| pkg/io/temp_test.go | Improved error handling in cleanup functions |
| pkg/io/file_test.go | Improved error handling, updated function signatures, simplified createTempDir |
| pkg/io/file_bench_test.go | Improved error handling in cleanup and error paths |
| pkg/io/dir_test.go | Improved error handling in cleanup functions |
| pkg/io/copy_test.go | Improved error handling in cleanup and added nolint directives |
| pkg/encoding/encoding_test.go | Extracted error message constant |
| pkg/converters/convert_test.go | Extracted error message and test value constants |
| pkg/auth/paseto_test.go | Extracted constants and added nolint directives |
| pkg/auth/jwt_test.go | Extracted constants for issuer and error messages |
| internal/io/write_fuzz_test.go | Improved error logging in fuzz test |
| internal/io/io_test.go | Changed file permissions to 0o600 and improved error handling |
| cspell.json | Added "unparam" to dictionary |
| .golangci.yaml | Updated allowed strings and integers for add-constant rule |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| t.Cleanup(func() { | ||
| //nolint:errcheck | ||
| _ = os.Remove(file.Name()) | ||
| }) |
There was a problem hiding this comment.
Potential issue with cleanup logic. The file is created in t.TempDir() on line 404, which means the test framework will automatically clean up the entire directory and its contents. The manual cleanup with os.Remove might fail because the file is already deleted as part of the temp directory cleanup. Consider removing this manual cleanup since it's redundant with t.TempDir()'s automatic cleanup, or handle the case where the file might already be deleted.
| detector, err := NewSQLInjectionDetector() | ||
| if err != nil { | ||
| t.Fatalf("expected detector, got %v", err) | ||
| t.Fatalf(errMsgDetector, err) |
There was a problem hiding this comment.
The constant errMsgDetector is used on line 100 but is not defined in this file. This will cause a compilation error. You need to add a constant definition at the top of the file, similar to what was done in pkg/sanitize/nosql_detect_test.go.
| symlinkPath := filepath.Join(tempDir, "symlink.txt") | ||
|
|
||
| err := os.WriteFile(validPath, []byte("test"), 0o644) | ||
| err := os.WriteFile(validPath, []byte("test"), 0o600) |
There was a problem hiding this comment.
The file permission has been changed from 0o644 to 0o600. This makes the file more restrictive (removing read permissions for group and others), which is generally a security improvement. However, please verify this change is intentional and doesn't break any test assertions or requirements.
| func TestWithSecretMaxLength(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| //nolint:revive |
There was a problem hiding this comment.
The nolint directive appears unnecessary here since the variable maxLen is actually used in the test function on lines 438 and 457. If this is meant to suppress a specific linter rule about test setup, please consider adding a more specific directive like //nolint:revive // Test setup variable with an explanation.
| //nolint:revive |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
No description provided.