feat(chproxy): add unit tests for config.go#2975
Conversation
- Add comprehensive test coverage for LoadConfig() - Update service version from 1.3.0 to 1.3.1 - Add config.Logger initialization in LoadConfig - Use structured logging with slog instead of log.Fatalf - Add .gitignore for chproxy binary and coverage files Signed-off-by: Ian Meyer (imeyer) <k@imeyer.io>
imeyer
requested review from
MichaelUnkey,
chronark,
mcstepp,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Main
participant ConfigLoader as LoadConfig
participant Logger
participant Telemetry as SetupTelemetry
Main->>ConfigLoader: Load configuration
ConfigLoader-->>Main: Returns config (includes ServiceVersion and Logger)
Main->>Main: Validate configuration result
alt Error Occurs
Main->>Logger: Log error with structured logging
Main-->>Main: Invoke os.Exit(1)
else Successful Load
Main->>Telemetry: Call setupTelemetry(config)
Telemetry-->>Main: Log trace sample rate (using slog.Float64)
Main->>Main: Continue with application initialization
end
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/chproxy/config_test.go (1)
77-79: Consider extending logger verificationWhile checking that the logger is non-nil is a good start, consider extending the test to verify specific logger properties (output destination, log level, etc.) to ensure it's configured correctly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/chproxy/.gitignore(1 hunks)apps/chproxy/config.go(1 hunks)apps/chproxy/config_test.go(1 hunks)apps/chproxy/main.go(2 hunks)apps/chproxy/otel.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Test Packages / Test ./packages/rbac
- GitHub Check: Test Packages / Test ./packages/cache
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./packages/api
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/hash
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: autofix
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (10)
apps/chproxy/.gitignore (1)
1-2: Good practice for ignoring binary and coverage files.Excluding the
chproxybinary andcoverage.outfile from version control is good practice. This prevents binary artifacts and test results from cluttering the repository.apps/chproxy/main.go (2)
36-37: Improved error handling with structured logging.The switch from
log.Fatalfto structured logging withconfig.Logger.Errorfollowed by explicitos.Exit(1)is a good improvement. This provides better error details in a structured format that's easier to parse in logging systems.
55-56: Consistent structured error logging for telemetry failures.Good application of the same structured logging pattern for telemetry setup errors, maintaining consistency throughout the codebase.
apps/chproxy/otel.go (1)
144-144: Enhanced type information in structured logging.Using
slog.Float64instead of a simple key-value pair adds explicit type information that improves log parsing and consistency.apps/chproxy/config.go (2)
32-32: Version bump from 1.3.0 to 1.3.1.Appropriate version increment that aligns with the PR objectives.
37-39: Good initialization of structured logger.The initialization of the logger in the Config struct provides a foundation for the structured logging improvements seen throughout the codebase. Using a text handler with stdout is a sensible default configuration.
apps/chproxy/config_test.go (4)
1-37: Well-structured test setup and teardownThe helper functions for environment manipulation are well-designed with proper use of
t.Helper()and comprehensive cleanup throught.Cleanup(). This ensures tests remain isolated and don't interfere with each other, which is essential for reliable testing.
39-80: Good use of table-driven tests for configuration verificationThe table-driven approach makes the tests maintainable and easy to extend when new configuration parameters are added. The verification of default values ensures that any future changes to defaults are explicitly recognized in the tests.
152-244: Comprehensive edge case testingThe test cases thoroughly validate boundary conditions for configuration parameters, including extreme values (0.0 and 1.0) for sample rate and custom settings for all configurable parameters. This comprehensive approach ensures the configuration behaves correctly across its entire range of valid inputs.
1-244: Excellent test coverage addressing the PR objectivesThe test file provides comprehensive coverage for the
LoadConfig()function, which directly addresses the PR objective of enhancing test coverage. The tests verify the updated service version (1.3.1), proper initialization of the logger, and correct handling of environment variables, all of which align with the PR goals.
| if got := err.Error(); got != tc.wantErr && got[:len(tc.wantErr)] != tc.wantErr { | ||
| t.Errorf("LoadConfig() error = %v, want to contain %v", got, tc.wantErr) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential index out of range error in error comparison
The current error check could panic if the actual error message is shorter than the expected error message.
Replace with a safer approach using strings.Contains:
- if got := err.Error(); got != tc.wantErr && got[:len(tc.wantErr)] != tc.wantErr {
- t.Errorf("LoadConfig() error = %v, want to contain %v", got, tc.wantErr)
- }
+ import "strings" // Add this to imports
+ if got := err.Error(); got != tc.wantErr && !strings.Contains(got, tc.wantErr) {
+ t.Errorf("LoadConfig() error = %v, want to contain %v", got, tc.wantErr)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if got := err.Error(); got != tc.wantErr && got[:len(tc.wantErr)] != tc.wantErr { | |
| t.Errorf("LoadConfig() error = %v, want to contain %v", got, tc.wantErr) | |
| // Add the following to your imports section if it's not already present: | |
| import "strings" | |
| // ... (other code) | |
| if got := err.Error(); got != tc.wantErr && !strings.Contains(got, tc.wantErr) { | |
| t.Errorf("LoadConfig() error = %v, want to contain %v", got, tc.wantErr) | |
| } |
2 participants
What does this PR do?
Includes tests because I messed up and introduced a nil pointer deref in f3dfdb3☹️
Summary by CodeRabbit