Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate ClientCallUnary test stubs. #398

Merged
merged 2 commits into from
Mar 11, 2019
Merged

Generate ClientCallUnary test stubs. #398

merged 2 commits into from
Mar 11, 2019

Conversation

mpetrov
Copy link
Contributor

@mpetrov mpetrov commented Mar 9, 2019

This simplifies testing for async calls, e.g. a failing request could
be simulated with something like:

  override func get(_ request: Echo_EchoRequest, metadata customMetadata: Metadata, completion: @escaping (Echo_EchoResponse?, CallResult) -> Void) throws -> Echo_EchoGetCall {
    completion(nil, .error(statusCode: .notFound))
    return Echo_EchoGetCallTestStub()
  }

This simplifies testing for async calls, e.g. a failing request could
be simulated with something like:
```
  override func get(_ request: Echo_EchoRequest, metadata customMetadata: Metadata, completion: @escaping (Echo_EchoResponse?, CallResult) -> Void) throws -> Echo_EchoGetCall {
    completion(nil, .error(statusCode: .notFound))
    return Echo_EchoGetCallTestStub()
  }
```
Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This is how I wanted to avoid having to create an unused Channel in
#378 (see my review of that PR). That PR needs a few more cleanups; afterwards stubbing async calls should be fully supported.

@@ -56,3 +56,10 @@ open class ClientCallUnaryBase<InputType: Message, OutputType: Message>: ClientC
return self
}
}

/// Simple fake implementation of `ClientCallUnary`.
open class ClientCallUnaryTestStub<InputType: Message, OutputType: Message>: ClientCallUnary {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be generic at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary at the moment, was primarily going for consistency with ClientCallClientStreamingTestStub. The generics would be necessary if we want similar request queueing or response recording, thought TBH I find that approach a bit to constrained for my use-cases. In unit tests I'd generally want to test a lot of error cases and in integration tests I'd generally want the stub to have business logic (as in, I really want an in-memory fake).

I don't have strong preferences regarding the generics, let me know if you'd prefer I remove them. Happy to update the PR either way.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the elaboration! For now, let's remove them; I'm open to re-adding them later on if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@MrMage MrMage merged commit c0478aa into grpc:master Mar 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants