Skip to content

Commit fdb705b

Browse files
miraleungmiraleung
authored
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)
1 parent 8f6f40e commit fdb705b

File tree

28 files changed

+128
-86
lines changed

28 files changed

+128
-86
lines changed

‎rules_bazel/java/integration_test.bzl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ def _diff_integration_goldens_impl(ctx):
2323
rm -rf $(find ./ -type f -name 'PlaceholderFile.java')
2424
rm -r $(find ./ -type d -empty)
2525
cd ..
26-
diff codegen_tmp test/integration/goldens/{api_name}/ > {diff_output}
26+
diff -r codegen_tmp test/integration/goldens/{api_name} > {diff_output}
2727
# Bash `diff` command will return exit code 1 when there are differences between the two
2828
# folders. So we explicitly `exit 0` after the diff command to avoid build failure.
2929
exit 0

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

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -29,19 +29,15 @@
2929
import com.google.api.gax.rpc.UnaryCallSettings;
3030
import com.google.api.generator.engine.ast.AnnotationNode;
3131
import com.google.api.generator.engine.ast.AssignmentExpr;
32-
import com.google.api.generator.engine.ast.CastExpr;
3332
import com.google.api.generator.engine.ast.ClassDefinition;
3433
import com.google.api.generator.engine.ast.CommentStatement;
3534
import com.google.api.generator.engine.ast.ConcreteReference;
3635
import com.google.api.generator.engine.ast.EmptyLineStatement;
37-
import com.google.api.generator.engine.ast.EnumRefExpr;
3836
import com.google.api.generator.engine.ast.Expr;
3937
import com.google.api.generator.engine.ast.ExprStatement;
40-
import com.google.api.generator.engine.ast.InstanceofExpr;
4138
import com.google.api.generator.engine.ast.LineComment;
4239
import com.google.api.generator.engine.ast.MethodDefinition;
4340
import com.google.api.generator.engine.ast.MethodInvocationExpr;
44-
import com.google.api.generator.engine.ast.NewObjectExpr;
4541
import com.google.api.generator.engine.ast.PrimitiveValue;
4642
import com.google.api.generator.engine.ast.Reference;
4743
import com.google.api.generator.engine.ast.ScopeNode;
@@ -80,6 +76,7 @@
8076
import java.util.Optional;
8177
import java.util.UUID;
8278
import java.util.concurrent.ExecutionException;
79+
import java.util.function.Function;
8380
import java.util.stream.Collectors;
8481
import javax.annotation.Generated;
8582
import org.junit.After;
@@ -98,7 +95,7 @@ public abstract class AbstractServiceClientTestClassComposer implements ClassCom
9895

9996
protected static final TypeStore FIXED_TYPESTORE = createStaticTypes();
10097
protected static final AnnotationNode TEST_ANNOTATION =
101-
AnnotationNode.withType(FIXED_TYPESTORE.get("Test"));
98+
AnnotationNode.withType(FIXED_TYPESTORE.get("Test"));
10299

103100
private final TransportContext transportContext;
104101

