Open Tatskaari opened 4 years ago
Thanks for the report I will look into this
I'm having a hard time coming up with a solution that doesn't involve panic
and doesn't involve changing the API for tests.
If there was a reliable way to tell whether we are running in the main test thread we could panic
only when we are in a subroutine
It would be great if we could do something channel based (or something like that)
I'm not sure how we would package the API but something like
select {
case <- testFinished:
....
case <- subRoutineFailed:
t.FailNow()
}
relevant issue https://github.com/golang/go/issues/15758
It may be possible to do something similar to the http2 pkg in order to track the main test routine https://github.com/golang/net/blob/2491c5de3490fced2f6cff376127c667efeed857/http2/gotrack.go#L22
The more I think about it the more building some kind of test wrapper seems like a really good idea.
1) T.Fatal
, T.FailNow
, etc shouldn't be called in a goroutine and that probably will never change
2) Calling panic is non-optimal as it prevents other tests from running
A test wrapper could look something like this https://play.golang.org/p/NXGIGTlK7fv
func Wrapper(t *testing.T, failChan chan bool, wrappedTestFunc func() error) {
done := func() chan bool {
doneChan := make(chan bool)
go func() {
wrappedTestFunc()
doneChan <- true
}()
return doneChan
}()
select {
case <-done:
log.Print("done")
case <-failChan:
time.Sleep(time.Second)
t.Fatal("meow")
}
}
The only other option I can think of would be to allow the function call on the mock and return zero values (iirc this is default behavior when Return
is not specified). We could then call T.FailNow
inside Controller.Finish
. This would prevent deadlocks and T.FailNow
from being called outside the main test routine. It seems like it could produce some unexpected behavior, but no more so than an EXPECT
with a Return
.
I'm not sure which would be better. The wrapper feels like idiomatic golang IMO and seems like it would be a more robust solution for concurrent/async code.
Allowing methods to be called on mocks even when there is no EXPECT
and then failing from the main test routine seems desirable because it requires no change to API. It does seem more brittle to me in some way though; it does depend on users calling Controller.Finish
which I don't know how often people forget that.
Related #145
To check whether we're on the main test goroutine, I have a couple of possible ideas:
NewController
is called, and when a mock fails, check whether the earlier captured stacktrace is part of the current goroutine's stacktrace. If it isn't, then we are not on the same goroutine.testing.tRunner
), and if we find that, we are on the main test goroutine and can use t.Fatalf
, otherwise, we cannot (and may be better off using panic
).The first suggestion assumes that functions like t.Run
don't start a separate goroutine, while the second suggestion relies on internals of the testing
library that may change across versions.
Current state
The gomock framework causes a deadlock when a call is made on a mock from within a goroutine when using testing.T as the TestReporter. This is because testing.T.FailNow() can't be called from any goroutine that's not the main test routine. If it is, that goroutine will exit and may cause hard to diagnose deadlocks in your code.
Here is an example "baz" service that depends on a "foo" service.
Here are some tests for the baz service:
Desired state
The unexpected call to Bar should result in the test erroring out with an "unexpected call" error. We could implement a TestReporter such as:
This will result in the whole test process dying so the next tests won't execute however at least it's clear what went wrong.
I've spent quite a lot of time sifting through code to try and figure out what's been happening. The goroutine this was happening in was actually deep in a library we're using so you can imagine how much fun I've had. Would it be worth having
NewController
wrap the test reporter in such a way so others don't get caught out like this?Side moan: if we could be trusted with IDs for goroutines, we could detect if we're on the main thread and only panic if we're not...