Use mock clock, unflake tests #813
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Fixing data race in benbjohnson/clock#42. |
Codecov Report
@@ Coverage Diff @@
## master #813 +/- ##
==========================================
- Coverage 99.25% 99.04% -0.21%
==========================================
Files 24 25 +1
Lines 941 946 +5
==========================================
+ Hits 934 937 +3
- Misses 6 7 +1
- Partials 1 2 +1
Continue to review full report at Codecov.
|
abhinav
commented
Nov 18, 2021
go.mod
Outdated
| github.com/stretchr/testify v1.7.0 | ||
| go.uber.org/dig v1.12.0 | ||
| go.uber.org/goleak v1.1.11 | ||
| go.uber.org/multierr v1.5.0 | ||
| go.uber.org/zap v1.16.0 | ||
| golang.org/x/sys v0.0.0-20210903071746-97244b99971b | ||
| ) | ||
|
|
||
| replace github.com/benbjohnson/clock => github.com/abhinav/clock v1.3.1-0.20211118204451-38de6f8a55b9 |
There was a problem hiding this comment.
We probably will not land this replace directive.
I'd like to wait a few days for the change to be merged upstream.
sywhang
approved these changes
Nov 18, 2021
r-hang
reviewed
Nov 18, 2021
abhinav
force-pushed
the
abg/onstop-race
branch
from
b49c226
to
5450aac
Compare
r-hang
approved these changes
Nov 18, 2021
abhinav
force-pushed
the
abg/onstop-race
branch
from
5450aac
to
cf899f7
Compare
|
This PR is now WIP while I'm digging further into this. |
abhinav
changed the title
abhinav
force-pushed
the
abg/onstop-race
branch
from
bdca9bb
to
62d2b29
Compare
Add an internal fxclock package that provides a minimal Clock interface. We will use this to abstract away Fx's use of time operations to help address test flakiness issues.
abhinav
force-pushed
the
abg/onstop-race
branch
2 times, most recently
from
e54a0b3
to
00653cb
Compare
Add a WithClock option to fx.App to inject an fxclock and use this clock for all time operations in App and Lifecycle. This option is only available from tests inside the same directory. We're intentionally not exposing this as part of our public API for now.
Use a mock clock for the tests in TestAppStart and TestAppStop that rely on blocking or sleeping. These tests were previously using a small timeout and blocking on `ctx.Done` indefinitely, or using `time.Sleep` directly. With a mock clock, we can use larger, more real timeouts and simply advance time to an even larger value. Besides making the tests more deterministic, this also makes them faster. Before: ``` $ go test -run TestAppStart -count 1 PASS ok go.uber.org/fx 1.066s $ go test -run TestAppStart -count 1 PASS ok go.uber.org/fx 1.060s ``` After: ``` $ go test -run TestAppStart -count 1 PASS ok go.uber.org/fx 0.083s $ go test -run TestAppStart -count 1 PASS ok go.uber.org/fx 0.064s ``` (TestAppStop was not affected similarly because it used a very small timeout to begin with.)
abhinav
force-pushed
the
abg/onstop-race
branch
2 times, most recently
from
29082d1
to
85a3278
Compare
The OnStop_timeout test was flaky because of the very small StartTimeout. We would sometimes hit a condition where: App.Run calls App.Start with a timeout of 1 millisecond. This takes just long enough that App.Start gives up. As a result, App.Run does not call App.Stop, which causes the test to fail because we don't see the Stop event. Resolve the flakiness with the use of a mock clock. Now, time doesn't advance while the start hook is running, so we can be certain that the really long OnStop hook is invoked and has a chance to time out. Note that the subtests for this test share hooks, but they can't share mock clocks (`clock.Add` is not thread-safe) so we instead use "hook builders" that build hooks given the mock clock.
abhinav
force-pushed
the
abg/onstop-race
branch
from
85a3278
to
cf09130
Compare
abhinav
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR resolves flakiness of some tests in Fx
by abstracting our use of time operations behind a Clock interface
and using a mock clock for the relevant tests.
This adds an internal/fxclock package that defines the Clock interface.
This interface is a compliant subset of "benbjohnson/clock".Clock.
This intentionally does not use benbjohnson/clock directly outside of tests
to keep it a test-only dependency.
The new internal-only
WithClockoptionaccepts and plumbs an implementation of
fxclock.Clock.Note that this option is declared in app_internal_test.go.
It's not part of the public API,
but it's accessible to external tests in the top-level Fx directory.
Finally, this switches to using the Clock for time operations where relevant,
and updates tests that rely on sleep/time-based blocking to use a mock clock.
Now,
time.Nowis only present in fxclock.System.time.Sinceis only present in fxclock.System.time.Sleepis only present in fxclock.System.context.WithTimeoutis present in fxclock.System,fxtest.App.Require{Start,Stop}, andexample_test.go.The latter two because fxclock.Clock is not part of our public API.
Resolves #799
Ref: GO-1001