-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
proposal: testing/synctest: new package for testing concurrent code #67434
Comments
I really like how simple this API is.
How does time work when goroutines aren't idle? Does it stand still, or does it advance at the usual rate? If it stands still, it seems like that could break software that assumes time will advance during computation (that maybe that's rare in practice). If it advances at the usual rate, it seems like that reintroduces a source of flakiness. E.g., in your example, the 1 second sleep will advance time by 1 second, but then on a slow system the checking thread may still not execute for a long time. What are the bounds of the fake time implementation? Presumably if you're making direct system calls that interact with times or durations, we're not going to do anything about that. Are we going to make any attempt at faking time in the file system?
What if a goroutine is blocked on a channel that goes outside the group? This came to mind in the context of whether this could be used to coordinate a multi-process client/server test, though I think it would also come up if there's any sort of interaction with a background worker goroutine or pool.
What happens if multiple goroutines in a group call Wait? I think the options are to panic or to consider all of them idle, in which case they would all wake up when every other goroutine in the group is idle. What happens if you have nested groups, say group A contains group B, and a goroutine in B is blocked in Wait, and then a goroutine in A calls Wait? I think your options are to panic (though that feels wrong), wake up both if all of the goroutines in group A are idle, or wake up just B if all of the goroutines in B are idle (but this block waking up A until nothing is calling Wait in group B). |
Time stands still, except when all goroutines in a group are idle. (Same as the playground behaves, I believe.) This would break software that assumes time will advance. You'd need to use something else to test that case.
The time package: Faking time in the filesystem seems complicated and highly specialized, so I don't think we should try. Code which cares about file timestamps will need to use a test
As proposed, this would count as an idle goroutine. If you fail to isolate the system under test this will probably cause problems, so don't do that.
As proposed, none of them ever wake up and your test times out, or possibly panics if we can detect that all goroutines are blocked in that case. Having them all wake at the same time would also be reasonable.
Oh, I didn't think of that. Nested groups are too complicated, |
This is a very interesting proposal! I feel worried that the Assuming that's a valid concern (if it isn't then I'll retract this entire comment!), I could imagine mitigating it in two different ways:
(I apologize in advance if I misunderstood any part of the proposal or if I am missing something existing that's already similarly convenient to |
The fact that I think using idle-wait synchronization outside of tests is always going to be a mistake. It's fragile and fiddly, and you're better served by explicit synchronization. (This prompts the question: Isn't this fragile and fiddly inside tests as well? It is, but using a fake clock removes much of the sources of fragility, and tests often have requirements that make the fiddliness a more worthwhile tradeoff. In the expiring cache example, for example, non-test code will never need to guarantee that a cache entry expires precisely at the nanosecond defined.) So while perhaps we could offer a standalone synchroniziation primitive outside of As for passing a |
Interesting proposal. I like that it allows for waiting for a group of goroutines, as opposed to all goroutines in my proposal (#65336), though I do have some concerns:
|
One of the goals of this proposal is to minimize the amount of unnatural code required to make a system testable. Mock time implementations require replacing calls to idiomatic time package functions with a testable interface. Putting fake time in the standard library would let us just write the idiomatic code without compromising testability. For timeouts, the Also, it would be pointless for |
I wanted to evaluate practical usage of the proposed API. I wrote a version of Run and Wait based on parsing the output of runtime.Stack. Wait calls runtime.Gosched in a loop until all goroutines in the current group are idle. I also wrote a fake time implementation. Combined, these form a reasonable facsimile of the proposed synctest package, with some limitations: The code under test needs to be instrumented to call the fake time functions, and to call a marking function after creating new goroutines. Also, you need to call a synctest.Sleep function in tests to advance the fake clock. I then added this instrumentation to net/http. The synctest package does not work with real network connections, so I added an in-memory net.Conn implementation to the net/http tests. I also added an additional helper to net/http's tests, which simplifies some of the experimentation below: var errStillRunning = errors.New("async op still running")
// asyncResult is the result of an asynchronous operation.
type asyncResult[T any] struct {}
// runAsync runs f in a new goroutine,
// and returns an asyncResult which is populated with the result of f when it finishes.
// runAsync calls synctest.Wait after running f.
func runAsync[T any](f func() (T, error)) *asyncResult[T]
// done reports whether the asynchronous operation has finished.
func (r *asyncResult[T]) done() bool
// result returns the result of the asynchronous operation.
// It returns errStillRunning if the operation is still running.
func (r *asyncResult[T]) result() (T, error) One of the longest-running tests in the net/http package is TestServerShutdownStateNew (https://go.googlesource.com/go/+/refs/tags/go1.22.3/src/net/http/serve_test.go#5611). This test creates a server, opens a connection to it, and calls Server.Shutdown. It asserts that the server, which is expected to wait 5 seconds for the idle connection to close, shuts down in no less than 2.5 seconds and no more than 7.5 seconds. This test generally takes about 5-6 seconds to run in both HTTP/1 and HTTP/2 modes. The portion of this test which performs the shutdown is: shutdownRes := make(chan error, 1)
go func() {
shutdownRes <- ts.Config.Shutdown(context.Background())
}()
readRes := make(chan error, 1)
go func() {
_, err := c.Read([]byte{0})
readRes <- err
}()
// TODO(#59037): This timeout is hard-coded in closeIdleConnections.
// It is undocumented, and some users may find it surprising.
// Either document it, or switch to a less surprising behavior.
const expectTimeout = 5 * time.Second
t0 := time.Now()
select {
case got := <-shutdownRes:
d := time.Since(t0)
if got != nil {
t.Fatalf("shutdown error after %v: %v", d, err)
}
if d < expectTimeout/2 {
t.Errorf("shutdown too soon after %v", d)
}
case <-time.After(expectTimeout * 3 / 2):
t.Fatalf("timeout waiting for shutdown")
}
// Wait for c.Read to unblock; should be already done at this point,
// or within a few milliseconds.
if err := <-readRes; err == nil {
t.Error("expected error from Read")
} I wrapped the test in a synctest.Run call and changed it to use the in-memory connection. I then rewrote this section of the test: shutdownRes := runAsync(func() (struct{}, error) {
return struct{}{}, ts.Config.Shutdown(context.Background())
})
readRes := runAsync(func() (int, error) {
return c.Read([]byte{0})
})
// TODO(#59037): This timeout is hard-coded in closeIdleConnections.
// It is undocumented, and some users may find it surprising.
// Either document it, or switch to a less surprising behavior.
const expectTimeout = 5 * time.Second
synctest.Sleep(expectTimeout - 1)
if shutdownRes.done() {
t.Fatal("shutdown too soon")
}
synctest.Sleep(2 * time.Second)
if _, err := shutdownRes.result(); err != nil {
t.Fatalf("Shutdown() = %v, want complete", err)
}
if n, err := readRes.result(); err == nil || err == errStillRunning {
t.Fatalf("Read() = %v, %v; want error", n, err)
} The test exercises the same behavior it did before, but it now runs instantaneously. (0.01 seconds on my laptop.) I made an interesting discovery after converting the test: The server does not actually shut down in 5 seconds. In the initial version of this test, I checked for shutdown exactly 5 seconds after calling Shutdown. The test failed, reporting that the Shutdown call had not completed. Examining the Shutdown function revealed that the server polls for closed connections during shutdown, with a maximum poll interval of 500ms, and therefore shutdown can be delayed slightly past the point where connections have shut down. I changed the test to check for shutdown after 6 seconds. But once again, the test failed. Further investigation revealed this code (https://go.googlesource.com/go/+/refs/tags/go1.22.3/src/net/http/server.go#3041): st, unixSec := c.getState()
// Issue 22682: treat StateNew connections as if
// they're idle if we haven't read the first request's
// header in over 5 seconds.
if st == StateNew && unixSec < time.Now().Unix()-5 {
st = StateIdle
} The comment states that new connections are considered idle for 5 seconds, but thanks to the low granularity of Unix timestamps the test can consider one idle for as little as 4 or as much as 6 seconds. Combined with the 500ms poll interval (and ignoring any added scheduler delay), Shutdown may take up to 6.5 seconds to complete, not 5. Using a fake clock rather than a real one not only speeds up this test dramatically, but it also allows us to more precisely test the behavior of the system under test. Another slow test is TestTransportExpect100Continue (https://go.googlesource.com/go/+/refs/tags/go1.22.3/src/net/http/transport_test.go#1188). This test sends an HTTP request containing an "Expect: 100-continue" header, which indicates that the client is waiting for the server to indicate that it wants the request body before it sends it. In one variation, the server does not send a response; after a 2 second timeout, the client gives up waiting and sends the request. This test takes 2 seconds to execute, thanks to this timeout. In addition, the test does not validate the timing of the client sending the request body; in particular, tests pass even if the client waits The portion of the test which sends the request is: resp, err := c.Do(req) I changed this to: rt := runAsync(func() (*Response, error) {
return c.Do(req)
})
if v.timeout {
synctest.Sleep(expectContinueTimeout-1)
if rt.done() {
t.Fatalf("RoundTrip finished too soon")
}
synctest.Sleep(1)
}
resp, err := rt.result()
if err != nil {
t.Fatal(err)
} This test now executes instantaneously. It also verifies that the client does or does not wait for the ExpectContinueTimeout as expected. I made one discovery while converting this test. The synctest.Run function blocks until all goroutines in the group have exited. (In the proposed synctest package, Run will panic if all goroutines become blocked (deadlock), but I have not implemented that feature in the test version of the package.) The test was hanging in Run, due to leaking a goroutine. I tracked this down to a missing net.Conn.Close call, which was leaving an HTTP client reading indefinitely from an idle and abandoned server connection. In this case, Run's behavior caused me some confusion, but ultimately led to the discovery of a real (if fairly minor) bug in the test. (I'd probably have experienced less confusion, but I initially assumed this was a bug in the implementation of Run.) At one point during this exercise, I accidentally called testing.T.Run from within a synctest.Run group. This results in, at the very best, quite confusing behavior. I think we would want to make it possible to detect when running within a group, and have testing.T.Run panic in this case. My experimental implementation of the synctest package includes a synctest.Sleep function by necessity: It was much easier to implement with an explicit call to advance the fake clock. However, I found in writing these tests that I often want to sleep and then wait for any timers to finish executing before continuing. I think, therefore, that we should have one additional convenience function: package synctest
// Sleep pauses the current goroutine for the duration d,
// and then blocks until every goroutine in the current group is idle.
// It is identical to calling time.Sleep(d) followed by Wait.
//
// The caller of Sleep must be in a goroutine created by Run,
// or a goroutine transitively started by Run.
// If it is not, Sleep panics.
func Sleep(d time.Duration) {
time.Sleep(d)
Wait()
} The net/http package was not designed to support testing with a fake clock. This has served as an obstacle to improving the state of the package's tests, many of which are slow, flaky, or both. Converting net/http to be testable with my experimental version of synctest required a small number of minor changes. A runtime-supported synctest would have required no changes at all to net/http itself. Converting net/http tests to use synctest required adding an in-memory net.Conn. (I didn't attempt to use net.Pipe, because its fully-synchronous behavior tends to cause problems in tests.) Aside from this, the changes required were very small. My experiment is in https://go.dev/cl/587657. |
This proposal has been added to the active column of the proposals project |
Commenting here due to @rsc's request: Relative to my proposal #65336, I have the following concerns:
|
Regarding overriding the The In contrast, we can test code which calls Time is fundamentally different in that there is no way to use real time in a test without making the test flaky and slow. Time is also different from an Since we can't use real time in tests, we can insert a testable wrapper around the In addition, if we define a standard testable wrapper around the clock, we are essentially declaring that all public packages which deal with time should provide a way to plumb in a clock. (Some packages do this already, of course; crypto/tls.Config.Time is an example in That's an option, of course. But it would be a very large change to the Go ecosystem as a whole. |
The pprof.SetGoroutineLabels disagrees.
It doesn't try to hide it, more like tries to restrict people from relying on numbers.
If I understood proposal correctly, it will wait for any goroutine (and recursively) that was started using |
Yes, if you call |
Given that there's more precedent for goroutine identity than I had previously thought, and seeing how However, I'm still a little ambivalent about goroutine groups affecting That being said, I agree that plumbing a time/clock interface through existing code is indeed tedious, and having |
Thanks for doing the experiment. I find the results pretty compelling.
I don't quite understand this function. Given the fake time implementation, if you sleep even a nanosecond past timer expiry, aren't you already guaranteed that those timers will have run because the fake time won't advance to your sleep deadline until everything is blocked again?
Partly I was wondering about nested groups because I've been scheming other things that the concept of a goroutine group could be used for. Though it's true that, even if we have groups for other purposes, it may make sense to say that synctest groups cannot be nested, even if in general groups can be nested. |
You're right that sleeping past the deadline of a timer is sufficient. The It's fairly natural to sleep to the exact instant of a timer, however. If a cache entry expires in some amount of time, it's easy to sleep for that exact amount of time, possibly using the same constant that the cache timeout was initialized with, rather than adding a nanosecond. Adding nanoseconds also adds a small but real amount of confusion to a test in various small ways: The time of logged events drifts off the integer second, rate calculations don't come out as cleanly, and so on. Plus, if you forget to add the necessary adjustment or otherwise accidentally sleep directly onto the instant of a timer's expiry, you get a race condition. Cleaner, I think, for the test code to always resynchronize after poking the system under test. This doesn't have to be a function in the synctest package, of course;
I'm very intrigued! I've just about convinced myself that there's a useful general purpose synchronization API hiding in here, but I'm not sure what it is or what it's useful for. |
For what it's worth, I think it's a good thing that virtual time is included in this, because it makes sure that this package isn't used in production settings. It makes it only suitable for tests (and very suitable). |
It sounds like the API is still:
Damien suggested adding also:
The difference between time.Sleep and synctest.Sleep seems subtle enough that it seems like you should have to spell out the Wait at the call sites where you need it. The only time you really need Wait is if you know someone else is waking up at that very moment. But then if they've both done the Sleep+Wait form then you still have a problem. You really only want some of the call sites (maybe just one) to use the Sleep+Wait form. I suppose that the production code will use time.Sleep since it's not importing synctest, so maybe it's clear that the test harness is the only one that will call Sleep+Wait. On the other hand, fixing a test failure by changing s/time.Sleep/synctest.Sleep/ will be a strange-looking bug fix. Better to have to add synctest.Wait instead. If we really need this, it could be synctest.SleepAndWait but that's what statements are for. Probably too subtle and should just limit the proposal to Run and Wait. |
Some additional suggestions for the description of the
Additionally, for "mutex operation", let's list out the the exact operations considered for implementation/testing completeness:
|
The API looks simple and that is excellent. What I am worried about is the unexpected failure modes, leading to undetected regressions, which might need tight support in the testing package to detect. Imagine you unit test your code but are unable to mock out a dependency. Maybe due to lack of experience or bad design of existing code I have to work with. That dependency that suddenly starts calling a syscall (e.g. to lazily try to tune the library using a sync.Once instead of on init time and having a timeout). Without support in testing you will never detect that now and only your tests will suddenly time out after an innocent minor dependency update. |
May I ortgogonally to the previous comment suggest to limit this package to standard library only to gather more experience with that approach before ? That would also allow to sketch out integration with the testing package in addition to finding more pitfalls. |
Can you expand more on what you mean by undetected regressions? If the code under test (either directly, or through a dependency) unexpectedly calls a blocking syscall,
What kind of support are you thinking of? |
What does this do?
Does it succeed or panic? It's not clear to me from the API docs because:
This is obviously a degenerate case, but I think it also applies if a test wanted to get the fake time features when testing otherwise non-concurrent code. |
In this case, the goroutine calling |
synctest.Run waits for all bubbled goroutines to exit before returning. Establish a happens-before relationship between the bubbled goroutines exiting and Run returning. For #67434 Change-Id: Ibda7ec2075ae50838c0851e60dc5b3c6f3ca70fb Reviewed-on: https://go-review.googlesource.com/c/go/+/647755 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Damien Neil <dneil@google.com> Reviewed-by: Michael Pratt <mpratt@google.com>
This comment has been minimized.
This comment has been minimized.
Change https://go.dev/cl/651555 mentions this issue: |
👋🏼 Hi there, not sure if this was mentioned but time.NewTicker that doesn't call Stop does not end up panicking. The test instead just hangs forever. Here's a repro: func TestTicker(t *testing.T) {
synctest.Run(func() {
time.NewTicker(time.Second)
})
} The above test hangs forever. If I instead call Stop, then it will pass correctly. I expect the above should panic similar to when a goroutine is left hanging when func ends like so: func TestDanglingGoroutine(t *testing.T) {
synctest.Run(func() {
go func() {
select {}
}()
})
} More general feedback:
|
Thanks for the report!
This is on my known issues list. (#67434 (comment)) I think that the fix here is going to be to stop advancing time once the goroutine started by Run exits. Any leftover Tickers or Timers will not fire. If there are any goroutines blocked reading from a ticker, Run will panic with a deadlock error.
Thanks especially for the experience report on this behavior. It's a bit unusual to enforce goroutine cleanup (there's a lot of code out there that leaks goroutines, especially in tests), so I'm glad to hear that it seems to have worked out as intended for you. |
Thank you @neild for "synctest". I was recently working on a task queue execution engine, and this would have been nearly impossible to thoroughly test without either lots of hooks, tests being slow, and/or being flaky. With "synctest", the tasks execute very quickly and I can easily craft the exact conditions for potential races and verifying that our locking patterns properly handle them. I am reasonably confident in the correctness of my code because of this package. Some thoughts:
|
In the case where you want to start the synthetic time at some other point, I think 2000-01-01 has a few advantages:
Oops, that's a bug. I thought we weren't recording a monotonic reading. I'll fix that.
That's an interesting thought. I think it would need to be a GODEBUG setting or similar; we don't want to make it easy to use synctest.Wait in non-test code. (Perhaps there's a good use case for a Wait-style operation in production code, but if so we should design that as a separate feature outside of synctest.) Would the bubble clock still start at a synthetic base time? I can see some tricky concerns either way. |
Change https://go.dev/cl/657415 mentions this issue: |
What is the general guidance for integration tests of HTTP servers which currently use https://pkg.go.dev/net/http/httptest? #71345 correctly points out that the docs say:
While this is true in the general sense, in practice many tests are currently written with httptest in such a way that it's only the same Go test and goroutines which interact via HTTP clients. I have some of those tests which currently use a fake clock to mimic e.g. cookie or access token expiry, and it's not clear to me how I should attempt to use synctest in these tests. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I think for httptest we'd need #14200 |
You can use synctest and httptest together, but it's a bit tricky at the moment. You need to provide a fake network, which you can inject by setting httptest.Server.Listener and http.Transport.DialContext to appropriate fake implementations. A very barebones example: The net/http package contains a more full-featured approach in its internal tests: If synctest is promoted out of experimental status, I think we'll want to consider whether we should provide an easy-to-use fake network for httptest (#14200). |
Don't include a monotonic time in time.Times created inside a bubble, to avoid the confusion of different Times using different monotonic clock epochs. For #67434 goos: darwin goarch: arm64 pkg: time cpu: Apple M1 Pro │ /tmp/bench.0 │ /tmp/bench.1 │ │ sec/op │ sec/op vs base │ Since-10 18.42n ± 2% 18.68n ± 1% ~ (p=0.101 n=10) Until-10 18.28n ± 2% 18.46n ± 2% +0.98% (p=0.009 n=10) geomean 18.35n 18.57n +1.20% Change-Id: Iaf1b80d0a4df52139c5b80d4bde4410ef8a49f2f Reviewed-on: https://go-review.googlesource.com/c/go/+/657415 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Damien Neil <dneil@google.com> Reviewed-by: Michael Pratt <mpratt@google.com>
I've been using One rough edge I've felt concerns its potential interaction with This is reasonably safe as of the current version but may not be safe in future versions. If we want the T Context to be safe to use inside a bubble, then there's a case to be made for an added test. I took a look around and didn't see a test covering this. This may also add a point in favor of the earlier
|
I've been thinking on the interactions between the synctest and testing packages. I've got a few ideas so far that seem workable, with various tradeoffs. The simplest I can think of is to add
We could then make the testing package aware of bubbles: This has the advantage of simplicity, but the disadvantage of losing the clean delineation of the bubble lifetime that Another option would be to provide a function that starts a bubble with a
Or in the testing package:
|
TL;DRI am exceedingly happy with My use-case is a single-binary that implements an execution platform. My main (self-inflicted) pain points were:
The only downside was giving up remote logging (via Despite those minor points, in <6 hours, I was able to:
It would be an understatement to say that I would be sad if *as further noted below, I was virtually forced to use **run on M3 Macbook Air DetailsGeneral ArchitectureThe following is an illustrative subset of the use-case I have; most notably it includes:
Prior to package broker
// Broker dispatchers HTTP to the broker via REST.
func Broker(ctx context.Context, ...) (chan<- Request, chan<- Refresh) {
reqs := make(chan Request)
refresh := make(chan Refresh)
db, cleanup, _ := dataplane.OpenRWConnection(dataplane.Broker, ...)
clientCache := make(map[dataplane.ExecutorT]*http.Client) // oauth2 details abstracted away here
logclient, flush, _ := monitoring.NewCloudLogger(...)
go func() {
for {
select {
case <-ctx.Done():
flush()
cleanup()
return
case <-time.After(time.Minute): // lightweight thing
case r := <-reqs: // 5 QPS, network I/O thing, uses `clientCache`
case r := <-refresh: // infrequent, network I/O thing, mutates `clientCache`
}
}
}()
return reqs, refresh
} package cache
// CacheLayer provides synchronized access to post-processed broker data.
func CacheLayer(ctx context.Context, dispatch chan<- broker.Request, ...) chan<- Request {
reqs := make(chan Request)
logclient, flush, _ := monitoring.NewCloudLogger(...)
cache := make(map[any]any) // abstracting details away
go func() {
for {
select {
case <-ctx.Done():
flush()
return
case <-time.After(time.Second): // heavy calculation (1000s of PDEs), network I/O via `dispatch`
case r := <-reqs: // 50 QPS, uses post-processed cache
}
}
}()
return reqs
} package executor
// Executor performs core business-logic using the data via the broker.
func Executor(ctx context.Context, dispatch chan<- broker.Request, oracle chan<- cache.Request, ...) {
db, cleanup, _ := dataplane.OpenRWConnection(dataplane.Executor, ...)
logclient, flush, _ := monitoring.NewCloudLogger(...)
trig := make(chan struct{}, 1)
go func() {
for {
select {
case <-ctx.Done():
flush()
cleanup()
return
case <-time.After(time.Minute): // uses `db`, `dispatch`, `oracle` [cheap, non-critical]
case <-time.After(4*time.Second): // uses `db`, `dispatch`, `oracle` [expensive, critical]
case <-trig: // uses `db`, `dispatch`, `oracle` [cheap, critical]
}
}
}()
} Testing StrategyPrevious to My testing platform implements 2 fake components:
These components are implemented in a similar manner to the above; there are a mix of time-based and event-based triggers with similar costs and criticality. I chose this strategy due to the high coverage of production code in a SUT. Additionally, it covers more components than those listed here for free. Some details are omitted below; however, it is a faithful representation of the use-case. package executor
func setupExecutorIntegrationTest(ctx context.Context) (chan<- sim.Request, func(), *sql.DB) { // with sqlc, you can just return `*<db-pkg>.Queries` in place of `*sql.DB`
brokersim := sim.BrokerPlatform(ctx, ...)
httpclient, netclean , _ := newTestHttpServer(ctx, brokersim) // large-ish implementation of switch { case ... } on real API paths
dispatch, refresh := broker.Broker(ctx, ...) // via abstraction, httpclient.Client() is passed here
oracle := cache.CacheLayer(ctx, dispath, ...)
Executor(ctx, dispatch, oracle, ...) // in-memory db via abstraction
db, dbclean, _ := dataplane.OpenReadOnlyConnection(dataplane.Executor, ...) // in-memory db
return brokersim, func() { netclean(); dbclean() }, db
} DownsidesThere are numerous, well-known downsides to this sort of testing instrumentation; in my case, the most relevant:
For example, I virtually was forced to run all my tests with func TestIntegration_SomeCase(...) {
t.Parallel()
ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
brokersim, clk, cleanup, db := setupExecutorIntegrationTest(ctx)
t.Cleanup(cleanup)
clk.Set(...)
simdata(brokersim, ...) // do some set up
for range 120 { // invariant is average flaky
clk.Add(time.Second)
}
assert(t, db, ...) // invariant for event-based / time-based behavior
simdata(brokersim, ...) // do some set up
for range 50 { // invariant is less flaky
clk.Add(time.Second)
}
assert(t, db, ...) // invariant for event-based / time-based behavior
simdata(brokersim, ...) // do some set up
for range 250 { // invariant is more flaky
clk.Add(time.Second)
}
assert(t, db, ...) // invariant for event-based / time-based behavior
}
|
To avoid introducing an additional level of nesting or function indirection, what do you think about adding a // Isolated signals that this test is to be run inside an isolated "bubble".
// The test function will first be run as normal. When this function is called
// for the first time, the test function will exit immediately (via runtime.Goexit()),
// and then the test function will be called again from a bubbled goroutine.
// The second call to this function will be a no-op.
func (t *T) Isolated() We can then write tests in the following manner: func TestSleep(t *testing.T) {
t.Isolated()
a := time.Now()
time.Sleep(24 * time.Hour)
b := time.Now()
if b.Sub(a) != 24 * time.Hour {
t.Error("sleep not exact")
}
}
|
You still need to call If we did keep the |
I personally like But if significant API changes are still on the table, a further idea that the above discussion inspired was to have This would mean that the bubble manipulations can be done only by code that has access to that "bubble" object. I could imagine arguing that both as an advantage (it means you don't need to worry about spooky action at a distance with random code elsewhere fiddling with the bubble in ways that are not obvious from the test code) and as a disadvantage (you now have another value in addition to |
I've created a sub-issue with a proposal for a revised version of the API that replaces (Creating as a separate proposal to keep the discussion of this specific change organized.) |
I've been using synctest lately, and it's pretty nice. I have one small request. Could we add to the comment on Consider also that there were a lot of discussions about whether a goroutine blocked on a Mutex should be durably blocked, and the initial proposal text mentioned mutex operations as supported. To quote Russ:
I agree it's weird and especially non-obvious. I understand the reasons for not support sync.Mutex right now. I'm not suggesting a change to support them. I am suggesting an addition to the comment on |
First of all, I haven't used synctest, so my comment is purely based on reading the docs. But I'd like to see more control over the wall clock. For testing line-of-business applications, setting a specific wall clock is often extremely useful in tests. E.g, setting the time to 23:59:59 the last day of the month, vs. 00:00:01 the following day, to check if a rule was applied. This becomes even more critical for multi-time zone systems (i.e., the ability to control UTC wall clock for testing a multi-time zone system is critical) According to the docs.
This should make it technically possible to forward time to the desired simulated wall clock. time.Sleep(MustParseISOTime("2025-04-01T12:00-01:00").Sub(time.Now())) But it's not as explicit as something like A suggestion could be passing a value to the bubble to interact with time itself? type T interface {
SetTime(time.Time)
}
Run(func(T)) |
This is exactly the right way to set a precise simulated time: We could add a Using |
Change https://go.dev/cl/662155 mentions this issue: |
Current proposal status: #67434 (comment)
This is a proposal for a new package to aid in testing concurrent code.
This package has two main features:
As an example, let us say we are testing an expiring concurrent cache:
A naive test for this cache might look something like this:
This test has a couple problems. It's slow, taking four seconds to execute. And it's flaky, because it assumes the cache entry will not have expired one second before its deadline and will have expired one second after. While computers are fast, it is not uncommon for an overloaded CI system to pause execution of a program for longer than a second.
We can make the test less flaky by making it slower, or we can make the test faster at the expense of making it flakier, but we can't make it fast and reliable using this approach.
We can design our Cache type to be more testable. We can inject a fake clock to give us control over time in tests. When advancing the fake clock, we will need some mechanism to ensure that any timers that fire have executed before progressing the test. These changes come at the expense of additional code complexity: We can no longer use time.Timer, but must use a testable wrapper. Background goroutines need additional synchronization points.
The synctest package simplifies all of this. Using synctest, we can write:
This is identical to the naive test above, wrapped in synctest.Run and with the addition of two calls to synctest.Wait. However:
A limitation of the synctest.Wait function is that it does not recognize goroutines blocked on network or other I/O operations as idle. While the scheduler can identify a goroutine blocked on I/O, it cannot distinguish between a goroutine that is genuinely blocked and one which is about to receive data from a kernel network buffer. For example, if a test creates a loopback TCP connection, starts a goroutine reading from one side of the connection, and then writes to the other, the read goroutine may remain in I/O wait for a brief time before the kernel indicates that the connection has become readable. If synctest.Wait considered a goroutine in I/O wait to be idle, this would cause nondeterminism in cases such as this,
Tests which use synctest with network connections or other external data sources should use a fake implementation with deterministic behavior. For net.Conn, net.Pipe can create a suitable in-memory connection.
This proposal is based in part on experience with tests in the golang.org/x/net/http2 package. Tests of an HTTP client or server often involve multiple interacting goroutines and timers. For example, a client request may involve goroutines writing to the server, reading from the server, and reading from the request body; as well as timers covering various stages of the request process. The combination of fake clocks and an operation which waits for all goroutines in the test to stabilize has proven effective.
@gabyhelp's overview of this issue: #67434 (comment)
The text was updated successfully, but these errors were encountered: