From 19876b9933899cd11ff3db6c87d597ec0cd79432 Mon Sep 17 00:00:00 2001 From: Ian Denhardt Date: Wed, 12 Oct 2022 20:53:25 -0400 Subject: [PATCH] Better isolate the tests under TestHandleReturn_regression Previously, these tests used the same connection for both tests. This fixes intermittent test failures on the second test, as discussed in #318. I think, in principle, the tests *should* pass in that case anyway, but: 1. Keeping the cases isolated makes this easier to understand 2. This way we can (and now do) run the tests in parallel 3. While we should better test the scenarios where: - a connection is used after a bootstrap fails - bootstrap is called more than once - etc. Doing so without tests dedicated to those things will be very difficult to maintain; we could add some but that should be a separate (low priority) task. --- rpc/level0_test.go | 86 ++++++++++++++++++++++++++-------------------- 1 file changed, 48 insertions(+), 38 deletions(-) diff --git a/rpc/level0_test.go b/rpc/level0_test.go index ef23877c..67251b95 100644 --- a/rpc/level0_test.go +++ b/rpc/level0_test.go @@ -1772,52 +1772,62 @@ func TestSendCancel(t *testing.T) { func TestHandleReturn_regression(t *testing.T) { t.Parallel() - p1, p2 := transport.NewPipe(1) - - srv := testcp.PingPong_ServerToClient(pingPongServer{}) - conn1 := rpc.NewConn(rpc.NewTransport(p2), &rpc.Options{ - BootstrapClient: capnp.Client(srv), - }) - defer conn1.Close() + // Common setup for the tests below. Creates a connection with + // a suitable bootstrap interface on one end, then passes the + // other end of the connection to the callback. + withConn := func(f func(*rpc.Conn)) { + p1, p2 := transport.NewPipe(1) + + srv := testcp.PingPong_ServerToClient(pingPongServer{}) + conn1 := rpc.NewConn(rpc.NewTransport(p2), &rpc.Options{ + BootstrapClient: capnp.Client(srv), + }) + defer conn1.Close() - conn2 := rpc.NewConn(rpc.NewTransport(p1), nil) - defer conn2.Close() + conn2 := rpc.NewConn(rpc.NewTransport(p1), nil) + defer conn2.Close() + f(conn2) + } t.Run("MethodCallWithExpiredContext", func(t *testing.T) { - pp := testcp.PingPong(conn2.Bootstrap(context.Background())) - defer pp.Release() - - // create an EXPIRED context - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - f, release := pp.EchoNum(ctx, func(ps testcp.PingPong_echoNum_Params) error { - ps.SetN(42) - return nil + withConn(func(conn *rpc.Conn) { + pp := testcp.PingPong(conn.Bootstrap(context.Background())) + defer pp.Release() + + // create an EXPIRED context + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + f, release := pp.EchoNum(ctx, func(ps testcp.PingPong_echoNum_Params) error { + ps.SetN(42) + return nil + }) + defer release() + + _, err := f.Struct() + assert.ErrorIs(t, err, ctx.Err()) }) - defer release() - - _, err := f.Struct() - assert.ErrorIs(t, err, ctx.Err()) }) t.Run("BootstrapWithExpiredContext", func(t *testing.T) { - // create an EXPIRED context - ctx, cancel := context.WithCancel(context.Background()) - cancel() - - // NOTE: bootstrap with expired context - pp := testcp.PingPong(conn2.Bootstrap(ctx)) - defer pp.Release() - - f, release := pp.EchoNum(ctx, func(ps testcp.PingPong_echoNum_Params) error { - ps.SetN(42) - return nil + withConn(func(conn *rpc.Conn) { + // create an EXPIRED context + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // NOTE: bootstrap with expired context + pp := testcp.PingPong(conn.Bootstrap(ctx)) + defer pp.Release() + + f, release := pp.EchoNum(ctx, func(ps testcp.PingPong_echoNum_Params) error { + ps.SetN(42) + return nil + }) + defer release() + + _, err := f.Struct() + assert.ErrorIs(t, err, ctx.Err()) }) - defer release() - - _, err := f.Struct() - assert.ErrorIs(t, err, ctx.Err()) }) }