-
Notifications
You must be signed in to change notification settings - Fork 290
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
test: add a custom mock clock implementation #1154
Conversation
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
var _ Clock = clock.Clock(nil) | ||
|
||
// Just a basic sanity check that everything is in order. |
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.
Instead of just the system clock sanity test, the test is now used for both, system clock and mock clock to verify basic outer expectations of the clocks.
Looks like you don't intend to put the clock in an externally addressable
location? :l
I should probably copy paste this into uber/ratelimit too. Not only is it
on bobjohnson but also on an ancient version of it - upgrading breaks the
tests :l
…On Fri, Feb 9, 2024, 03:18 Abhinav Gupta ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In internal/fxclock/clock_test.go
<#1154 (comment)>:
> "github.com/stretchr/testify/assert"
)
-var _ Clock = clock.Clock(nil)
-
-// Just a basic sanity check that everything is in order.
Instead of just the system clock sanity test, the test is now used for
both, system clock and mock clock to verify basic outer expectations of the
clocks.
—
Reply to this email directly, view it on GitHub
<#1154 (review)>,
or unsubscribe
<https:/notifications/unsubscribe-auth/AACG3UUGE5SGHUG5YCNLUUDYSW5UDAVCNFSM6AAAAABDA6E5YWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNZRG4ZDGNJVGY>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
We do! @prashantv has a nicer version of the clock in progress for re-use between projects. |
benbjohnson/clock has been archived and no longer maintained. Removing that dependency is desirable. This change adds a mock clock adapted from Zap's mock clock (uber-go/zap#1349), but with operations specific to Fx's needs. There are two areas that require more scrutiny because of divergence from Zap's mock clock: WithTimeout requires us to implement a custom `context.Context` so that it can report context.DeadlineExceeded when it's time. We cannot just use `context.WithCancelCause` here because the cause for a context failure is considered different from `ctx.Err`, so `ctx.Err` would still report `context.Canceled` for a timeout. When testing that a sleep behaves as expected, there's a data race between the `Sleep` and the `Add` that progresses time. To resolve this, this change adds an `AwaitScheduled` method that blocks until there are operations scheduled for the future. This is done by using a `sync.Cond`. This gives us a way to wait until the sleep is scheduled before we progress time. This change also had to update Zap to pick up the release with the custom clock to drop the benbjohnson/clock dependency from the go.mod completely. Refs uber-go/zap#1349 Resolves uber-go#1135
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1154 +/- ##
==========================================
+ Coverage 98.74% 98.78% +0.04%
==========================================
Files 30 30
Lines 2950 3057 +107
==========================================
+ Hits 2913 3020 +107
Misses 30 30
Partials 7 7 ☔ View full report in Codecov by Sentry. |
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.
LGTM I think, just a couple questions/comments on your tests, and probably should wait for another maintainer to take a look too before merging.
Co-authored-by: Jacob Oaks <[email protected]>
Co-authored-by: Jacob Oaks <[email protected]>
The same check we have elsewhere with AwaitScheduled is needed for the end-to-end Clock test.
quick first pass, will come back to this later tonight |
benbjohnson/clock has been archived and no longer maintained.
Removing that dependency is desirable.
This change adds a mock clock adapted from Zap's mock clock
(uber-go/zap#1349), but with operations specific to Fx's needs.
There are two areas that require more scrutiny
because of divergence from Zap's mock clock:
WithTimeout requires us to implement a custom
context.Context
so that it can report context.DeadlineExceeded when it's time.
We cannot just use
context.WithCancelCause
here because the causefor a context failure is considered different from
ctx.Err
,so
ctx.Err
would still reportcontext.Canceled
for a timeout.When testing that a sleep behaves as expected, there's a data race
between the
Sleep
and theAdd
that progresses time.To resolve this, this change adds an
AwaitScheduled
methodthat blocks until there are operations scheduled for the future.
This is done by using a
sync.Cond
.This gives us a way to wait until the sleep is scheduled
before we progress time.
This change also had to update Zap to pick up the release with the
custom clock to drop the benbjohnson/clock dependency from the go.mod
completely.
Refs uber-go/zap#1349
Resolves #1135