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

test: add showcase test for api-version #2737

Merged
merged 9 commits into from
May 10, 2024
Merged

test: add showcase test for api-version #2737

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dd1f819
test: add gapic-showcase test to verify api-version is sent correctly…
zhumin8 May 6, 2024
39dcec2
test: update showcase version.
zhumin8 May 6, 2024
219c8fd
fix: fmt.
zhumin8 May 6, 2024
5c7cfca
fix: minor fixes and added todo comment.
zhumin8 May 7, 2024
c9d8ded
fix: expose ApiClientHeaderProvider.API_VERSION_HEADER_KEY as public …
zhumin8 May 7, 2024
f30f325
add additional todo comment.
zhumin8 May 7, 2024
bc0cf7c
test: added test cases for attempting add user api version headers.
zhumin8 May 8, 2024
64ac5a7
test: switch to assertThrows to capture exception.
zhumin8 May 9, 2024
428f5ce
fix: add awaitTermination for test cleanup.
zhumin8 May 10, 2024
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
2 changes: 1 addition & 1 deletion gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public class ApiClientHeaderProvider implements HeaderProvider, Serializable {
private static final long serialVersionUID = -8876627296793342119L;
static final String QUOTA_PROJECT_ID_HEADER_KEY = "x-goog-user-project";

static final String API_VERSION_HEADER_KEY = "x-goog-api-version";
public static final String API_VERSION_HEADER_KEY = "x-goog-api-version";

private final Map<String, String> headers;

Expand Down
2 changes: 1 addition & 1 deletion showcase/gapic-showcase/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
</parent>

<properties>
<gapic-showcase.version>0.33.0</gapic-showcase.version>
<gapic-showcase.version>0.35.0</gapic-showcase.version>
</properties>

<build>
Expand Down
214 changes: 214 additions & 0 deletions ...case/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
/*
* Copyright 2024 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.google.showcase.v1beta1.it;

import static com.google.common.truth.Truth.assertThat;

import com.google.api.gax.httpjson.*;
import com.google.api.gax.rpc.ApiClientHeaderProvider;
import com.google.common.collect.ImmutableList;
import com.google.showcase.v1beta1.*;
import com.google.showcase.v1beta1.it.util.TestClientInitializer;
import io.grpc.*;
import java.util.ArrayList;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;

// TODO: add testing on error responses once feat is implemented in showcase.
// https://github.com/googleapis/gapic-showcase/pull/1456
// TODO: watch for showcase gRPC trailer changes suggested in
// https://github.com/googleapis/gapic-showcase/pull/1509#issuecomment-2089147103
public class ITApiVersionHeaders {
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
private static final String HTTP_RESPONSE_HEADER_STRING =
"x-showcase-request-" + ApiClientHeaderProvider.API_VERSION_HEADER_KEY;
private static final Metadata.Key<String> API_VERSION_HEADER_KEY =
Metadata.Key.of(
ApiClientHeaderProvider.API_VERSION_HEADER_KEY, Metadata.ASCII_STRING_MARSHALLER);

private static final String EXPECTED_ECHO_API_VERSION = "v1_20240408";

// Implement a client interceptor to retrieve the trailing metadata from response.
private static class GrpcCapturingClientInterceptor implements ClientInterceptor {
private Metadata metadata;

@Override
public <RequestT, ResponseT> ClientCall<RequestT, ResponseT> interceptCall(
MethodDescriptor<RequestT, ResponseT> method, final CallOptions callOptions, Channel next) {
ClientCall<RequestT, ResponseT> call = next.newCall(method, callOptions);
return new ForwardingClientCall.SimpleForwardingClientCall<RequestT, ResponseT>(call) {
@Override
public void start(Listener<ResponseT> responseListener, Metadata headers) {
Listener<ResponseT> wrappedListener =
new SimpleForwardingClientCallListener<ResponseT>(responseListener) {
@Override
public void onClose(Status status, Metadata trailers) {
if (status.isOk()) {
metadata = trailers;
}
super.onClose(status, trailers);
}
};

super.start(wrappedListener, headers);
}
};
}
lqiu96 marked this conversation as resolved.
Show resolved Hide resolved
}

private static class SimpleForwardingClientCallListener<RespT>
extends ClientCall.Listener<RespT> {
private final ClientCall.Listener<RespT> delegate;

SimpleForwardingClientCallListener(ClientCall.Listener<RespT> delegate) {
this.delegate = delegate;
}

@Override
public void onHeaders(Metadata headers) {
delegate.onHeaders(headers);
}

@Override
public void onMessage(RespT message) {
delegate.onMessage(message);
}

@Override
public void onClose(Status status, Metadata trailers) {
delegate.onClose(status, trailers);
}

@Override
public void onReady() {
delegate.onReady();
}
}
// Implement a client interceptor to retrieve the response headers
private static class HttpJsonCapturingClientInterceptor implements HttpJsonClientInterceptor {
private HttpJsonMetadata metadata;

@Override
public <RequestT, ResponseT> HttpJsonClientCall<RequestT, ResponseT> interceptCall(
ApiMethodDescriptor<RequestT, ResponseT> method,
HttpJsonCallOptions callOptions,
HttpJsonChannel next) {
HttpJsonClientCall<RequestT, ResponseT> call = next.newCall(method, callOptions);
return new ForwardingHttpJsonClientCall.SimpleForwardingHttpJsonClientCall<
RequestT, ResponseT>(call) {
@Override
public void start(Listener<ResponseT> responseListener, HttpJsonMetadata requestHeaders) {
Listener<ResponseT> forwardingResponseListener =
new ForwardingHttpJsonClientCallListener.SimpleForwardingHttpJsonClientCallListener<
ResponseT>(responseListener) {
@Override
public void onHeaders(HttpJsonMetadata responseHeaders) {
metadata = responseHeaders;
super.onHeaders(responseHeaders);
}

@Override
public void onMessage(ResponseT message) {
super.onMessage(message);
}

@Override
public void onClose(int statusCode, HttpJsonMetadata trailers) {
super.onClose(statusCode, trailers);
}
};

super.start(forwardingResponseListener, requestHeaders);
}
};
}
}

private HttpJsonCapturingClientInterceptor httpJsonInterceptor;
private GrpcCapturingClientInterceptor grpcInterceptor;
private HttpJsonCapturingClientInterceptor httpJsonComplianceInterceptor;
private GrpcCapturingClientInterceptor grpcComplianceInterceptor;
private EchoClient grpcClient;
private EchoClient httpJsonClient;
private ComplianceClient grpcComplianceClient;
private ComplianceClient httpJsonComplianceClient;

@Before
public void createClients() throws Exception {
// Create gRPC Interceptor and Client
grpcInterceptor = new GrpcCapturingClientInterceptor();
grpcClient = TestClientInitializer.createGrpcEchoClient(ImmutableList.of(grpcInterceptor));

// Create HttpJson Interceptor and Client
httpJsonInterceptor = new HttpJsonCapturingClientInterceptor();
httpJsonClient =
TestClientInitializer.createHttpJsonEchoClient(ImmutableList.of(httpJsonInterceptor));

// Create gRPC ComplianceClient and Interceptor
// Creating a compliance client to test case where api version is not set
grpcComplianceInterceptor = new GrpcCapturingClientInterceptor();
grpcComplianceClient =
TestClientInitializer.createGrpcComplianceClient(
ImmutableList.of(grpcComplianceInterceptor));

// Create HttpJson ComplianceClient and Interceptor
httpJsonComplianceInterceptor = new HttpJsonCapturingClientInterceptor();
httpJsonComplianceClient =
TestClientInitializer.createHttpJsonComplianceClient(
ImmutableList.of(httpJsonComplianceInterceptor));
Comment on lines +172 to +183
Copy link
Contributor

@lqiu96 lqiu96 May 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Perhaps we keep one type of client where possible (i.e. Echo for all tests cases). We can try and override the ApiClientHeaderProvider so it unsets the showcase apiVersion value and if it doesn't work, I'm fine with 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.

In current setup, we could override with .setHeaderProvider() when creating the test client. I am reluctant to do so though.

  • I feel like showcase integration testing should test expected behavior of un-modified service behaviors E2E.
  • As we discussed in doc comment last time, I am inclined to not allow user override this "x-goog-api-version" header with client libraries. (Will raise a separate pr) In this case, "unsets the showcase apiVersion value" should not work.

Copy link
Contributor

@lqiu96 lqiu96 May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like showcase integration testing should test expected behavior of un-modified service behaviors E2E.

IMO, it shouldn't just be limited to un-modified service behaviors. We should try to account for customizations where we can (i.e. If new features are added or showcase has new functionality). We're not in state where every modification/ feature is covered, but I think we should strive to get as close as possible. There are going to be certain spots we miss and new edge cases that are discovered, but we should try to backfill/ add them in as we discover them.

I am inclined to not allow user override this "x-goog-api-version" header with client libraries.

Makes sense. Let's get some insight from the core team to see if this is prioritized (and if not, we can always prioritize this for Java clients ourselves). The public setter method is pubic as of now and removing will be challenging/ impossible. We can explore some options to see how we want to proceed.

Being that removing may be challenging/impossible, I think it may make sense to include both Echo and Compliance clients in then since we test for the behavior of add custom headers to both clients that that have an auto generated ApiVersion vs clients that don't have this generated.

i.e. I believe as of now, Echo with custom headers set should have a conflict error message and Compliance with custom headers should not have a conflict error message.

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 agree with most of these, but wanted to clarify a few:

  • This ITApiVersionHeaders.java tests for behavior when client has an auto generated ApiVersion (Echo) and gets correctly send via header on requests, and when client don't have this generated (Compliance) then no such header sent. User header are not covered.
  • To test behavior of adding user header to both clients also makes sense, I will add based on current behavior. This may change in the future. In fact, I have a todo item in feat: throw exception when user attempt to override api-version header. #2745 for it if we decide to go with this approach.
  • "The public setter method" >> I assume you are referring to this setter? If so, this is only one HeaderProvider the client library code uses, users could use other HeaderProvider. But to guard users to not override "x-goog-api-version" header with setHeaderProvider(), I am thinking adding to conflict resolution as in pr2745
  • "Let's get some insight from the core team if this is prioritized " >> Other than comment on proposal doc, I've started a thread on the aip here

To summarize, I am keeping both clients in this test to cover scenario of service opted-in for api-version and not. Also adding tests for setting custom api version headers for these clients, these behavior may change via a separate pr.

Copy link
Contributor

@lqiu96 lqiu96 May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This ITApiVersionHeaders.java tests for behavior when client has an auto generated ApiVersion (Echo) and gets correctly send via header on requests, and when client don't have this generated (Compliance) then no such header sent. User header are not covered.

Yep, I think that's good! The test cases do look good. I don't mean that we should try to cover modifying every user header, but rather the relevant one to the test (api-version header).

In fact, I have a todo item in #2745

Let's have an internal discussion. I do prefer this behavior but I believe this may put us at a "behavior breaking change" which is something that we try not to do (if possible). I think this may be a case where we can argue for it given that it's a relatively new feature.

Also adding tests for setting custom api version headers for these clients, these behavior may change via a separate pr.

I'll do another round of review on this PR. No worries about adding modifications to the header in this PR if you haven't already. I believe it's already a pretty niche case and it can be added and/or tested based on what happens to #2745

}

@After
public void destroyClient() {
grpcClient.close();
httpJsonClient.close();
grpcComplianceClient.close();
httpJsonComplianceClient.close();
Comment on lines +188 to +191
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add awaitTermination() here after the .close() calls?

Copy link
Contributor Author

@zhumin8 zhumin8 May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

IIUC, .close() is same as .shutdown() (code). We should .shutdown() a client or .awaitTermination(), not both, and the main difference is that .awaitTermination() has timeout (comment here).
However, if we want to prevent a test hanging, this may not work, as these are called within @After and I believe if a test hangs and does not finish, the @After method will not be executed. An alternative is to add @Test(timeout = ..) to tests. But I wonder if you have a specific concern about these tests? Or do you prefer to implement timeouts for all integration tests as precautious?

Please correct me if I am assuming anything wrong?

Copy link
Contributor

@lqiu96 lqiu96 May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We typically use close() in conjunction with awaitTermination(), with close() being called first. You're right close() just calls shutdown() and all shutdown() does is to prevent new tasks from being scheduled, but still allows currently in-flight tasks to continue. We simply use awaitTermination() to allow the current in-flight tasks to finish (it blocks until they are done or until the timeout has been reached). Most showcase calls should finish relatively quick (we try to keep our tests quick) so we won't be awaiting for a long time

We had previously seen some grpc-java warnings about managed channels still being allocated without trying to wait to the tasks to terminated. i.e something like:

io.grpc.internal.ManagedChannelImpl$ManagedChannelReference cleanQueue
SEVERE: *~*~*~ Channel io.grpc.internal.ManagedChannelImpl-2096 for target 127.0.0.1:8771 was not shutdown properly!!! ~*~*~*
    Make sure to call shutdown()/shutdownNow() and awaitTermination().
java.lang.RuntimeException: ManagedChannel allocation site

Copy link
Contributor

Choose a reason for hiding this comment

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

Bumping 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.

Thanks for explaining. Added.

}

@Test
public void testGrpc_matchesApiVersion() {
grpcClient.echo(EchoRequest.newBuilder().build());
String headerValue = grpcInterceptor.metadata.get(API_VERSION_HEADER_KEY);
assertThat(headerValue).isEqualTo(EXPECTED_ECHO_API_VERSION);
}

@Test
public void testHttpJson_matchesHeaderName() {
httpJsonClient.echo(EchoRequest.newBuilder().build());
ArrayList headerValues =
(ArrayList) httpJsonInterceptor.metadata.getHeaders().get(HTTP_RESPONSE_HEADER_STRING);
String headerValue = (String) headerValues.get(0);
assertThat(headerValue).isEqualTo(EXPECTED_ECHO_API_VERSION);
}

@Test
public void testGrpc_noApiVersion() {
RepeatRequest request =
RepeatRequest.newBuilder().setInfo(ComplianceData.newBuilder().setFString("test")).build();
grpcComplianceClient.repeatDataSimplePath(request);
assertThat(API_VERSION_HEADER_KEY).isNotIn(grpcComplianceInterceptor.metadata.keys());
}

@Test
public void testHttpJson_noApiVersion() {
RepeatRequest request =
RepeatRequest.newBuilder().setInfo(ComplianceData.newBuilder().setFString("test")).build();
httpJsonComplianceClient.repeatDataSimplePath(request);
assertThat(API_VERSION_HEADER_KEY)
.isNotIn(httpJsonComplianceInterceptor.metadata.getHeaders().keySet());
}
}
Loading