@@ -149,16 +146,35 @@ protected abstract Map<String, VariableExpr> createClassMemberVarExprs(
149146

150147
protected List<Statement> createClassMemberFieldDecls(
151148
Map<String, VariableExpr> classMemberVarExprs) {
152-
return classMemberVarExprs.values().stream()
153-
.map(
154-
v ->
155-
ExprStatement.withExpr(
156-
v.toBuilder()
157-
.setIsDecl(true)
158-
.setScope(ScopeNode.PRIVATE)
159-
.setIsStatic(v.type().reference().name().startsWith("Mock"))
160-
.build()))
161-
.collect(Collectors.toList());
149+
Function<VariableExpr, Boolean> isMockVarExprFn =
150+
v -> v.type().reference().name().startsWith("Mock");
151+
152+
// Ordering matters for pretty-printing and ensuring that test output is deterministic.
153+
List<Statement> fieldDeclStatements = new ArrayList<>();
154+
155+
// Static fields go first.
156+
fieldDeclStatements.addAll(
157+
classMemberVarExprs.values().stream()
158+
.filter(v -> isMockVarExprFn.apply(v))
159+
.map(
160+
v ->
161+
ExprStatement.withExpr(
162+
v.toBuilder()
163+
.setIsDecl(true)
164+
.setScope(ScopeNode.PRIVATE)
165+
.setIsStatic(true)
166+
.build()))
167+
.collect(Collectors.toList()));
168+
169+
fieldDeclStatements.addAll(
170+
classMemberVarExprs.values().stream()
171+
.filter(v -> !isMockVarExprFn.apply(v))
172+
.map(
173+
v ->
174+
ExprStatement.withExpr(
175+
v.toBuilder().setIsDecl(true).setScope(ScopeNode.PRIVATE).build()))
176+
.collect(Collectors.toList()));
177+
return fieldDeclStatements;
162178
}
163179

164180
private List<MethodDefinition> createClassMethods(

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -53,14 +53,13 @@
5353
import com.google.api.generator.gapic.model.Service;
5454
import com.google.api.generator.gapic.utils.JavaStyle;
5555
import com.google.common.base.Preconditions;
56-
import com.google.common.collect.ImmutableMap;
5756
import com.google.protobuf.AbstractMessage;
5857
import io.grpc.StatusRuntimeException;
5958
import java.util.ArrayList;
6059
import java.util.Arrays;
61-
import java.util.LinkedHashMap;
6260
import java.util.List;
6361
import java.util.Map;
62+
import java.util.TreeMap;
6463
import java.util.concurrent.ExecutionException;
6564
import java.util.function.BiFunction;
6665
import java.util.function.Function;
@@ -115,7 +114,8 @@ protected Map<String, VariableExpr> createClassMemberVarExprs(
115114
BiFunction<String, TypeNode, VariableExpr> varExprFn =
116115
(name, type) ->
117116
VariableExpr.withVariable(Variable.builder().setName(name).setType(type).build());
118-
Map<String, TypeNode> fields = new LinkedHashMap<>();
117+
// Keep keys sorted in alphabetical order to ensure that the test output is deterministic.
118+
Map<String, TypeNode> fields = new TreeMap<>();
119119
fields.put(
120120
getMockServiceVarName(service), typeStore.get(ClassNames.getMockServiceClassName(service)));
121121
for (Service mixinService : context.mixinServices()) {
@@ -132,8 +132,10 @@ protected Map<String, VariableExpr> createClassMemberVarExprs(
132132
Collectors.toMap(
133133
Map.Entry::getKey,
134134
e -> varExprFn.apply(e.getKey(), e.getValue()),
135-
(u, v) -> {throw new IllegalStateException();},
136-
LinkedHashMap::new));
135+
(u, v) -> {
136+
throw new IllegalStateException();
137+
},
138+
TreeMap::new));
137139
}
138140

139141
@Override

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,8 +26,8 @@ import org.junit.Test;
2626
public class DeprecatedServiceClientTest {
2727
private static MockDeprecatedService mockDeprecatedService;
2828
private static MockServiceHelper mockServiceHelper;
29-
private DeprecatedServiceClient client;
3029
private LocalChannelProvider channelProvider;
30+
private DeprecatedServiceClient client;
3131

3232
@BeforeClass
3333
public static void startStaticServer() {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,8 @@ import org.junit.Test;
4242
public class EchoClientTest {
4343
private static MockEcho mockEcho;
4444
private static MockServiceHelper mockServiceHelper;
45-
private EchoClient client;
4645
private LocalChannelProvider channelProvider;
46+
private EchoClient client;
4747

4848
@BeforeClass
4949
public static void startStaticServer() {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@ import org.junit.Test;
4545
public class LoggingServiceV2ClientTest {
4646
private static MockLoggingServiceV2 mockLoggingServiceV2;
4747
private static MockServiceHelper mockServiceHelper;
48-
private LoggingServiceV2Client client;
4948
private LocalChannelProvider channelProvider;
49+
private LoggingServiceV2Client client;
5050

5151
@BeforeClass
5252
public static void startStaticServer() {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,10 +36,10 @@ import org.junit.Test;
3636

3737
@Generated("by gapic-generator-java")
3838
public class SubscriberClientTest {
39-
private static MockSubscriber mockSubscriber;
4039
private static MockServiceHelper mockServiceHelper;
41-
private SubscriberClient client;
40+
private static MockSubscriber mockSubscriber;
4241
private LocalChannelProvider channelProvider;
42+
private SubscriberClient client;
4343

4444
@BeforeClass
4545
public static void startStaticServer() {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,10 @@ import org.junit.Test;
3131

3232
@Generated("by gapic-generator-java")
3333
public class TestingClientTest {
34-
private static MockTesting mockTesting;
3534
private static MockServiceHelper mockServiceHelper;
36-
private TestingClient client;
35+
private static MockTesting mockTesting;
3736
private LocalChannelProvider channelProvider;
37+
private TestingClient client;
3838

3939
@BeforeClass
4040
public static void startStaticServer() {

‎test/integration/goldens/asset/BUILD.bazel‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,11 @@ package(default_visibility = ["//visibility:public"])
22

33
filegroup(
44
name = "goldens_files",
5-
srcs = glob([
6-
"*.java",
7-
"gapic_metadata.json",
8-
]),
5+
srcs = glob(
6+
["**/*"],
7+
exclude = [
8+
"BUILD.bazel",
9+
".*.sw*",
10+
],
11+
),
912
)

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,10 @@
5252

5353
@Generated("by gapic-generator-java")
5454
public class AssetServiceClientTest {
55+
private static MockAssetService mockAssetService;
5556
private static MockServiceHelper mockServiceHelper;
56-
private AssetServiceClient client;
5757
private LocalChannelProvider channelProvider;
58-
private static MockAssetService mockAssetService;
58+
private AssetServiceClient client;
5959

6060
@BeforeClass
6161
public static void startStaticServer() {

0 commit comments

Comments
 (0)