unit test ports.SchedulerService
#849
Conversation
WalkthroughAdds an Option-based configurable ticker interval to the block scheduler, changes NewScheduler to accept options, refactors Start to use a time.Ticker driven by the configured interval for periodic task fetching and concurrent execution, and adds tests that exercise schedulers against a mock block-tip HTTP endpoint. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-08-28T08:21:01.170ZApplied to files:
🧬 Code graph analysis (1)internal/infrastructure/scheduler/service_test.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (3)
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 |
![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)
internal/infrastructure/scheduler/block/service.go (1)
81-84: Potential panic ifStop()is called multiple times or beforeStart().Sending to
stopChfollowed byclose(stopCh)will panic ifStop()is called twice or if no goroutine is reading from the channel yet. Consider using async.Onceor a non-blocking send pattern.Example fix using
sync.Once:type service struct { tipURL string lock sync.Locker taskes map[int64][]func() stopCh chan struct{} tickerInterval time.Duration + stopOnce sync.Once } func (s *service) Stop() { - s.stopCh <- struct{}{} - close(s.stopCh) + s.stopOnce.Do(func() { + close(s.stopCh) + }) }This also simplifies Stop() by just closing the channel (the select will receive from a closed channel).
🧹 Nitpick comments (2)
internal/infrastructure/scheduler/service_test.go (1)
37-37: Consider reducing the sleep duration for faster tests.The test sleeps for 3 seconds. Combined with the block scheduler's 1-second ticker interval, this could be optimized. Consider using a polling loop with a shorter timeout or reducing the sleep to
2 * time.Secondto match the minimal expected execution window more closely while keeping tests fast.internal/infrastructure/scheduler/block/service.go (1)
27-27: Typo:taskesshould betasks.The field name
taskesappears to be a typo and should betasksfor clarity. This also applies to the local variables and method namepopTaskes.type service struct { tipURL string lock sync.Locker - taskes map[int64][]func() + tasks map[int64][]func() stopCh chan struct{} tickerInterval time.Duration }Similar renames would be needed for
popTaskes→popTasksand related local variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/infrastructure/scheduler/block/service.go(2 hunks)internal/infrastructure/scheduler/service_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/infrastructure/scheduler/block/service.go (1)
internal/core/ports/scheduler.go (1)
SchedulerService(10-18)
internal/infrastructure/scheduler/service_test.go (2)
internal/core/ports/scheduler.go (1)
SchedulerService(10-18)internal/infrastructure/scheduler/block/service.go (2)
NewScheduler(32-55)WithTickerInterval(18-22)
🔇 Additional comments (3)
internal/infrastructure/scheduler/block/service.go (3)
16-22: LGTM!The Option pattern is correctly implemented, providing a clean way to configure the ticker interval.
32-55: LGTM!The NewScheduler function properly initializes defaults and applies options. The default 10-second ticker interval is reasonable for block-based scheduling.
57-79: LGTM!The Start() method now correctly uses
time.Tickerfor periodic execution instead of a fixed sleep. The ticker is properly stopped viadefer ticker.Stop(), and the select handles both the stop signal and tick events appropriately.
| func servicesToTest(t *testing.T) []service { | ||
| // mock esplora server for block tip endpoint | ||
| var blockHeight int64 = 99 | ||
| mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.URL.Path == "/blocks/tip/height" { | ||
| w.WriteHeader(http.StatusOK) | ||
| height := atomic.AddInt64(&blockHeight, 1) | ||
| // nolint:errcheck | ||
| fmt.Fprintf(w, "%d", height) | ||
| } else { | ||
| w.WriteHeader(http.StatusNotFound) | ||
| } | ||
| })) | ||
| t.Cleanup(func() { | ||
| mockServer.Close() | ||
| }) | ||
|
|
||
| blockService, err := blockscheduler.NewScheduler( | ||
| mockServer.URL, | ||
| blockscheduler.WithTickerInterval(time.Second*1), | ||
| ) | ||
| if err != nil { | ||
| t.Fatalf("failed to create block scheduler: %v", err) | ||
| } | ||
|
|
||
| svcs := []service{ | ||
| {name: "gocron", scheduler: timescheduler.NewScheduler()}, | ||
| {name: "block", scheduler: blockService}, | ||
| } | ||
|
|
||
| for _, svc := range svcs { | ||
| svc.scheduler.Start() | ||
| t.Cleanup(func() { svc.scheduler.Stop() }) | ||
| } | ||
|
|
||
| return svcs |
There was a problem hiding this comment.
Cleanup closure captures loop variable—only the last scheduler will be stopped.
In the cleanup loop (lines 74-77), the closure captures svc by reference. By the time cleanup runs, svc refers to the last element in svcs, so only the last scheduler's Stop() will be called (twice), leaving the first scheduler running.
Apply this diff to fix the issue:
for _, svc := range svcs {
+ svc := svc // capture range variable
svc.scheduler.Start()
t.Cleanup(func() { svc.scheduler.Stop() })
}📝 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.
| func servicesToTest(t *testing.T) []service { | |
| // mock esplora server for block tip endpoint | |
| var blockHeight int64 = 99 | |
| mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| if r.URL.Path == "/blocks/tip/height" { | |
| w.WriteHeader(http.StatusOK) | |
| height := atomic.AddInt64(&blockHeight, 1) | |
| // nolint:errcheck | |
| fmt.Fprintf(w, "%d", height) | |
| } else { | |
| w.WriteHeader(http.StatusNotFound) | |
| } | |
| })) | |
| t.Cleanup(func() { | |
| mockServer.Close() | |
| }) | |
| blockService, err := blockscheduler.NewScheduler( | |
| mockServer.URL, | |
| blockscheduler.WithTickerInterval(time.Second*1), | |
| ) | |
| if err != nil { | |
| t.Fatalf("failed to create block scheduler: %v", err) | |
| } | |
| svcs := []service{ | |
| {name: "gocron", scheduler: timescheduler.NewScheduler()}, | |
| {name: "block", scheduler: blockService}, | |
| } | |
| for _, svc := range svcs { | |
| svc.scheduler.Start() | |
| t.Cleanup(func() { svc.scheduler.Stop() }) | |
| } | |
| return svcs | |
| func servicesToTest(t *testing.T) []service { | |
| // mock esplora server for block tip endpoint | |
| var blockHeight int64 = 99 | |
| mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| if r.URL.Path == "/blocks/tip/height" { | |
| w.WriteHeader(http.StatusOK) | |
| height := atomic.AddInt64(&blockHeight, 1) | |
| // nolint:errcheck | |
| fmt.Fprintf(w, "%d", height) | |
| } else { | |
| w.WriteHeader(http.StatusNotFound) | |
| } | |
| })) | |
| t.Cleanup(func() { | |
| mockServer.Close() | |
| }) | |
| blockService, err := blockscheduler.NewScheduler( | |
| mockServer.URL, | |
| blockscheduler.WithTickerInterval(time.Second*1), | |
| ) | |
| if err != nil { | |
| t.Fatalf("failed to create block scheduler: %v", err) | |
| } | |
| svcs := []service{ | |
| {name: "gocron", scheduler: timescheduler.NewScheduler()}, | |
| {name: "block", scheduler: blockService}, | |
| } | |
| for _, svc := range svcs { | |
| svc := svc // capture range variable | |
| svc.scheduler.Start() | |
| t.Cleanup(func() { svc.scheduler.Stop() }) | |
| } | |
| return svcs | |
| } |
🤖 Prompt for AI Agents
internal/infrastructure/scheduler/service_test.go around lines 44-79: the
t.Cleanup closure in the for loop captures the loop variable svc, so when
cleanup runs it only stops the last scheduler; fix by creating a new local
variable per iteration and capture that instead (e.g., inside the loop assign s
:= svc then call s.scheduler.Start() and t.Cleanup(func() { s.scheduler.Stop()
})), ensuring each scheduler gets its own cleanup closure.
2 participants
@altafan please review
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.