Cannot test correct handling of `serverStream.Send` error

[edmondop]

Describe the bug

ServerStream.Send() can return an error, which should be correctly handled in implementation of Handler.

// ServerStream is the handler's view of a server streaming RPC.
//
// It's constructed as part of [Handler] invocation, but doesn't currently have
// an exported constructor.
type ServerStream[Res any] struct {
    conn StreamingHandlerConn
}
type StreamingHandlerConn interface {
    Spec() Spec
    Peer() Peer

    Receive(any) error
    RequestHeader() http.Header

    Send(any) error
    ResponseHeader() http.Header
    ResponseTrailer() http.Header
}

Unfortunately, the lack of a public constructor for ServerStream doesn't allow to mock StreamingHandlerConn, and test for the correct behavior of the handler. For example, if the handler needs to do some special cleanup or action when Send fails, today it's impossible to unit test this

Additional context I'll be happy to contribute back the fix, though it's unclear to me whether we want to make the Handler depends on an interface rather than the ServerStream struct

[emcfarlane]

Hey @edmondop, thanks for raising the issue. Similar discussions have been raised recently in https://github.com/connectrpc/connect-go/discussions/708 and https://github.com/connectrpc/connect-go/issues/694 . Opening the type up for constructors seems like a valuable way to solve this unit style testing. I'd be happy to see an approach but will leave it to others to weigh in on the API.

For now you may use an interceptor with a WrapStreamingHandler method that takes in the parent connection and injects faults on Send. The faults could be triggered by a test header or context variable.

type testStreamConn struct {
    connect.StreamingHandlerConn
}

func (c *testStreamConn) Send(msg any) error {
    if c.RequestHeader().Get("Fail-Send") != "" {
        return errors.New("fail send")
    }
    return c.StreamingHandlerConn.Send(msg)
}

Note: this would still require setting up a httptest server or similar for serving the service.

[pd93]

Hey @emcfarlane, is there any reason this is more complicated than just adding something like this?

func NewServerStream[Res any](conn StreamingHandlerConn) *ServerStream[Res] {
    return &ServerStream[Res]{
        conn: conn,
    }
}

We've recently started moving from unary calls to streaming on some of our APIs and mocking them has been a bit of a problem. However, I've got everything working perfectly in this fork with minimal changes. if you're happy with the API, I'm happy to open a PR from my fork.

[emcfarlane]

@pd93 thanks for having a look. The new constructor methods should be used internally too.

[pd93]

@emcfarlane Good point, I've updated my fork and opened #731.

[emcfarlane]

Hey @pd93, the extra API surface may cause issues with maintenance going forwards. For the use cases of mocking another solution could be to create your own interface types which are called from the connect handler methods. Now the stream can be mocked for testing by implementing the interface and calling the indirect implementation.

// BidiStream is an interface subset of the methods implemented from [connect.BidiStream]
type BidiStream[Req, Res any] interface {
    Send(*Res) error
    Receive() (*Req, error)
    // other methods required by implementations
}

// PingService.CumSum
// See: https://github.com/connectrpc/connect-go/blob/1abde82b828ae6e6842d09174713defaff0f48de/internal/gen/connect/ping/v1/pingv1connect/ping.connect.go#L180
func (s *Server) CumSum(ctx context.Context, stream *connect.BidiStream[v1.CumSumRequest, v1.CumSumResponse]) error {
    return s.cumSum(ctx, stream)
}

func (s *Server) cumSum(ctx context.Context, stream BidiStream[v1.CumSumRequest, v1.CumSumResponse]) error {
    return nil // TODO
}

Mocking is generally difficult to get right and would usually encourage testing using a connect client to invoke the service.

[emcfarlane]

Closing the issue until we have a stronger use case for extending the API surface. Workarounds posted in this thread and previous discussions cover the current case.