-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add a WithSynchronousAfterFunc option #68
Conversation
clockwork.go
Outdated
// WithSynchronousAfterFunc is a FakeClockOption which ensures that the callback to AfterFunc is called synchronously | ||
// instead of in a separate goroutine. If true, this option will cause any Advance calls to block until the callback | ||
// from all fired timers has completed. | ||
func WithSynchronousAfterFunc(b bool) FakeClockOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a first-pass, I don't think the argument is needed. The name of the option communicates the intent: if present, the AfterFunc is sync, if absent, AfterFunc is async (the default).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, thanks
clockwork_test.go
Outdated
func TestFakeClockSynchronousAfterFunc(t *testing.T) { | ||
t.Parallel() | ||
fc := NewFakeClock(WithSynchronousAfterFunc(true)) | ||
var called atomic.Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's avoid atomic unless as a last resort. Here you can pretty readily use a channel:
var called atomic.Bool
- > called := make(chan bool)
called.Store(true)
-> close(called)
if called.Load() { t.Falal(...) }
->
select {
case <-called:
t.Falal(...)
default:
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually realized that since this is called synchronously, there's no need for further synchronization, so I just used a bool. I ran the same test with the -race
flag on, and I verified that the test passed; and then I also verified that the race detector test fails without the WithSynchronousAfterFunc
option.
Take a look at #69 . I think we can find a way meet your needs without adding dimensionality to the behavior of |
Hmm, how would you do something like the |
This is for #67
I'm happy to add more context if you'd like, but the main idea is that this makes it a lot easier to test certain things. For example, if you want to verify that a timer did fire without this option, you could just block for a while and wait for it to return. However, if you wanted to verify that timer did not fire, you'd have to set an arbitrary wait amount.