fix(tests): handle Java 11 set ordering differences for RPCs and fiel…

…ds in test/mock classes [ggj] (#750)

* chore: add context to diff

* fix: Sort services and methods by name

* fix: add more context

* fix: more service ordering

* fix: test

* fix: test

* fix: test

* fix: test

* fix: test

* fix: test ordering again

* fix: cleanup

src/main/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java

@@ -159,7 +159,12 @@ protected MethodDefinition createStartStaticServerMethod(
     List<Expr> mockServiceVarExprs = new ArrayList<>();
     varInitExprs.add(serviceToVarInitExprFn.apply(service));
     mockServiceVarExprs.add(serviceToVarExprFn.apply(service));
-    for (Service mixinService : context.mixinServices()) {
+    // Careful: Java 8 and 11 make different ordering choices if this set is not explicitly sorted.
+    // Context: https://github.com/googleapis/gapic-generator-java/pull/750
+    for (Service mixinService :
+        context.mixinServices().stream()
+            .sorted((s1, s2) -> s2.name().compareTo(s1.name()))
+            .collect(Collectors.toList())) {
       varInitExprs.add(serviceToVarInitExprFn.apply(mixinService));
       mockServiceVarExprs.add(serviceToVarExprFn.apply(mixinService));
     }
@@ -201,12 +206,15 @@ protected MethodDefinition createStartStaticServerMethod(
     varInitExprs.add(initServiceHelperExpr);
     varInitExprs.add(startServiceHelperExpr);
 
+    List<Statement> body = new ArrayList<>();
+
     return MethodDefinition.builder()
         .setAnnotations(Arrays.asList(AnnotationNode.withType(FIXED_TYPESTORE.get("BeforeClass"))))
         .setScope(ScopeNode.PUBLIC)
         .setIsStatic(true)
         .setReturnType(TypeNode.VOID)
         .setName("startStaticServer")
+        .setBody(body)
         .setBody(
             varInitExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()))
         .build();

src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

@@ -270,9 +270,15 @@ public static List<Service> parseServices(
       }
     }
 
+    // Sort potential mixin services alphabetically.
+    List<Service> orderedBlockedCodegenMixinApis =
+        blockedCodegenMixinApis.stream()
+            .sorted((s1, s2) -> s2.name().compareTo(s1.name()))
+            .collect(Collectors.toList());
+
     Set<String> apiDefinedRpcs = new HashSet<>();
     for (Service service : services) {
-      if (blockedCodegenMixinApis.contains(service)) {
+      if (orderedBlockedCodegenMixinApis.contains(service)) {
         continue;
       }
       apiDefinedRpcs.addAll(
@@ -285,7 +291,7 @@ public static List<Service> parseServices(
         Service originalService = services.get(i);
         List<Method> updatedOriginalServiceMethods = new ArrayList<>(originalService.methods());
         // If mixin APIs are present, add the methods to all other services.
-        for (Service mixinService : blockedCodegenMixinApis) {
+        for (Service mixinService : orderedBlockedCodegenMixinApis) {
           final String mixinServiceFullName = serviceFullNameFn.apply(mixinService);
           if (!mixedInApis.contains(mixinServiceFullName)) {
             continue;
@@ -327,6 +333,11 @@ public static List<Service> parseServices(
                           m.toBuilder()
                               .setMixedInApiName(serviceFullNameFn.apply(mixinService))
                               .build()));
+          // Sort by method name, to ensure a deterministic method ordering (for tests).
+          updatedMixinMethods =
+              updatedMixinMethods.stream()
+                  .sorted((m1, m2) -> m2.name().compareTo(m1.name()))
+                  .collect(Collectors.toList());
           outputMixinServiceSet.add(
               mixinService.toBuilder().setMethods(updatedMixinMethods).build());
         }
@@ -343,7 +354,10 @@ public static List<Service> parseServices(
     }
 
     // Use a list to ensure ordering for deterministic tests.
-    outputMixinServices.addAll(outputMixinServiceSet);
+    outputMixinServices.addAll(
+        outputMixinServiceSet.stream()
+            .sorted((s1, s2) -> s2.name().compareTo(s1.name()))
+            .collect(Collectors.toSet()));
     return services;
   }
 

test/integration/apis/kms/v1/cloudkms_test_mixins_v1.yaml

@@ -16,12 +16,14 @@ documentation:
     Manages keys and performs cryptographic operations in a central cloud
     service, for direct use by other cloud resources and applications.
   rules:
+  # This RPC shouldn't appear in the proto, since it's been clobered by KMS's definition in the proto.
   - selector: google.iam.v1.IAMPolicy.GetIamPolicy
     description: |-
       Gets the access control policy for a resource. Returns an empty policy
       if the resource exists and does not have a policy set.
 
-  # This RPC shouldn't appear in the proto even though the documentation field is set.
+  # This RPC shouldn't appear in the proto, since it's not in the HTTP rules list below,
+  # even though the documentation field is set.
   - selector: google.iam.v1.IAMPolicy.SetIamPolicy
     description: |-
       Sets the access control policy on the specified resource. Replaces

test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClientTest.java

@@ -76,12 +76,12 @@ public class KeyManagementServiceClientTest {
   @BeforeClass
   public static void startStaticServer() {
     mockKeyManagementService = new MockKeyManagementService();
-    mockIAMPolicy = new MockIAMPolicy();
     mockLocations = new MockLocations();
+    mockIAMPolicy = new MockIAMPolicy();
     mockServiceHelper =
         new MockServiceHelper(
             UUID.randomUUID().toString(),
-            Arrays.<MockGrpcService>asList(mockKeyManagementService, mockIAMPolicy, mockLocations));
+            Arrays.<MockGrpcService>asList(mockKeyManagementService, mockLocations, mockIAMPolicy));
     mockServiceHelper.start();
   }
 

test/integration/goldens/kms/com/google/iam/v1/MockIAMPolicyImpl.java

@@ -59,43 +59,43 @@ public void reset() {
   }
 
   @Override
-  public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
+  public void testIamPermissions(
+      TestIamPermissionsRequest request,
+      StreamObserver<TestIamPermissionsResponse> responseObserver) {
     Object response = responses.poll();
-    if (response instanceof Policy) {
+    if (response instanceof TestIamPermissionsResponse) {
       requests.add(request);
-      responseObserver.onNext(((Policy) response));
+      responseObserver.onNext(((TestIamPermissionsResponse) response));
       responseObserver.onCompleted();
     } else if (response instanceof Exception) {
       responseObserver.onError(((Exception) response));
     } else {
       responseObserver.onError(
           new IllegalArgumentException(
               String.format(
-                  "Unrecognized response type %s for method GetIamPolicy, expected %s or %s",
+                  "Unrecognized response type %s for method TestIamPermissions, expected %s or %s",
                   response == null ? "null" : response.getClass().getName(),
-                  Policy.class.getName(),
+                  TestIamPermissionsResponse.class.getName(),
                   Exception.class.getName())));
     }
   }
 
   @Override
-  public void testIamPermissions(
-      TestIamPermissionsRequest request,
-      StreamObserver<TestIamPermissionsResponse> responseObserver) {
+  public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
     Object response = responses.poll();
-    if (response instanceof TestIamPermissionsResponse) {
+    if (response instanceof Policy) {
       requests.add(request);
-      responseObserver.onNext(((TestIamPermissionsResponse) response));
+      responseObserver.onNext(((Policy) response));
       responseObserver.onCompleted();
     } else if (response instanceof Exception) {
       responseObserver.onError(((Exception) response));
     } else {
       responseObserver.onError(
           new IllegalArgumentException(
               String.format(
-                  "Unrecognized response type %s for method TestIamPermissions, expected %s or %s",
+                  "Unrecognized response type %s for method GetIamPolicy, expected %s or %s",
                   response == null ? "null" : response.getClass().getName(),
-                  TestIamPermissionsResponse.class.getName(),
+                  Policy.class.getName(),
                   Exception.class.getName())));
     }
   }

test/integration/goldens/pubsub/com/google/cloud/pubsub/v1/MockIAMPolicyImpl.java

@@ -64,27 +64,29 @@ public void reset() {
   }
 
   @Override
-  public void setIamPolicy(SetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
+  public void testIamPermissions(
+      TestIamPermissionsRequest request,
+      StreamObserver<TestIamPermissionsResponse> responseObserver) {
     Object response = responses.poll();
-    if (response instanceof Policy) {
+    if (response instanceof TestIamPermissionsResponse) {
       requests.add(request);
-      responseObserver.onNext(((Policy) response));
+      responseObserver.onNext(((TestIamPermissionsResponse) response));
       responseObserver.onCompleted();
     } else if (response instanceof Exception) {
       responseObserver.onError(((Exception) response));
     } else {
       responseObserver.onError(
           new IllegalArgumentException(
               String.format(
-                  "Unrecognized response type %s for method SetIamPolicy, expected %s or %s",
+                  "Unrecognized response type %s for method TestIamPermissions, expected %s or %s",
                   response == null ? "null" : response.getClass().getName(),
-                  Policy.class.getName(),
+                  TestIamPermissionsResponse.class.getName(),
                   Exception.class.getName())));
     }
   }
 
   @Override
-  public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
+  public void setIamPolicy(SetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
     Object response = responses.poll();
     if (response instanceof Policy) {
       requests.add(request);
@@ -96,31 +98,29 @@ public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> res
       responseObserver.onError(
           new IllegalArgumentException(
               String.format(
-                  "Unrecognized response type %s for method GetIamPolicy, expected %s or %s",
+                  "Unrecognized response type %s for method SetIamPolicy, expected %s or %s",
                   response == null ? "null" : response.getClass().getName(),
                   Policy.class.getName(),
                   Exception.class.getName())));
     }
   }
 
   @Override
-  public void testIamPermissions(
-      TestIamPermissionsRequest request,
-      StreamObserver<TestIamPermissionsResponse> responseObserver) {
+  public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
     Object response = responses.poll();
-    if (response instanceof TestIamPermissionsResponse) {
+    if (response instanceof Policy) {
       requests.add(request);
-      responseObserver.onNext(((TestIamPermissionsResponse) response));
+      responseObserver.onNext(((Policy) response));
       responseObserver.onCompleted();
     } else if (response instanceof Exception) {
       responseObserver.onError(((Exception) response));
     } else {
       responseObserver.onError(
           new IllegalArgumentException(
               String.format(
-                  "Unrecognized response type %s for method TestIamPermissions, expected %s or %s",
+                  "Unrecognized response type %s for method GetIamPolicy, expected %s or %s",
                   response == null ? "null" : response.getClass().getName(),
-                  TestIamPermissionsResponse.class.getName(),
+                  Policy.class.getName(),
                   Exception.class.getName())));
     }
   }