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

chore: add unary-unary showcase test for gRPC support #1501

Merged
merged 16 commits into from
Mar 22, 2023
Merged

Conversation

mpeddada1
Copy link
Contributor

Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide

Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

@product-auto-label product-auto-label bot added the size: l Pull request size is large. label Mar 16, 2023
@mpeddada1 mpeddada1 marked this pull request as ready for review March 16, 2023 20:42
@mpeddada1 mpeddada1 requested a review from a team as a code owner March 16, 2023 20:42
@lqiu96
Copy link
Contributor

lqiu96 commented Mar 16, 2023

I have a similar PR: #1483 for callables. I can move the httpjson unary tests to ITUnaryCallable after this goes in

public void testGrpc_serverResponseError_throwsException() {
Status cancelledStatus = Status.newBuilder().setCode(StatusCode.Code.CANCELLED.ordinal()).build();
EchoRequest requestWithServerError = EchoRequest.newBuilder().setError(cancelledStatus).build();
CancelledException exception = assertThrows(CancelledException.class, () -> grpcClient.echo(requestWithServerError));
Copy link
Member

Choose a reason for hiding this comment

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

Do we have code formatting set up for this project? Mix of 2+4 space indents plus some rather large lines makes me think something is awry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! Sorry, I missed importing the formatter after switching IDEs.


@Test
public void testHttpJson() {
assertThat(echoHttpJson("http-echo?")).isEqualTo("http-echo?");
Copy link
Member

Choose a reason for hiding this comment

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

What's the plan with the Http tests? Duplicates of the above coming soon?

Copy link
Contributor

@lqiu96 lqiu96 Mar 17, 2023

Choose a reason for hiding this comment

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

I can add in httpjson versions of the gRPC calls in a separate PR

@mpeddada1
Copy link
Contributor Author

I have a similar PR: #1483 for callables. I can move the httpjson unary tests to ITUnaryCallable after this goes in

Thanks @lqiu96! Yes, if all looks okay with how the tests are set up here then perhaps we can continue building on top of this class.

@mpeddada1
Copy link
Contributor Author

Lint check for showcase is now failing as expected after introducing changes that are incompatible with the Google Java style guide.
Screenshot 2023-03-20 at 3 50 15 PM

@Test
public void testGrpc_shutdown() {
assertThat(grpcClient.isShutdown()).isFalse();
grpcClient.shutdown();
Copy link
Contributor

Choose a reason for hiding this comment

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

QQ, would this make the test dependent on the ordering? i.e. if testGrpc_shutdown gets called first and then the client is shutdown, would the other tests still run properly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch! I think you're right, this would make the tests interdependent. Let me look into modifying this.

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, looks like avoiding the static states as @burkedavison suggested in the previous comment is the way to go if we are to have self-contained tests.

@sonarqubecloud
Copy link

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@mpeddada1 mpeddada1 merged commit dbed53a into main Mar 22, 2023
@mpeddada1 mpeddada1 deleted the unary-grpc branch March 22, 2023 19:57
@lqiu96 lqiu96 mentioned this pull request Mar 22, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants