Increase test coverage to 83.8% and fix SetupHystrixPrometheus panic#43
Increase test coverage to 83.8% and fix SetupHystrixPrometheus panic#43
Conversation
…panic - Add comprehensive test suite (core_coverage_test.go) covering server lifecycle, HTTP handler routing, initializers, VTProto codec, and utility functions - Fix SetupHystrixPrometheus to use sync.Once, preventing panic on duplicate Prometheus collector registration - Regenerate READMEs via gomarkdoc
📝 WalkthroughWalkthroughDocumentation updates for API reference line ranges and health endpoint changes; introduction of a comprehensive 912-line test suite for the core package; and modification of SetupHystrixPrometheus to use sync.Once for thread-safe Prometheus collector registration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR improves reliability and test confidence in the core package by making Hystrix Prometheus setup idempotent and adding a broad, coverage-oriented test suite, along with regenerated gomarkdoc READMEs.
Changes:
- Prevent
SetupHystrixPrometheusfrom panicking on duplicate registration by guarding it withsync.Once. - Add
core_coverage_test.gocovering server lifecycle, HTTP routing, initializers, VTProto codec, and utility helpers to substantially increase coverage. - Regenerate README documentation via gomarkdoc (including updated API reference line links and descriptions).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.md | Regenerated API docs and updated SetupHystrixPrometheus description to mention sync.Once. |
| initializers.go | Wrap Hystrix Prometheus collector setup in sync.Once to avoid duplicate-registration panics. |
| core_coverage_test.go | New comprehensive test suite exercising many previously uncovered branches and behaviors. |
| config/README.md | Updates quick-start docs line describing health check endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…theus regression test
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
41-41:⚠️ Potential issue | 🟠 MajorDon't advertise probe endpoints that core doesn't register.
The core HTTP handler only wires swagger, pprof, and metrics;
/healthcheckand/readycheckare not added by core. Publishing these paths in the generated README(s) will send users toward failing liveness/readiness probes, so please either add the routes or update the gomarkdoc source to match the actual surface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 41, The README advertises /healthcheck and /readycheck but the core HTTP handler only registers swagger, pprof and metrics; either register the probe endpoints or remove them from the generated docs. Fix by updating the core HTTP handler to add health and ready routes (the function that wires HTTP routes where swagger, pprof and metrics are registered) or modify the gomarkdoc generation configuration / README template to stop listing /healthcheck and /readycheck so docs match the actual surface.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core_coverage_test.go`:
- Around line 764-772: The fixed time.Sleep should be replaced with explicit
synchronization so Stop() doesn't race reading c.grpcServer/c.httpServer; update
the service lifecycle to signal when Run() has finished assigning servers (for
example add/close a channel like svc.started or use a sync.WaitGroup) and have
the test wait on that channel instead of sleeping after waiting on svc.ready;
modify the Run() implementation to close svc.started (or call wg.Done()) after
it sets c.grpcServer and c.httpServer, and change the test to wait for that
signal before calling Stop().
- Around line 362-367: The test TestSetupHystrixPrometheus_CalledTwice is
ineffective because prior tests call processConfig(), which already invokes
SetupHystrixPrometheus() and causes hystrixOnce to be set; update the test to
either reset the hystrixOnce/global registration state before calling
SetupHystrixPrometheus twice or execute the two-call assertion in a fresh
subprocess; specifically locate the SetupHystrixPrometheus and hystrixOnce
symbols and either reinitialize hystrixOnce (or the related registration
globals) at the start of TestSetupHystrixPrometheus_CalledTwice or change the
test to spawn a new process that calls SetupHystrixPrometheus twice to ensure
the registration path is exercised.
---
Outside diff comments:
In `@README.md`:
- Line 41: The README advertises /healthcheck and /readycheck but the core HTTP
handler only registers swagger, pprof and metrics; either register the probe
endpoints or remove them from the generated docs. Fix by updating the core HTTP
handler to add health and ready routes (the function that wires HTTP routes
where swagger, pprof and metrics are registered) or modify the gomarkdoc
generation configuration / README template to stop listing /healthcheck and
/readycheck so docs match the actual surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f71fa99e-b0e5-42d9-8f97-8c79fde50d7e
📒 Files selected for processing (4)
README.mdconfig/README.mdcore_coverage_test.goinitializers.go
Summary
core_coverage_test.go) covering server lifecycle, HTTP handler routing, initializers, VTProto codec, and utility functionsSetupHystrixPrometheuswithsync.Onceto prevent panic on duplicate Prometheus collector registrationTest plan
go test -race -cover ./...passes with 83.8% coverageSetupHystrixPrometheusno longer panics when called multiple timesSummary by CodeRabbit
Documentation
Bug Fixes
Tests