-
Notifications
You must be signed in to change notification settings - Fork 52
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
chore: Add Showcase Http Annotations Test #1568
Conversation
I believe I need to include the |
167df68
to
f06161c
Compare
362276f
to
13e0cdf
Compare
ComplianceSettings.newHttpJsonBuilder() | ||
.setCredentialsProvider(NoCredentialsProvider.create()) | ||
.setTransportChannelProvider( | ||
EchoSettings.defaultHttpJsonTransportProviderBuilder() |
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.
This should be ComplianceSettings
instead of EchoSettings
? Surprised it worked 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.
Ah yep, updated!
.gitignore
Outdated
@@ -16,3 +16,5 @@ target/ | |||
.vscode/settings.json | |||
|
|||
*.iml | |||
|
|||
showcase/gapic-showcase/src/test/resources/compliance_suite.json |
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 think we should check it in if it is required for testing.
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.
Ok, I can add it in.
} | ||
|
||
@Test | ||
public void testCompliance() { |
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'm actually not sure what we are verifying, can you make it clear with a better name and/or comments?
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.
Added some comments and updated the test function's name.
for (String rpcName : validRpcList) { | ||
for (RepeatRequest repeatRequest : compliancegroup.getRequestsList()) { | ||
ComplianceData expectedData = repeatRequest.getInfo(); | ||
RepeatResponse response = complianceValidRpcMap.get(rpcName).apply(repeatRequest); |
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 like how you've structured this with the map.
Minor suggestion: I got caught on this line having to figure out what was going on. Consider the slightly more explicit:
for (String rpcName : validRpcList) {
Function<Request, Response> rpc = complianceValidRpcMap.get(rpcName);
for (Request request : getRequestsList()) {
Response response = rpc.apply(request);
...
}
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.
Sure, will do!
for (ComplianceGroup compliancegroup : complianceSuite.getGroupList()) { | ||
List<String> validRpcList = | ||
compliancegroup.getRpcsList().stream() | ||
.filter(complianceValidRpcMap::containsKey) | ||
.collect(Collectors.toList()); | ||
for (String rpcName : validRpcList) { | ||
Function<RepeatRequest, RepeatResponse> rpc = complianceValidRpcMap.get(rpcName); | ||
for (RepeatRequest repeatRequest : compliancegroup.getRequestsList()) { | ||
ComplianceData expectedData = repeatRequest.getInfo(); | ||
RepeatResponse response = rpc.apply(repeatRequest); | ||
assertThat(response.getRequest().getInfo()).isEqualTo(expectedData); |
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.
Could a helper method help reduce the number of nested for loops needed here? Reducing the amount of logic done in a test method can help with debugging in the future and prevent the test itself from developing bugs.
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.
Would it make more sense to just have each ComplianceGroup as its own test then? It could remove the outer most for loop if that's cleaner.
For each group, we test the product of rpcName and requestList.
Or did you mean something like:
@Test
public void testHttpAnnotations() {
for (ComplianceGroup compliancegroup : complianceSuite.getGroupList()) {
List<String> validRpcList =
compliancegroup.getRpcsList().stream()
.filter(complianceValidRpcMap::containsKey)
.collect(Collectors.toList());
helper(compliancegroup, validRpcList);
}
}
private void helper(ComplianceGroup compliancegroup, List<String> validRpcList) {
for (String rpcName : validRpcList) {
Function<RepeatRequest, RepeatResponse> rpc = complianceValidRpcMap.get(rpcName);
for (RepeatRequest repeatRequest : compliancegroup.getRequestsList()) {
ComplianceData expectedData = repeatRequest.getInfo();
RepeatResponse response = rpc.apply(repeatRequest);
assertThat(response.getRequest().getInfo()).isEqualTo(expectedData);
}
}
}
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 do like the idea of splitting the test based on the ComplianceGroup since it would reduce cognitive load needed in understanding the behavior the test is trying to verify. Also, in the event that a test fails it will be easier to determine where/what behavior is failing.
Perhaps something similar to how ComplianceClientTest is structured.
Will also let @burkedavison and @blakeli0 weigh in.
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.
Sure, makes sense. I've split them by ComplianceGroup (unless there is also hesitancy about this). The groups are split by behavior (i.e. bindings, weird/extreme values, templating, etc.) so a failure for a test case in testing one group should alert us to the problematic behavior.
for (ComplianceGroup compliancegroup : complianceSuite.getGroupList()) { | ||
List<String> validRpcList = | ||
compliancegroup.getRpcsList().stream() | ||
.filter(complianceValidRpcMap::containsKey) | ||
.collect(Collectors.toList()); | ||
for (String rpcName : validRpcList) { | ||
Function<RepeatRequest, RepeatResponse> rpc = complianceValidRpcMap.get(rpcName); | ||
for (RepeatRequest repeatRequest : compliancegroup.getRequestsList()) { | ||
ComplianceData expectedData = repeatRequest.getInfo(); | ||
RepeatResponse response = rpc.apply(repeatRequest); | ||
assertThat(response.getRequest().getInfo()).isEqualTo(expectedData); |
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.
This may require a big change but was wondering - would it be possible to explicitly specify the expected value is without computing it?
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.
As written, the compliance_suite.json is defined in a language-neutral way and can be (should?*) be owned by the gapic-showcase repo. If we hardcode expected values in the java test that duplicates the expected input, I would think this would eliminate our ability to have a common test suite defined elsewhere.
(*) Is there a way to get the compliance_suite.json from the gapic-showcase dependency rather than copying it into this repo?
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'm a bit lost here (I might be misunderstanding the comments above). Is this not specifying the expected value: ComplianceData expectedData = repeatRequest.getInfo();
? The test expects that showcase is able to return the Request's info properly back.
Is there a way to get the compliance_suite.json from the gapic-showcase dependency rather than copying it into this repo?
If I'm understand this correctly, the concern is that we may not be pulling in the latest/ specified json file version?
Right now, the json file is downloaded in the generate-test-resources
maven lifecycle every time it's run. The file is versioned and downloads the from the gapic-showcase version in the pom file. It should be updated when we upgrade the gapic-showcase version and the file (with any changes) would be updated in this repo as well.
I had the json file ignored by git, but wasn't sure if it mattered. Since it's downloaded every time showcase is run, it'll always have the version we specify.
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.
Great - this is perfect.
I vote to .gitignore it since it's a "generated" file to avoid confusion over the source of truth; but the main concern you've already covered.
Thank you.
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.
Sounds good. Going to loop in @blakeli0 again if he has some other concerns that might be missed. If not, I can add the file back into the .gitignore
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.
re comment on hard-coding the expected value: Thanks for the clarification! I didn't take this aspect into account: I would think this would eliminate our ability to have a common test suite defined elsewhere.
Given this reasoning, computing the value instead of hard-cording looks like the way to go.
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 vote to .gitignore it since it's a "generated" file to avoid confusion over the source of truth; but the main concern you've already covered.
I feel it might be better to include this compliance_suite.json
file in our repo and maybe the protos as well, as it would be easier to read and troubleshoot the tests. Even though they are "generated", but I consider them as part of the whole gapic-showcase test suite, just like the "generated" gapic client libraries, which are already included in our repo.
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.
Sure, added the json file in.
showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITHttpAnnotation.java
Outdated
Show resolved
Hide resolved
@blakeli0 @burkedavison @mpeddada1 Could you all give a re-review whenever you all get a chance? |
showcase/gapic-showcase/src/test/java/com/google/showcase/v1beta1/it/ITHttpAnnotation.java
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,119 @@ | |||
package com.google.showcase.v1beta1.it; |
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 may be missing a copyright header for this file?
Hm I wonder why our checkstyle didn't fail.
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.
Added the copyright header. I'll need to check why it didn't run for the test files.
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 @lqiu96!
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed! |
[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Let me know if the changes look fine @mpeddada1 @blakeli0 |
Thank you for opening a Pull Request! For general contributing guidelines, please refer to contributing guide
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Part of #1439 ☕️
No longer blocked as #1522 has been merged in.