Conversation
- Update go.mod to use hyperlogger v0.1.1 (from v0.1.0) - Refresh go.sum checksums accordingly
- Enable tests in golangci-lint (.golangci.yaml) and expand allowed integer literals (e.g., 0o004). - Replace magic numbers in tests with named constants (e.g., base64MaxLength, writeMaxSize, readMaxSize, backupCodeCount, longInputLength, keyLength). - Update option usage to rely on these constants (WithBase64MaxLength/WithHexMaxLength/WithJSONMaxBytes, WithWriteMaxSize/WithReadMaxSize, WithBackupCodeCount). - Prefer t.Fatal over t.Fatalf where formatting is unnecessary; add targeted nolint comments for intentional values. - Clarify boundary/invalid cases in HOTP/TOTP tests; minor cleanups across encoding, io, memory, mfa, password, sanitize, secrets, tokens, and validate packages. - No production code changes; tests and lint config only.
There was a problem hiding this comment.
Pull request overview
This PR addresses linting issues in test files by enabling linting for tests and fixing various violations. The changes focus on improving test code quality through constant extraction, appropriate use of t.Fatal vs t.Fatalf, and adding nolint directives where necessary.
Changes:
- Enabled linting for test files in
.golangci.yamland added0o004to allowed integers - Extracted magic numbers and repeated values into named constants across test files
- Corrected usage of
t.Fatal()vst.Fatalf()based on whether format arguments are present - Added
//nolint:revivedirectives to suppress specific linting warnings for unavoidable violations - Updated
hyperloggerdependency from v0.1.0 to v0.1.1
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| .golangci.yaml | Enabled linting for test files and added 0o004 to allowed integers |
| go.mod | Updated hyperlogger dependency to v0.1.1 |
| go.sum | Updated checksums for hyperlogger v0.1.1 |
| pkg/validate/url_test.go | Added nolint directive for validator variable |
| pkg/tokens/tokens_test.go | Added nolint directives for validator and token variables |
| pkg/secrets/secrets_test.go | Extracted longInputLength constant, fixed t.Fatal usage |
| pkg/sanitize/filename_test.go | Added nolint directive for sanitizer variable |
| pkg/password/argon2id_test.go | Extracted keyLength constant |
| pkg/mfa/totp_test.go | Extracted invalidValue constant, fixed t.Fatal usage |
| pkg/mfa/hotp_test.go | Extracted counter and resync constants, removed duplicate variable declarations, fixed t.Fatal usage |
| pkg/mfa/backup_test.go | Extracted backupCodeCount constant, improved error message formatting |
| pkg/memory/reader_test.go | Added nolint directive |
| pkg/io/file_test.go | References writeMaxSize constant from copy_test.go |
| pkg/io/copy_test.go | Defined writeMaxSize constant |
| pkg/encoding/encoding_test.go | Extracted base64MaxLength constant (used for multiple purposes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| filename := filepath.Base(uniqueTempPath(t, "sectools-reader-max-")) | ||
|
|
||
| client, err := NewWithOptions(WithWriteMaxSize(3)) | ||
| client, err := NewWithOptions(WithWriteMaxSize(writeMaxSize)) |
There was a problem hiding this comment.
The constant writeMaxSize is used here but is not defined in this file. It appears to be defined in pkg/io/copy_test.go. Since test files in the same package can share constants, this will work, but it creates a confusing dependency between test files. Consider defining this constant in this file as well, or moving shared test constants to a common test helper file.
| client, err := NewWithOptions(WithWriteMaxSize(writeMaxSize)) | |
| client, err := NewWithOptions(WithWriteMaxSize(readMaxSize)) |
| opts: []HOTPOption{WithHOTPDigits(Digits(invalidValue))}, | ||
| }, | ||
| { | ||
| name: "invalid-algorithm", | ||
| opts: []HOTPOption{WithHOTPAlgorithm(Algorithm(99))}, | ||
| opts: []HOTPOption{WithHOTPAlgorithm(Algorithm(invalidValue))}, |
There was a problem hiding this comment.
The constant invalidValue is used here but is not defined in this file. It appears to be defined in pkg/mfa/totp_test.go. Since test files in the same package can share constants, this will work, but it creates a confusing dependency between test files. Consider defining this constant in this file as well, or moving shared test constants to a common location in the package.
pkg/encoding/encoding_test.go
Outdated
|
|
||
| const errMsgUnexpected = "expected decoded, got %v" | ||
|
|
||
| const base64MaxLength = 4 |
There was a problem hiding this comment.
The constant name base64MaxLength is misleading because it's also used for hex encoding and JSON decoding limits. Consider renaming to a more generic name like maxLengthForTest or smallMaxLength, or create separate constants with appropriate names for each use case.
Update .pre-commit-config.yaml to use yamllint v1.38.0 (from v1.37.1). Keeps lint tooling current; may surface new lint warnings, no runtime changes.
1 participant
No description provided.