chore: Add Showcase Http Annotations Test #1568

lqiu96 · 2023-03-28T14:21:59Z

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:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Part of #1439 ☕️

No longer blocked as #1522 has been merged in.

lqiu96 · 2023-03-28T15:42:33Z

  JUnit Vintage:ITHttpAnnotation:testCompliance
    MethodSource [className = 'com.google.showcase.v1beta1.it.ITHttpAnnotation', methodName = 'testCompliance', methodParameterTypes = '']
    => org.opentest4j.MultipleFailuresError: Multiple Failures (2 failures)
	java.io.FileNotFoundException: src/test/resources/compliance_suite.json (No such file or directory)
	java.lang.NullPointerException: <no message>

I believe I need to include the compliance_suite.json file in the native build.

blakeli0 · 2023-03-29T13:31:27Z

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

+        ComplianceSettings.newHttpJsonBuilder()
+            .setCredentialsProvider(NoCredentialsProvider.create())
+            .setTransportChannelProvider(
+                EchoSettings.defaultHttpJsonTransportProviderBuilder()


This should be ComplianceSettings instead of EchoSettings? Surprised it worked though.

Ah yep, updated!

blakeli0 · 2023-03-29T13:42:55Z

.gitignore

@@ -16,3 +16,5 @@ target/
 .vscode/settings.json

 *.iml
+
+showcase/gapic-showcase/src/test/resources/compliance_suite.json


I think we should check it in if it is required for testing.

Ok, I can add it in.

blakeli0 · 2023-03-29T13:45:40Z

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

+  }
+
+  @Test
+  public void testCompliance() {


I'm actually not sure what we are verifying, can you make it clear with a better name and/or comments?

Added some comments and updated the test function's name.

burkedavison · 2023-03-29T14:26:02Z

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

+      for (String rpcName : validRpcList) {
+        for (RepeatRequest repeatRequest : compliancegroup.getRequestsList()) {
+          ComplianceData expectedData = repeatRequest.getInfo();
+          RepeatResponse response = complianceValidRpcMap.get(rpcName).apply(repeatRequest);


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

Sure, will do!

lqiu96 · 2023-03-29T16:28:38Z

@blakeli0 From our discussion earlier, I think we could split it up by ComplianceGroup and have a @test function for each group. Each group would be testing features like values and bindings against a series of RPCs. WDYT?

mpeddada1 · 2023-03-29T17:35:15Z

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

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


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.

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); } } }

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.

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.

mpeddada1 · 2023-03-29T17:37:34Z

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

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


This may require a big change but was wondering - would it be possible to explicitly specify the expected value is without computing it?

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?

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.

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.

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

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.

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.

Sure, added the json file in.

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

lqiu96 · 2023-03-30T19:30:27Z

@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

mpeddada1 · 2023-03-31T17:24:07Z

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

@@ -0,0 +1,119 @@
+package com.google.showcase.v1beta1.it;


We may be missing a copyright header for this file?

Hm I wonder why our checkstyle didn't fail.

Added the copyright header. I'll need to check why it didn't run for the test files.

Thanks @lqiu96!

sonarcloud · 2023-03-31T19:12:48Z

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

sonarcloud · 2023-03-31T19:17:00Z

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

sonarcloud · 2023-03-31T19:22:41Z

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

lqiu96 · 2023-03-31T19:39:55Z

Let me know if the changes look fine @mpeddada1 @blakeli0

product-auto-label bot added the size: m Pull request size is medium. label Mar 28, 2023

lqiu96 requested review from blakeli0, burkedavison and mpeddada1 March 28, 2023 14:57

lqiu96 marked this pull request as ready for review March 28, 2023 14:58

lqiu96 requested a review from a team as a code owner March 28, 2023 14:58

lqiu96 force-pushed the main-showcase_compliance branch from 167df68 to f06161c Compare March 28, 2023 16:11

