Skip to content

fix: fix mock TickerFunc to act like real #7

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion mock.go
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,8 @@ type mockTickerFunc struct {

// cond is a condition Locked on the main Mock.mu
cond *sync.Cond
// inProgress is true when we are actively calling f
inProgress bool
// done is true when the ticker exits
done bool
// err holds the error when the ticker exits
Expand All @@ -479,15 +481,18 @@ func (m *mockTickerFunc) next() time.Time {
func (m *mockTickerFunc) fire(_ time.Time) {
m.mock.mu.Lock()
defer m.mock.mu.Unlock()
if m.done {
if m.done || m.inProgress {
return
}
m.nxt = m.nxt.Add(m.d)
m.mock.recomputeNextLocked()

m.inProgress = true
m.mock.mu.Unlock()
err := m.f()
m.mock.mu.Lock()
m.inProgress = false
m.cond.Broadcast() // wake up anything waiting for f to finish
if err != nil {
m.exitLocked(err)
}
Expand All @@ -507,6 +512,9 @@ func (m *mockTickerFunc) waitForCtx() {
<-m.ctx.Done()
m.mock.mu.Lock()
defer m.mock.mu.Unlock()
for m.inProgress {
m.cond.Wait()
}
Comment on lines +515 to +517
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: this is the canonical usage from https://pkg.go.dev/sync#Cond.Wait

m.exitLocked(m.ctx.Err())
}

Expand Down
105 changes: 105 additions & 0 deletions mock_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package quartz_test

import (
"context"
"errors"
"testing"
"time"

Expand Down Expand Up @@ -210,3 +211,107 @@ func TestPeek(t *testing.T) {
t.Fatal("expected Peek() to return false")
}
}

// TestTickerFunc_ContextDoneDuringTick tests that TickerFunc.Wait() can't return while the tick
// function callback is in progress.
func TestTickerFunc_ContextDoneDuringTick(t *testing.T) {
t.Parallel()
testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second)
defer testCancel()
mClock := quartz.NewMock(t)

tickStart := make(chan struct{})
tickDone := make(chan struct{})
ctx, cancel := context.WithCancel(testCtx)
defer cancel()
tkr := mClock.TickerFunc(ctx, time.Second, func() error {
close(tickStart)
select {
case <-tickDone:
case <-testCtx.Done():
t.Error("timeout waiting for tickDone")
}
return nil
})
w := mClock.Advance(time.Second)
select {
case <-tickStart:
// OK
case <-testCtx.Done():
t.Fatal("timeout waiting for tickStart")
}
waitErr := make(chan error, 1)
go func() {
waitErr <- tkr.Wait()
}()
cancel()
select {
case <-waitErr:
t.Fatal("wait should not return while tick callback in progress")
case <-time.After(time.Millisecond * 100):
// OK
}
close(tickDone)
select {
case err := <-waitErr:
if !errors.Is(err, context.Canceled) {
t.Fatal("expected context.Canceled error")
}
case <-testCtx.Done():
t.Fatal("timed out waiting for wait to finish")
}
w.MustWait(testCtx)
}

// TestTickerFunc_LongCallback tests that we don't call the ticker func a second time while the
// first is still executing.
func TestTickerFunc_LongCallback(t *testing.T) {
t.Parallel()
testCtx, testCancel := context.WithTimeout(context.Background(), 10*time.Second)
defer testCancel()
mClock := quartz.NewMock(t)

expectedErr := errors.New("callback error")
tickStart := make(chan struct{})
tickDone := make(chan struct{})
ctx, cancel := context.WithCancel(testCtx)
defer cancel()
tkr := mClock.TickerFunc(ctx, time.Second, func() error {
close(tickStart)
select {
case <-tickDone:
case <-testCtx.Done():
t.Error("timeout waiting for tickDone")
}
return expectedErr
})
w := mClock.Advance(time.Second)
select {
case <-tickStart:
// OK
case <-testCtx.Done():
t.Fatal("timeout waiting for tickStart")
}
// second tick completes immediately, since it doesn't actually call the
// ticker function.
mClock.Advance(time.Second).MustWait(testCtx)

waitErr := make(chan error, 1)
go func() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Do we need a waitgroup to ensure this goroutine is completely finished by the time the text exits? Ditto above on L244.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We read from waitErr on L307, so a waitgroup is redundant. On success, we know this goroutine has terminated.

Generally speaking, we mainly care about whether the code under test leaks goroutines. Leaking a test goroutine almost never matters, since the test is finite (the exception is if the leaked goroutine interferes with other tests, which is rare).

waitErr <- tkr.Wait()
}()
cancel()
close(tickDone)

select {
case err := <-waitErr:
// we should get the function error, not the context error, since context was canceled while
// we were calling the function, and it returned an error.
if !errors.Is(err, expectedErr) {
t.Fatalf("wrong error: %s", err)
}
case <-testCtx.Done():
t.Fatal("timed out waiting for wait to finish")
}
w.MustWait(testCtx)
}