test deadline protection and convenience methods #416
Conversation
patrickhuie19
temporarily deployed
to
integration
GitHub Actions
Inactive
patrickhuie19
temporarily deployed
to
integration
GitHub Actions
Inactive
patrickhuie19
temporarily deployed
to
integration
GitHub Actions
Inactive
| @@ -68,3 +72,34 @@ func RequireSignal(t *testing.T, ch <-chan struct{}, failMsg string) { | |||
| t.Fatal(failMsg) | |||
| } | |||
| } | |||
|
|
|||
| // Convenience method to setup a client connection with a default timeout of 5 seconds | |||
| func SetupClientConnWithOptsAndTimeout(t *testing.T, target string, opts ...grpc.DialOption) (*grpc.ClientConn, error) { | |||
There was a problem hiding this comment.
If you like this convenience method, I can refactor the other tests to consume it.
|
|
||
| // SetupServer is a convenience method to set up a grpc server for most testing | ||
| // usecases. | ||
| func SetupServer(t *testing.T, opts ...grpc.ServerOption) (net.Listener, *grpc.Server, string) { |
There was a problem hiding this comment.
If you like this convenience method, I can refactor the other tests to consume it.
There was a problem hiding this comment.
thanks for being proactive.
i had a personal TODO to investigate a refactor of some of the copy-pasta amongst the grpc tests.
i think there would be value to some sort of test template driver that can take a take a server and client type, set them up and run a func. that is, there is a lot of repeated boilerplate that could be reduced.
however, i don't like the proposed changes here. they strike me a bad middle ground. the RequireSignal and SetupClienConn... are convoluted indirection to simple primitives; the reader can immediately understand the original code but has to context switch for the new code and the new code is not provide any simplification
the setupserver is better, but only if we can avoid returning the listener, and the eventually and timeouts are extra cruft that is not needed.
| t.Log("shutting down server") | ||
| testServer.Stop() | ||
| }() | ||
| // create a price registry client | ||
| conn, err := grpc.Dial(addr, grpc.WithTransportCredentials(insecure.NewCredentials())) | ||
| conn, err := tests.SetupClientConnWithOptsAndTimeout(t, addr, grpc.WithTransportCredentials(insecure.NewCredentials())) |
There was a problem hiding this comment.
maybe if we change this to ~ test.InsecureDial(t, addr) and mv the require.NoError & t.cleanup calls
| @@ -65,12 +58,12 @@ func TestOnRampGRPC(t *testing.T) { | |||
| wg.Add(1) | |||
| go func() { | |||
| defer wg.Done() | |||
| <-shutdown | |||
| tests.RequireSignal(t, shutdown, "Failed to receive shutdown signal") | |||
There was a problem hiding this comment.
This was actually why I wanted to open the PR - I was worried about shutdown hanging indefinitely and preventing the test from closing (and then saw the opportunity to reduce some redundancy in the setup used by all the tests).
In my first iteration on this PR I implemented RequireSignal by hand (i.e. a for loop to check if the shutdown had received anything or if we had time'd out), and then later saw that Jordan had beat me to it a while back.
There was a problem hiding this comment.
the test timeout will prevent them from hanging forever. perhaps requiresignal is a nice abstraction in a complicated code base, where you don't control the channel. here it is overkill.
| // we explicitly stop the server later, do not add a cleanup function here | ||
| testServer := grpc.NewServer() | ||
| lis, testServer, addr := tests.SetupServer(t) |
There was a problem hiding this comment.
why do we need to return lis?
tests.SetupGRPCServer would be a better name
There was a problem hiding this comment.
Fair - I'm not sure how much I like it either. I followed a similar pattern to WSRPC tests.
|
|
||
| port := freeport.GetOne(t) | ||
| addr := fmt.Sprintf("localhost:%d", port) | ||
| require.Eventually(t, func() bool { |
There was a problem hiding this comment.
i don't think the eventually is needed. it's a synchronous call that has no good reason to recover.
There was a problem hiding this comment.
The OS can still be freeing up the port while the next test is executed. This eventually block gives the OS some wiggle room to do that. This isn't strictly needed as you the current tests use freeport, but there may be scenarios in the future that want to use the same port across tests.
There was a problem hiding this comment.
freeport library handles the concern. there is no clear advantage to reusing ports across unit tests and there are clear disadvantages. we removed a number of flake integration test in core by adopting freeport instead of trying to manage it ourselves
Fair, and I agree the current state needs edits. I wanted to get something quick up before I got sucked back into other things and I thought you might have some ideas given you wrote these tests 😅 . We can keep iterating if its headed the right direction. |
No description provided.