Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
test: add showcase test for api-version #2737
Changes from 3 commits
dd1f819
39dcec2
219c8fd
5c7cfca
c9d8ded
f30f325
bc0cf7c
64ac5a7
428f5ce
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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:
sdk-platform-java/gax-java/gax/src/main/java/com/google/api/gax/rpc/ApiClientHeaderProvider.java
Line 45 in 43e62b9
We tried package-private, but showcase needs it as well so I think public makes sense.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
There was a problem hiding this comment.
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:
setHeaderProvider()
, I am thinking adding to conflict resolution as in pr2745To 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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.
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
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 withawaitTermination()
, withclose()
being called first. You're rightclose()
just callsshutdown()
and allshutdown()
does is to prevent new tasks from being scheduled, but still allows currently in-flight tasks to continue. We simply useawaitTermination()
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 timeWe 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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumping this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining. Added.