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.

Sign up for GitHub

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 Showcase Callables IT #1483

Merged
merged 19 commits into from
Mar 23, 2023
Merged

chore: Add Showcase Callables IT #1483

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
b43a82a
chore: Add Showcase Callables IT
lqiu96 Mar 13, 2023
45bdea1
chore: Add test scope
lqiu96 Mar 13, 2023
5ef67e9
deps: update google http client dependencies to v1.43.1 (#1487)
renovate-bot Mar 14, 2023
ba9fb82
build(deps): update dependency org.apache.maven.plugins:maven-surefir…
renovate-bot Mar 14, 2023
1206c4c
chore(main): release 2.15.3 (#1445)
release-please[bot] Mar 14, 2023
5de06df
chore(main): release 2.15.4-SNAPSHOT (#1489)
release-please[bot] Mar 14, 2023
640e35f
chore: Refactor paged logic
lqiu96 Mar 14, 2023
46b0b1b
chore: Format ITCallables
lqiu96 Mar 14, 2023
208c1ad
Merge branch 'main' into main-showcase_callables
lqiu96 Mar 14, 2023
eed5def
chore: Format ITCallables
lqiu96 Mar 14, 2023
4d26862
chore: Fix to not found status code
lqiu96 Mar 16, 2023
61ac41a
Merge branch 'main' into main-showcase_callables
lqiu96 Mar 22, 2023
ca777dd
chore: Add HttpJson Unary Callable Showcase Test
lqiu96 Mar 22, 2023
29c0516
chore: Fix truth dependency
lqiu96 Mar 22, 2023
2a7af58
chore: Update test name
lqiu96 Mar 22, 2023
5a13d48
chore: Address code smell
lqiu96 Mar 22, 2023
a6a3676
chore: Remove incorrect comment
lqiu96 Mar 22, 2023
533deec
chore: Update test to CANCELLED
lqiu96 Mar 23, 2023
d3453a3
chore: Clean up PR
lqiu96 Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion showcase/gapic-showcase/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>

<dependency>
<groupId>com.google.truth</groupId>
<artifactId>truth</artifactId>
Expand Down
30 changes: 29 additions & 1 deletion showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITUnaryCallable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import com.google.api.gax.core.NoCredentialsProvider;
import com.google.api.gax.grpc.GrpcStatusCode;
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
import com.google.api.gax.rpc.ApiException;
import com.google.api.gax.rpc.CancelledException;
import com.google.api.gax.rpc.StatusCode;
import com.google.rpc.Status;
Expand Down Expand Up @@ -100,11 +101,38 @@ public void testGrpc_shutdown() {
}

@Test
public void testHttpJson() {
public void testHttpJson_receiveContent() {
assertThat(echoHttpJson("http-echo?")).isEqualTo("http-echo?");
assertThat(echoHttpJson("http-echo!")).isEqualTo("http-echo!");
}

/*
This tests has the server return an error back as the result.
We use 404 NOT_FOUND Status as that has the same gRPC <-> HttpJson code mapping
(showcase sever has a map that translates the code). The showcase server expects
a gRPC Status Code and the result is the HttpJson's mapped value
*/
@Test
public void testHttpJson_serverResponseError_throwsException() {
StatusCode.Code notFoundStatusCode = StatusCode.Code.NOT_FOUND;
Copy link
Contributor

Choose a reason for hiding this comment

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

You may have already covered this in the javadoc but is there an advantage of using NOT_FOUND over CANCELLED? Mostly wondering if the grpc tests should also be switched to use 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.

I just picked NOT_FOUND instead of CANCELLED since the docs seem to suggest that CANCELLED status is from a user action (https://chromium.googlesource.com/external/github.com/grpc/grpc/+/refs/tags/v1.21.4-pre1/doc/statuscodes.md). Any error code should be fine since we just need the server to respond back with the error.

Copy link
Member

Choose a reason for hiding this comment

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

Whether we choose NOT_FOUND or CANCELLED, let's please make this and testGrpc_serverResponseError_throwsException consistent so we aren't implying a difference in behavior. If we want to test both behaviors, let's do so explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update to CANCELLED.

ApiException exception =
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can make the exception more specific here? To make sure that the test doesn't pass for a false positive case since ApiException can occur for a number of reasons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah changed.

assertThrows(
ApiException.class,
() ->
httpJsonClient.echo(
EchoRequest.newBuilder()
.setError(Status.newBuilder().setCode(notFoundStatusCode.ordinal()).build())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this may not work but can the inner build() call be omitted when we nest builders? i.e. does EchoRequest.newBuilder().setError(Status.newBuilder().setCode(notFoundStatusCode.ordinal())).build() work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it out to a variable

.build()));
assertThat(exception.getStatusCode().getCode()).isEqualTo(notFoundStatusCode);
}

@Test
public void testHttpJson_shutdown() {
assertThat(httpJsonClient.isShutdown()).isFalse();
httpJsonClient.shutdown();
assertThat(httpJsonClient.isShutdown()).isTrue();
}

private String echoGrpc(String value) {
EchoResponse response = grpcClient.echo(EchoRequest.newBuilder().setContent(value).build());
return response.getContent();
Expand Down