test: add showcase test for api-version #2737

zhumin8 · 2024-05-06T10:04:01Z

This pr updates gapic-showcase version to 0.35.0 and adds showcase tests to verify behavior of api version headers being emitted (changes in #2630 and #2671).

… via headers.

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java

lqiu96 · 2024-05-06T20:59:38Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java

+
+public class ITApiVersionHeaders {
+  private static final String SPLIT_TOKEN = "&";
+  private static final String API_VERSION_HEADER_STRING = "x-goog-api-version";


nit: Personal opinion is that we can consider making the constant public:

sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java

Line 45 in 43e62b9

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

so that we don't redefine it multiple spots.

We tried package-private, but showcase needs it as well so I think public makes sense.

I tend to agree. But I was not 100% on if this a breaking change to gax. Even though we are exposing a new public field, this is a final field and no breaking to existing usage, so I suppose it's okay?

It was package-private so we can change it since no one else should have access to it.

It is now public field, but I think that's fine since we're doing it so that this constant isn't re-defined in multiple spots.

lqiu96 · 2024-05-06T21:02:38Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java

+    // 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));


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.

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.

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.

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.

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

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java

…for testing.

lqiu96 · 2024-05-07T18:30:36Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java

+    grpcClient.close();
+    httpJsonClient.close();
+    grpcComplianceClient.close();
+    httpJsonComplianceClient.close();


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

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?

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

Bumping this.

Thanks for explaining. Added.

lqiu96 · 2024-05-08T17:22:43Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java

+        .isNotIn(httpJsonComplianceInterceptor.metadata.getHeaders().keySet());
+  }
+
+  @Test(expected = IllegalArgumentException.class)


Can you use assertThrows() instead of @Test(expected = ...)? This will help us when we do our migration to JUnit5. Something like:

sdk-platform-java/showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITUnaryCallable.java

Lines 70 to 71 in 3611e8f

CancelledException exception =

assertThrows(CancelledException.class, () -> grpcClient.echo(requestWithServerError));

lqiu96 · 2024-05-08T17:23:14Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java

+
+    try (EchoClient echo =
+        EchoClient.create(EchoSettings.create((EchoStubSettings) stubSettings))) {
+      assertThat(true).isFalse();


What is this line doing?

Oh, nvm.
I was not aware the assertThrows() as you mentioned in above comment is available in Junit 4 too, and was trying to be more precise on where exception is thrown. Switching to assertThrows() approach.

lqiu96 · 2024-05-08T17:23:33Z

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java

+  @Test(expected = IllegalArgumentException.class)
+  public void testHttpJsonEcho_userApiVersionThrowsException() throws IOException {
+    StubSettings stubSettings =
+        httpJsonClient
+            .getSettings()
+            .getStubSettings()
+            .toBuilder()
+            .setHeaderProvider(
+                FixedHeaderProvider.create(
+                    ApiClientHeaderProvider.API_VERSION_HEADER_KEY, CUSTOM_API_VERSION))
+            .build();
+
+    try (EchoClient echo =
+        EchoClient.create(EchoSettings.create((EchoStubSettings) stubSettings))) {
+      assertThat(true).isFalse();
+    }
+  }


Same as the two comments above.

same above.

lqiu96 · 2024-05-08T17:25:30Z

LGTM. Just a few nits/ questions, but I'll approve once those are addressed.

lqiu96

LGTM. Small addition for @After if you could resolve

sonarcloud · 2024-05-10T09:58:34Z

Quality Gate passed for 'gapic-generator-java-root'

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarcloud · 2024-05-10T10:01:43Z

Quality Gate passed for 'java_showcase_integration_tests'

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

zhumin8 added 2 commits May 6, 2024 09:46

test: add gapic-showcase test to verify api-version is sent correctly…

dd1f819

… via headers.

test: update showcase version.

39dcec2

zhumin8 requested a review from a team as a code owner May 6, 2024 10:04

product-auto-label bot added the size: m Pull request size is medium. label May 6, 2024

fix: fmt.

219c8fd

zhumin8 requested a review from lqiu96 May 6, 2024 10:56

lqiu96 reviewed May 6, 2024

View reviewed changes

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java Show resolved Hide resolved

lqiu96 reviewed May 6, 2024

View reviewed changes

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java Show resolved Hide resolved

lqiu96 reviewed May 6, 2024

View reviewed changes

showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITApiVersionHeaders.java Outdated Show resolved Hide resolved

zhumin8 mentioned this pull request May 7, 2024

showcase test: refactor and migrate client interceptors to TestClientInitializer #2743

Open

zhumin8 added 3 commits May 7, 2024 02:52

fix: minor fixes and added todo comment.

5c7cfca

fix: expose ApiClientHeaderProvider.API_VERSION_HEADER_KEY as public …

c9d8ded

…for testing.

add additional todo comment.

f30f325

zhumin8 mentioned this pull request May 7, 2024

feat: throw exception when user attempt to override api-version header. #2745

Draft

lqiu96 reviewed May 7, 2024

View reviewed changes

test: added test cases for attempting add user api version headers.

bc0cf7c

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels May 8, 2024

zhumin8 mentioned this pull request May 8, 2024

feat: Update Gapic generator and Gax to emit api-versioning via header #2671

Merged

lqiu96 reviewed May 8, 2024

View reviewed changes

test: switch to assertThrows to capture exception.

64ac5a7

zhumin8 mentioned this pull request May 9, 2024

chore: Migrate Junit 4 to Junit 5 for showcase #2757

Open

lqiu96 approved these changes May 9, 2024

View reviewed changes

fix: add awaitTermination for test cleanup.

428f5ce

zhumin8 enabled auto-merge (squash) May 10, 2024 09:51

zhumin8 merged commit e1e1fb6 into main May 10, 2024
44 of 48 checks passed

zhumin8 deleted the api-version-showcase-test branch May 10, 2024 10:24

diegomarquezp mentioned this pull request May 17, 2024

test: restore showcase autogen IT GoogleCloudPlatform/spring-cloud-gcp#2895

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add showcase test for api-version #2737

test: add showcase test for api-version #2737

zhumin8 commented May 6, 2024

lqiu96 May 6, 2024 •

edited

zhumin8 May 7, 2024

lqiu96 May 7, 2024

lqiu96 May 6, 2024 •

edited

zhumin8 May 7, 2024

lqiu96 May 7, 2024 •

edited

zhumin8 May 8, 2024

lqiu96 May 8, 2024 •

edited

lqiu96 May 7, 2024

zhumin8 May 8, 2024 •

edited

lqiu96 May 8, 2024 •

edited

lqiu96 May 9, 2024

zhumin8 May 10, 2024

lqiu96 May 8, 2024 •

edited

lqiu96 May 8, 2024

zhumin8 May 9, 2024

lqiu96 May 8, 2024

zhumin8 May 9, 2024

lqiu96 commented May 8, 2024

lqiu96 left a comment

sonarcloud bot commented May 10, 2024

sonarcloud bot commented May 10, 2024

	CancelledException exception =
	assertThrows(CancelledException.class, () -> grpcClient.echo(requestWithServerError));

test: add showcase test for api-version #2737

test: add showcase test for api-version #2737

Conversation

zhumin8 commented May 6, 2024

lqiu96 May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 May 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhumin8 May 8, 2024 • edited

Choose a reason for hiding this comment

lqiu96 May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 commented May 8, 2024

lqiu96 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 10, 2024

Quality Gate passed for 'gapic-generator-java-root'

sonarcloud bot commented May 10, 2024

Quality Gate passed for 'java_showcase_integration_tests'

lqiu96 May 6, 2024 •

edited

lqiu96 May 6, 2024 •

edited

lqiu96 May 7, 2024 •

edited

lqiu96 May 8, 2024 •

edited

zhumin8 May 8, 2024 •

edited

lqiu96 May 8, 2024 •

edited

lqiu96 May 8, 2024 •

edited