test: standardize errors with ewrap; add constants; parallelize timers#18
test: standardize errors with ewrap; add constants; parallelize timers#18
Conversation
- tests/retrier_test.go, tests/registry_test.go:
- Replace errors.New with ewrap.New and introduce shared errTemporary.
- Add test-only constants (e.g., errFailedToCreateRetrier) and update
assertions/initializers for consistent temporary/non-temporary errors.
- tests/timer_test.go: mark timer pool tests with t.Parallel for concurrency.
- ci(golangci-lint): enable the testpackage linter in .golangci.yaml.
- docs(README): update example to use ewrap.New("temporary error") and tidy
formatting.
Why: unify test error semantics, improve maintainability and test performance,
and catch test package issues earlier via linting. No production code changes.
There was a problem hiding this comment.
Pull request overview
This PR standardizes error handling in test files by replacing errors.New with ewrap.New, introduces shared test constants for better maintainability, and parallelizes timer tests for improved test performance. It also attempts to enable the testpackage linter in golangci-lint configuration and updates the README example.
Changes:
- Replaced
errors.Newwithewrap.Newfor consistent error wrapping in tests - Added test constants (
errTemporary,errRetryCountMismatch,errNonTemporary,errFailedToCreateRetrier) to reduce duplication - Added
t.Parallel()to all test functions in timer_test.go, retrier_test.go, and registry_test.go for concurrent execution - Updated README.md example to use
ewrap.New - Modified .golangci.yaml linter configuration
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/timer_test.go | Added t.Parallel() to all three timer pool tests for concurrent execution |
| tests/retrier_test.go | Standardized errors with ewrap.New, introduced constants, renamed variables (r → retrier), improved error handling, and added t.Parallel() to all tests |
| tests/registry_test.go | Replaced errors.New with ewrap.New, added errFailedToCreateRetrier constant, renamed variable (r → registry), and added t.Parallel() to tests |
| .golangci.yaml | Added testpackage to the linter disable list |
| README.md | Updated example code to use ewrap.New("temporary error") and added trailing newline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| errRetryCountMismatch = "retry did not retry the function the expected number of times. Got: %d, Expecting: %d" | ||
| errNonTemporary = "non-temporary error" |
There was a problem hiding this comment.
The constant errFailedToCreateRetrier is used throughout this file (lines 197, 217, 228, 251, 269, 310, 342, 408, 437, 469, 506, 572, 611, 642) but is not defined here. It's only defined in registry_test.go. This will cause compilation errors. Add the constant definition to this file's const block.
| errRetryCountMismatch = "retry did not retry the function the expected number of times. Got: %d, Expecting: %d" | |
| errNonTemporary = "non-temporary error" | |
| errRetryCountMismatch = "retry did not retry the function the expected number of times. Got: %d, Expecting: %d" | |
| errNonTemporary = "non-temporary error" | |
| errFailedToCreateRetrier = "failed to create new Retrier: %v" |
|
|
||
| // Register a temporary error. | ||
| tempErr := errors.New("temporary error") | ||
| tempErr := ewrap.New("temporary error") |
There was a problem hiding this comment.
The code example uses ewrap.New("temporary error") but the import section doesn't include the ewrap package. Either add the ewrap import (e.g., "github.com/hyp3rd/ewrap") or replace ewrap.New with errors.New to match the existing imports.
| tempErr := ewrap.New("temporary error") | |
| tempErr := errors.New("temporary error") |
| - recvcheck | ||
| - tagliatelle | ||
| - tagalign | ||
| - testpackage |
There was a problem hiding this comment.
The PR description states "enable the testpackage linter" but this change adds testpackage to the disable list, which means it will remain disabled. To enable the linter, it should be removed from the disable list or added to the enable list (if the default is not 'all').
| - testpackage |
- Initialize options earlier in constructor so defaults/overrides are applied before use. - Ensure retry error aggregation initializes Attempts before append; lazily init errors pool. - Guard debug logging with Logger.Enabled(ctx, slog.LevelDebug) to avoid unnecessary work. - Make handleAttemptError robust to nil Attempts and safely append entries. - tests: refresh benchmarks in tests/retrier_test.go to reflect current retry behavior and reset state per iteration. - docs(README): update example paths and benchmark outputs. - ci(golangci-lint): raise cyclop max-complexity to 15. - examples: move examples/* to __examples/* to keep them out of package discovery/build. Why: prevent nil dereferences in error tracking, reduce logging overhead when debug is off, and align docs/benchmarks with current behavior while keeping examples from polluting the module graph. BREAKING CHANGE: none
1 participant
Why: unify test error semantics, improve maintainability and test performance,
and catch test package issues earlier via linting. No production code changes.