lqiu96 mentioned this pull request Mar 28, 2023

REGAPIC Showcase Coverage #1439

Closed

27 tasks

lqiu96 added 5 commits March 28, 2023 15:51

chore: Add Showcase Compliance Suite

ef36868

chore: Download compliance_suite.json when running showcase ITs

fdbb116

chore: Rename ITCompliance file to ITHttpAnnotation

db555b3

chore: Update HttpAnnotation class

c08ee83

chore: Update the compliance suite

13e0cdf

lqiu96 force-pushed the main-showcase_compliance branch from 362276f to 13e0cdf Compare March 28, 2023 19:52

lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 28, 2023

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 28, 2023

blakeli0 reviewed Mar 29, 2023

View reviewed changes

chore: Update PR comments

118aa11

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Mar 29, 2023

burkedavison reviewed Mar 29, 2023

View reviewed changes

chore: Update PR comments

0a7a1fb

lqiu96 closed this Mar 29, 2023

lqiu96 reopened this Mar 29, 2023

mpeddada1 reviewed Mar 29, 2023

View reviewed changes

chore: Split each group into a separate test

c58bb65

chore: Add in gitignore

a5bc209

product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Mar 29, 2023

lqiu96 added 2 commits March 29, 2023 19:19

chore: Restructure to use parameters

b0845c1

chore: Cleanup the files

e5446ca

burkedavison approved these changes Mar 30, 2023

View reviewed changes

mpeddada1 reviewed Mar 31, 2023

View reviewed changes

lqiu96 added 2 commits March 31, 2023 15:04

chore: Fix pr comments

fecfeb5

chore: Add json file

23d4d00

product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Mar 31, 2023

mpeddada1 approved these changes Mar 31, 2023

View reviewed changes

lqiu96 merged commit d14875a into main Mar 31, 2023
18 checks passed

lqiu96 deleted the main-showcase_compliance branch March 31, 2023 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Add Showcase Http Annotations Test #1568

chore: Add Showcase Http Annotations Test #1568

lqiu96 commented Mar 28, 2023 •

edited

lqiu96 commented Mar 28, 2023

blakeli0 Mar 29, 2023

lqiu96 Mar 29, 2023

blakeli0 Mar 29, 2023

lqiu96 Mar 29, 2023

blakeli0 Mar 29, 2023

lqiu96 Mar 29, 2023

burkedavison Mar 29, 2023

lqiu96 Mar 29, 2023

lqiu96 commented Mar 29, 2023

mpeddada1 Mar 29, 2023

lqiu96 Mar 29, 2023 •

edited

mpeddada1 Mar 29, 2023

lqiu96 Mar 29, 2023

mpeddada1 Mar 29, 2023

burkedavison Mar 29, 2023

lqiu96 Mar 29, 2023 •

edited

burkedavison Mar 29, 2023

lqiu96 Mar 29, 2023

mpeddada1 Mar 29, 2023

blakeli0 Mar 31, 2023

lqiu96 Mar 31, 2023

lqiu96 commented Mar 30, 2023

mpeddada1 Mar 31, 2023

lqiu96 Mar 31, 2023

mpeddada1 Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

lqiu96 commented Mar 31, 2023

chore: Add Showcase Http Annotations Test #1568

chore: Add Showcase Http Annotations Test #1568

Conversation

lqiu96 commented Mar 28, 2023 • edited

lqiu96 commented Mar 28, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 commented Mar 29, 2023

Choose a reason for hiding this comment

lqiu96 Mar 29, 2023 • 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 Mar 29, 2023 • 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

Choose a reason for hiding this comment

lqiu96 commented Mar 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

sonarcloud bot commented Mar 31, 2023

lqiu96 commented Mar 31, 2023

lqiu96 commented Mar 28, 2023 •

edited

lqiu96 Mar 29, 2023 •

edited

lqiu96 Mar 29, 2023 •

edited