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

test: add showcase test for api-version #2737

Merged
merged 9 commits into from
May 10, 2024
Merged

Conversation

zhumin8
Copy link
Contributor

@zhumin8 zhumin8 commented May 6, 2024

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

@zhumin8 zhumin8 requested a review from a team as a code owner May 6, 2024 10:04
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label May 6, 2024
@zhumin8 zhumin8 requested a review from lqiu96 May 6, 2024 10:56

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

@lqiu96 lqiu96 May 6, 2024

Choose a reason for hiding this comment

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

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

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.

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 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Comment on lines +154 to +165
// 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));
Copy link
Contributor

@lqiu96 lqiu96 May 6, 2024

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

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

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

Comment on lines +176 to +179
grpcClient.close();
httpJsonClient.close();
grpcComplianceClient.close();
httpJsonComplianceClient.close();
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

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

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.

@product-auto-label 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
.isNotIn(httpJsonComplianceInterceptor.metadata.getHeaders().keySet());
}

@Test(expected = IllegalArgumentException.class)
Copy link
Contributor

@lqiu96 lqiu96 May 8, 2024

Choose a reason for hiding this comment

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

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

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


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

Choose a reason for hiding this comment

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

What is this line doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines 239 to 255
@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();
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the two comments above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same above.

@lqiu96
Copy link
Contributor

lqiu96 commented May 8, 2024

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

Copy link
Contributor

@lqiu96 lqiu96 left a comment

Choose a reason for hiding this comment

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

LGTM. Small addition for @After if you could resolve

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

Quality Gate Passed 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

Copy link

Quality Gate Passed 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 zhumin8 merged commit e1e1fb6 into main May 10, 2024
44 of 48 checks passed
@zhumin8 zhumin8 deleted the api-version-showcase-test branch May 10, 2024 10:24
lqiu96 added a commit that referenced this pull request May 20, 2024
fixes #2728, attempt to remove Junit 4 support after migration.
Other than POM dependency migrate, changes include:
- package name changes
- Junit 5 syntax upgrades, e.g. `@Before` --> `@BeforeEach`, Replace
assertion methods
- remove public modifier on tests and test classes.
- Refactor JUnit 4 TemporaryFolder `@Rule` in
[ITGdch.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-6ae7755a0b038e1a2febae2d27e36c762f6751b8c7db577421667069399884b4)
to JUnit 5 `@TempDir`
- Replace `@Test(timeout = 15000L)` in
[ITClientShutdown.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-70d1df57471178a7a63302f82e4a4855ffbbd642ea67d92d501bd1f7008957ca)
with `@Timeout(15)`
- Update `@RunWith(Parameterized.class)` test in
[ITHttpAnnotation.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-03d420650ecc9fe78ad4887761043c4fdceaa978f464ce30cfc4ed5f8be9b64d)
to `@ParameterizedTest` with `@MethodSource("data")`

~~Note: #2737 creates a new test class with JUnit4 syntax. Depending on
merging order, I will either update in this pr, or #2737.~~ Updated.


Due to truth library depending on junit 4 ([see
issue](google/truth#333)), junit 4 cannot be
completely removed, or will encounter `java.lang.ClassNotFoundException:
org.junit.runner.notification.RunListener` running tests with maven
surefire. To keep things cleaner, excluding the implicitly junit brought
in from truth and `junit-vintage-engine`. We could also do the reverse,
and make a comment if that's prefered.

---------

Co-authored-by: Burke Davison <40617934+burkedavison@users.noreply.github.com>
Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
lqiu96 pushed a commit that referenced this pull request May 22, 2024
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).
lqiu96 added a commit that referenced this pull request May 22, 2024
fixes #2728, attempt to remove Junit 4 support after migration.
Other than POM dependency migrate, changes include:
- package name changes
- Junit 5 syntax upgrades, e.g. `@Before` --> `@BeforeEach`, Replace
assertion methods
- remove public modifier on tests and test classes.
- Refactor JUnit 4 TemporaryFolder `@Rule` in
[ITGdch.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-6ae7755a0b038e1a2febae2d27e36c762f6751b8c7db577421667069399884b4)
to JUnit 5 `@TempDir`
- Replace `@Test(timeout = 15000L)` in
[ITClientShutdown.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-70d1df57471178a7a63302f82e4a4855ffbbd642ea67d92d501bd1f7008957ca)
with `@Timeout(15)`
- Update `@RunWith(Parameterized.class)` test in
[ITHttpAnnotation.java](https://github.com/googleapis/sdk-platform-java/pull/2757/files#diff-03d420650ecc9fe78ad4887761043c4fdceaa978f464ce30cfc4ed5f8be9b64d)
to `@ParameterizedTest` with `@MethodSource("data")`

~~Note: #2737 creates a new test class with JUnit4 syntax. Depending on
merging order, I will either update in this pr, or #2737.~~ Updated.


Due to truth library depending on junit 4 ([see
issue](google/truth#333)), junit 4 cannot be
completely removed, or will encounter `java.lang.ClassNotFoundException:
org.junit.runner.notification.RunListener` running tests with maven
surefire. To keep things cleaner, excluding the implicitly junit brought
in from truth and `junit-vintage-engine`. We could also do the reverse,
and make a comment if that's prefered.

---------

Co-authored-by: Burke Davison <40617934+burkedavison@users.noreply.github.com>
Co-authored-by: Lawrence Qiu <lawrenceqiu@google.com>
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.

2 participants