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)
googleapis · Jun 2, 2021 · fdb705b · fdb705b
1 parent 8f6f40e
commit fdb705b
Show file tree

Hide file tree

Showing 28 changed files with 128 additions and 86 deletions.
diff --git a/rules_bazel/java/integration_test.bzl b/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

diff --git a/...om/google/api/generator/gapic/composer/common/AbstractServiceClientTestClassComposer.java b/...om/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(

diff --git a/...ain/java/com/google/api/generator/gapic/composer/grpc/ServiceClientTestClassComposer.java b/...ain/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

diff --git a/...a/com/google/api/generator/gapic/composer/grpc/goldens/DeprecatedServiceClientTest.golden b/...a/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() {

diff --git a/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/EchoClientTest.golden b/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() {

diff --git a/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/LoggingClientTest.golden b/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() {

diff --git a/...est/java/com/google/api/generator/gapic/composer/grpc/goldens/SubscriberClientTest.golden b/...est/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() {

diff --git a/src/test/java/com/google/api/generator/gapic/composer/grpc/goldens/TestingClientTest.golden b/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() {

diff --git a/test/integration/goldens/asset/BUILD.bazel b/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*",
+        ],
+    ),
 )
diff --git a/test/integration/goldens/asset/com/google/cloud/asset/v1/AssetServiceClientTest.java b/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() {

diff --git a/test/integration/goldens/credentials/BUILD.bazel b/test/integration/goldens/credentials/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*",
+        ],
+    ),
 )
diff --git a/...ion/goldens/credentials/com/google/cloud/iam/credentials/v1/IAMCredentialsClientTest.java b/...ion/goldens/credentials/com/google/cloud/iam/credentials/v1/IAMCredentialsClientTest.java
@@ -43,10 +43,10 @@
 
 @Generated("by gapic-generator-java")
 public class IAMCredentialsClientTest {
-  private static MockServiceHelper mockServiceHelper;
   private static MockIAMCredentials mockIAMCredentials;
-  private IamCredentialsClient client;
+  private static MockServiceHelper mockServiceHelper;
   private LocalChannelProvider channelProvider;
+  private IamCredentialsClient client;
 
   @BeforeClass
   public static void startStaticServer() {

diff --git a/test/integration/goldens/iam/BUILD.bazel b/test/integration/goldens/iam/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*",
+        ],
+    ),
 )
diff --git a/test/integration/goldens/iam/com/google/iam/v1/IAMPolicyClientTest.java b/test/integration/goldens/iam/com/google/iam/v1/IAMPolicyClientTest.java
@@ -41,10 +41,10 @@
 
 @Generated("by gapic-generator-java")
 public class IAMPolicyClientTest {
-  private static MockServiceHelper mockServiceHelper;
-  private IAMPolicyClient client;
   private static MockIAMPolicy mockIAMPolicy;
+  private static MockServiceHelper mockServiceHelper;
   private LocalChannelProvider channelProvider;
+  private IAMPolicyClient client;
 
   @BeforeClass
   public static void startStaticServer() {

diff --git a/test/integration/goldens/kms/BUILD.bazel b/test/integration/goldens/kms/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*",
+        ],
+    ),
 )
diff --git a/test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClientTest.java b/test/integration/goldens/kms/com/google/cloud/kms/v1/KeyManagementServiceClientTest.java
@@ -66,12 +66,12 @@
 
 @Generated("by gapic-generator-java")
 public class KeyManagementServiceClientTest {
-  private static MockKeyManagementService mockKeyManagementService;
-  private static MockServiceHelper mockServiceHelper;
-  private KeyManagementServiceClient client;
   private static MockIAMPolicy mockIAMPolicy;
+  private static MockKeyManagementService mockKeyManagementService;
   private static MockLocations mockLocations;
+  private static MockServiceHelper mockServiceHelper;
   private LocalChannelProvider channelProvider;
+  private KeyManagementServiceClient client;
 
   @BeforeClass
   public static void startStaticServer() {

diff --git a/test/integration/goldens/library/BUILD.bazel b/test/integration/goldens/library/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*",
+        ],
+    ),
 )
diff --git a/...gration/goldens/library/com/google/cloud/example/library/v1/LibraryServiceClientTest.java b/...gration/goldens/library/com/google/cloud/example/library/v1/LibraryServiceClientTest.java
@@ -61,10 +61,10 @@
 
 @Generated("by gapic-generator-java")
 public class LibraryServiceClientTest {
-  private static MockServiceHelper mockServiceHelper;
-  private LibraryServiceClient client;
   private static MockLibraryService mockLibraryService;
+  private static MockServiceHelper mockServiceHelper;
   private LocalChannelProvider channelProvider;
+  private LibraryServiceClient client;
 
   @BeforeClass
   public static void startStaticServer() {

diff --git a/test/integration/goldens/logging/BUILD.bazel b/test/integration/goldens/logging/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*",
+        ],
+    ),
 )
diff --git a/test/integration/goldens/logging/com/google/cloud/logging/v2/ConfigClientTest.java b/test/integration/goldens/logging/com/google/cloud/logging/v2/ConfigClientTest.java
@@ -95,10 +95,10 @@
 
 @Generated("by gapic-generator-java")
 public class ConfigClientTest {
+  private static MockConfigServiceV2 mockConfigServiceV2;
   private static MockServiceHelper mockServiceHelper;
-  private ConfigClient client;
   private LocalChannelProvider channelProvider;
-  private static MockConfigServiceV2 mockConfigServiceV2;
+  private ConfigClient client;
 
   @BeforeClass
   public static void startStaticServer() {

diff --git a/test/integration/goldens/logging/com/google/cloud/logging/v2/LoggingClientTest.java b/test/integration/goldens/logging/com/google/cloud/logging/v2/LoggingClientTest.java
@@ -73,10 +73,10 @@
 
 @Generated("by gapic-generator-java")
 public class LoggingClientTest {
-  private static MockServiceHelper mockServiceHelper;
   private static MockLoggingServiceV2 mockLoggingServiceV2;
-  private LoggingClient client;
+  private static MockServiceHelper mockServiceHelper;
   private LocalChannelProvider channelProvider;
+  private LoggingClient client;
 
   @BeforeClass
   public static void startStaticServer() {

diff --git a/test/integration/goldens/logging/com/google/cloud/logging/v2/MetricsClientTest.java b/test/integration/goldens/logging/com/google/cloud/logging/v2/MetricsClientTest.java
@@ -56,10 +56,10 @@
 
 @Generated("by gapic-generator-java")
 public class MetricsClientTest {
-  private static MockServiceHelper mockServiceHelper;
-  private MetricsClient client;
   private static MockMetricsServiceV2 mockMetricsServiceV2;
+  private static MockServiceHelper mockServiceHelper;
   private LocalChannelProvider channelProvider;
+  private MetricsClient client;
 
   @BeforeClass
   public static void startStaticServer() {