fix(tests): Ensure deterministic field ordering in test classes [ggj] (#743)

* fix(tests): Ensure deterministic field ordering in test classes

* fix(bazel): fix recursive integration test file diffs (#744)

Author: miraleung

rules_bazel/java/integration_test.bzl

@@ -23,7 +23,7 @@ def _diff_integration_goldens_impl(ctx):
     rm -rf $(find ./ -type f -name 'PlaceholderFile.java')
     rm -r $(find ./ -type d -empty)
     cd ..
-    diff codegen_tmp test/integration/goldens/{api_name}/ > {diff_output}
+    diff -r codegen_tmp test/integration/goldens/{api_name} > {diff_output}
     # Bash `diff` command will return exit code 1 when there are differences between the two
     # folders. So we explicitly `exit 0` after the diff command to avoid build failure.
     exit 0

src/main/java/com/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java

@@ -29,19 +29,15 @@
 import com.google.api.gax.rpc.UnaryCallSettings;
 import com.google.api.generator.engine.ast.AnnotationNode;
 import com.google.api.generator.engine.ast.AssignmentExpr;
-import com.google.api.generator.engine.ast.CastExpr;
 import com.google.api.generator.engine.ast.ClassDefinition;
 import com.google.api.generator.engine.ast.CommentStatement;
 import com.google.api.generator.engine.ast.ConcreteReference;
 import com.google.api.generator.engine.ast.EmptyLineStatement;
-import com.google.api.generator.engine.ast.EnumRefExpr;
 import com.google.api.generator.engine.ast.Expr;
 import com.google.api.generator.engine.ast.ExprStatement;
-import com.google.api.generator.engine.ast.InstanceofExpr;
 import com.google.api.generator.engine.ast.LineComment;
 import com.google.api.generator.engine.ast.MethodDefinition;
 import com.google.api.generator.engine.ast.MethodInvocationExpr;
-import com.google.api.generator.engine.ast.NewObjectExpr;
 import com.google.api.generator.engine.ast.PrimitiveValue;
 import com.google.api.generator.engine.ast.Reference;
 import com.google.api.generator.engine.ast.ScopeNode;
@@ -80,6 +76,7 @@
 import java.util.Optional;
 import java.util.UUID;
 import java.util.concurrent.ExecutionException;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 import javax.annotation.Generated;
 import org.junit.After;
@@ -98,7 +95,7 @@ public abstract class AbstractServiceClientTestClassComposer implements ClassCom
 
   protected static final TypeStore FIXED_TYPESTORE = createStaticTypes();
   protected static final AnnotationNode TEST_ANNOTATION =
-        AnnotationNode.withType(FIXED_TYPESTORE.get("Test"));
+      AnnotationNode.withType(FIXED_TYPESTORE.get("Test"));
 
   private final TransportContext transportContext;
 
@@ -149,16 +146,35 @@ protected abstract Map<String, VariableExpr> createClassMemberVarExprs(
 
   protected List<Statement> createClassMemberFieldDecls(
       Map<String, VariableExpr> classMemberVarExprs) {
-    return classMemberVarExprs.values().stream()
-        .map(
-            v ->
-                ExprStatement.withExpr(
-                    v.toBuilder()
-                        .setIsDecl(true)
-                        .setScope(ScopeNode.PRIVATE)
-                        .setIsStatic(v.type().reference().name().startsWith("Mock"))
-                        .build()))
-        .collect(Collectors.toList());
+    Function<VariableExpr, Boolean> isMockVarExprFn =
+        v -> v.type().reference().name().startsWith("Mock");
+
+    // Ordering matters for pretty-printing and ensuring that test output is deterministic.
+    List<Statement> fieldDeclStatements = new ArrayList<>();
+
+    // Static fields go first.
+    fieldDeclStatements.addAll(
+        classMemberVarExprs.values().stream()
+            .filter(v -> isMockVarExprFn.apply(v))
+            .map(
+                v ->
+                    ExprStatement.withExpr(
+                        v.toBuilder()
+                            .setIsDecl(true)
+                            .setScope(ScopeNode.PRIVATE)
+                            .setIsStatic(true)
+                            .build()))
+            .collect(Collectors.toList()));
+
+    fieldDeclStatements.addAll(
+        classMemberVarExprs.values().stream()
+            .filter(v -> !isMockVarExprFn.apply(v))
+            .map(
+                v ->
+                    ExprStatement.withExpr(
+                        v.toBuilder().setIsDecl(true).setScope(ScopeNode.PRIVATE).build()))
+            .collect(Collectors.toList()));
+    return fieldDeclStatements;
   }
 
   private List<MethodDefinition> createClassMethods(

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

@@ -53,14 +53,13 @@
 import com.google.api.generator.gapic.model.Service;
 import com.google.api.generator.gapic.utils.JavaStyle;
 import com.google.common.base.Preconditions;
-import com.google.common.collect.ImmutableMap;
 import com.google.protobuf.AbstractMessage;
 import io.grpc.StatusRuntimeException;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.TreeMap;
 import java.util.concurrent.ExecutionException;
 import java.util.function.BiFunction;
 import java.util.function.Function;
@@ -115,7 +114,8 @@ protected Map<String, VariableExpr> createClassMemberVarExprs(
     BiFunction<String, TypeNode, VariableExpr> varExprFn =
         (name, type) ->
             VariableExpr.withVariable(Variable.builder().setName(name).setType(type).build());
-    Map<String, TypeNode> fields = new LinkedHashMap<>();
+    // Keep keys sorted in alphabetical order to ensure that the test output is deterministic.
+    Map<String, TypeNode> fields = new TreeMap<>();
     fields.put(
         getMockServiceVarName(service), typeStore.get(ClassNames.getMockServiceClassName(service)));
     for (Service mixinService : context.mixinServices()) {
@@ -132,8 +132,10 @@ protected Map<String, VariableExpr> createClassMemberVarExprs(
             Collectors.toMap(
                 Map.Entry::getKey,
                 e -> varExprFn.apply(e.getKey(), e.getValue()),
-                (u, v) -> {throw new IllegalStateException();},
-                LinkedHashMap::new));
+                (u, v) -> {
+                  throw new IllegalStateException();
+                },
+                TreeMap::new));
   }
 
   @Override

src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/DeprecatedServiceClientTest.golden

@@ -26,8 +26,8 @@ import org.junit.Test;
 public class DeprecatedServiceClientTest {
   private static MockDeprecatedService mockDeprecatedService;
   private static MockServiceHelper mockServiceHelper;
-  private DeprecatedServiceClient client;
   private LocalChannelProvider channelProvider;
+  private DeprecatedServiceClient client;
 
   @BeforeClass
   public static void startStaticServer() {

src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/EchoClientTest.golden

@@ -42,8 +42,8 @@ import org.junit.Test;
 public class EchoClientTest {
   private static MockEcho mockEcho;
   private static MockServiceHelper mockServiceHelper;
-  private EchoClient client;
   private LocalChannelProvider channelProvider;
+  private EchoClient client;
 
   @BeforeClass
   public static void startStaticServer() {

src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/LoggingClientTest.golden

@@ -45,8 +45,8 @@ import org.junit.Test;
 public class LoggingServiceV2ClientTest {
   private static MockLoggingServiceV2 mockLoggingServiceV2;
   private static MockServiceHelper mockServiceHelper;
-  private LoggingServiceV2Client client;
   private LocalChannelProvider channelProvider;
+  private LoggingServiceV2Client client;
 
   @BeforeClass
   public static void startStaticServer() {

src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/SubscriberClientTest.golden

@@ -36,10 +36,10 @@ import org.junit.Test;
 
 @Generated("by gapic-generator-java")
 public class SubscriberClientTest {
-  private static MockSubscriber mockSubscriber;
   private static MockServiceHelper mockServiceHelper;
-  private SubscriberClient client;
+  private static MockSubscriber mockSubscriber;
   private LocalChannelProvider channelProvider;
+  private SubscriberClient client;
 
   @BeforeClass
   public static void startStaticServer() {

src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/TestingClientTest.golden

@@ -31,10 +31,10 @@ import org.junit.Test;
 
 @Generated("by gapic-generator-java")
 public class TestingClientTest {
-  private static MockTesting mockTesting;
   private static MockServiceHelper mockServiceHelper;
-  private TestingClient client;
+  private static MockTesting mockTesting;
   private LocalChannelProvider channelProvider;
+  private TestingClient client;
 
   @BeforeClass
   public static void startStaticServer() {

test/integration/goldens/asset/BUILD.bazel

@@ -2,8 +2,11 @@ package(default_visibility = ["//visibility:public"])
 
 filegroup(
     name = "goldens_files",
-    srcs = glob([
-        "*.java",
-        "gapic_metadata.json",
-    ]),
+    srcs = glob(
+        ["**/*"],
+        exclude = [
+            "BUILD.bazel",
+            ".*.sw*",
+        ],
+    ),
 )

test/integration/goldens/asset/com/google/cloud/asset/v1/AssetServiceClientTest.java

@@ -52,10 +52,10 @@
 
 @Generated("by gapic-generator-java")
 public class AssetServiceClientTest {
+  private static MockAssetService mockAssetService;
   private static MockServiceHelper mockServiceHelper;
-  private AssetServiceClient client;
   private LocalChannelProvider channelProvider;
-  private static MockAssetService mockAssetService;
+  private AssetServiceClient client;
 
   @BeforeClass
   public static void startStaticServer() {