[draft] client test stubs #800

glbrntt · 2020-05-18T15:20:59Z

@MrMage this is a draft for the client test stubs. The relevant parts here are:

Sources/Examples/Echo/Model/echo.grpc.swift, and
Tests/GRPCTests/TestClientExample.swift

Would like your feedback on the general shape of this before I take it any further!

This change is

MrMage

Looks good overall as far as I can tell without documentation 👍

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @glbrntt)

Sources/Examples/Echo/Model/echo.grpc.swift, line 145 at r1 (raw file):

    }
  }
  public init() {

super-nit: newline above this?

Sources/GRPC/ClientCalls/BaseClientCall.swift, line 134 at r1 (raw file):

  }

  init(

Document this method? (Also on the other client calls)

Sources/GRPC/ClientCalls/TestResponse.swift, line 19 at r1 (raw file):

import NIO

enum Event<Message: GRPCPayload> {

I would probably also document these two enums

Sources/GRPC/ClientCalls/TestResponse.swift, line 26 at r1 (raw file):

enum EventDelivery<Message: GRPCPayload> {
  case buffered([Event<Message>])
  case callback((Message) -> Void)

I like the idea of passing in blocks to "handle" the request, but I don't quite understand yet how this would work in practice.

Sources/GRPC/ClientCalls/TestResponse.swift, line 58 at r1 (raw file):

  }

  public func sendEnd(status: GRPCStatus = .ok, metadata: HPACKHeaders = [:]) {

rename "metadata" to "trailingMetadata"? (Can be assumed implicitly, but I am a fan of spelling these things out without any room for ambiguity.)

Sources/GRPC/ClientCalls/TestResponse.swift, line 59 at r1 (raw file):

  public func sendEnd(status: GRPCStatus = .ok, metadata: HPACKHeaders = [:]) {
    self.trailingMetadata.succeed(metadata)

Shouldn't these only get fulfilled after all responses have been sent? I.e. "enqueue" these?

Sources/GRPC/ClientCalls/TestResponse.swift, line 153 at r1 (raw file):

    let status = GRPCStatus(
      code: .failedPrecondition,
      message: "Test response must be added before calling the RPC"

How about something like "test response buffer exhausted"?

Sources/GRPC/ClientCalls/TestResponse.swift, line 156 at r1 (raw file):

    )
    call.sendEnd(status: status)
    try! loop.syncShutdownGracefully()

Are we sure that the loop can already be shut down again at this point? I am concerned that when we return call after this line and its properties/loop get used, we get into trouble.

Tests/GRPCTests/TestClientExample.swift, line 31 at r1 (raw file):

    })

    // We should probably (?) just `sendEnd()` anyway after `sendResponse()` is called on unary

Any way we could make this more convenient, at least for unary calls?

Tests/GRPCTests/TestClientExample.swift, line 36 at r1 (raw file):

    let get = client.get(.with {
      $0.text = "This request will be ignored"

Should the client also record the requests sent, so that we can validate that the correct requests got sent?

Tests/GRPCTests/TestClientExample.swift, line 45 at r1 (raw file):

  }

  func testBidirectional() {

I think we should have tests for all 4 modes, but I guess you were already planning to add those later on.

Tests/GRPCTests/TestClientExample.swift, line 61 at r1 (raw file):

    // ... as well as after making the call.
    responseStream.sendResponse(.with { $0.text = "2" })
    XCTAssertEqual(responses, [.with { $0.text = "1" }, .with { $0.text = "2" }])

Same here; could we somehow also validate the sent requests (and maybe even the metadata)?

glbrntt

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @glbrntt and @MrMage)

Sources/GRPC/ClientCalls/TestResponse.swift, line 59 at r1 (raw file):