Skip to content

Commit eaf4592

Browse files
miraleungmiraleung
authored
fix(tests): handle Java 11 set ordering differences for RPCs and fields 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
1 parent ddc75f9 commit eaf4592

6 files changed

Lines changed: 57 additions & 33 deletions

File tree

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

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,12 @@ protected MethodDefinition createStartStaticServerMethod(
159159
List<Expr> mockServiceVarExprs = new ArrayList<>();
160160
varInitExprs.add(serviceToVarInitExprFn.apply(service));
161161
mockServiceVarExprs.add(serviceToVarExprFn.apply(service));
162-
for (Service mixinService : context.mixinServices()) {
162+
// Careful: Java 8 and 11 make different ordering choices if this set is not explicitly sorted.
163+
// Context: https://github.com/googleapis/gapic-generator-java/pull/750
164+
for (Service mixinService :
165+
context.mixinServices().stream()
166+
.sorted((s1, s2) -> s2.name().compareTo(s1.name()))
167+
.collect(Collectors.toList())) {
163168
varInitExprs.add(serviceToVarInitExprFn.apply(mixinService));
164169
mockServiceVarExprs.add(serviceToVarExprFn.apply(mixinService));
165170
}
@@ -201,12 +206,15 @@ protected MethodDefinition createStartStaticServerMethod(
201206
varInitExprs.add(initServiceHelperExpr);
202207
varInitExprs.add(startServiceHelperExpr);
203208

209+
List<Statement> body = new ArrayList<>();
210+
204211
return MethodDefinition.builder()
205212
.setAnnotations(Arrays.asList(AnnotationNode.withType(FIXED_TYPESTORE.get("BeforeClass"))))
206213
.setScope(ScopeNode.PUBLIC)
207214
.setIsStatic(true)
208215
.setReturnType(TypeNode.VOID)
209216
.setName("startStaticServer")
217+
.setBody(body)
210218
.setBody(
211219
varInitExprs.stream().map(e -> ExprStatement.withExpr(e)).collect(Collectors.toList()))
212220
.build();

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

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -270,9 +270,15 @@ public static List<Service> parseServices(
270270
}
271271
}
272272

273+
// Sort potential mixin services alphabetically.
274+
List<Service> orderedBlockedCodegenMixinApis =
275+
blockedCodegenMixinApis.stream()
276+
.sorted((s1, s2) -> s2.name().compareTo(s1.name()))
277+
.collect(Collectors.toList());
278+
273279
Set<String> apiDefinedRpcs = new HashSet<>();
274280
for (Service service : services) {
275-
if (blockedCodegenMixinApis.contains(service)) {
281+
if (orderedBlockedCodegenMixinApis.contains(service)) {
276282
continue;
277283
}
278284
apiDefinedRpcs.addAll(
@@ -285,7 +291,7 @@ public static List<Service> parseServices(
285291
Service originalService = services.get(i);
286292
List<Method> updatedOriginalServiceMethods = new ArrayList<>(originalService.methods());
287293
// If mixin APIs are present, add the methods to all other services.
288-
for (Service mixinService : blockedCodegenMixinApis) {
294+
for (Service mixinService : orderedBlockedCodegenMixinApis) {
289295
final String mixinServiceFullName = serviceFullNameFn.apply(mixinService);
290296
if (!mixedInApis.contains(mixinServiceFullName)) {
291297
continue;
@@ -327,6 +333,11 @@ public static List<Service> parseServices(
327333
m.toBuilder()
328334
.setMixedInApiName(serviceFullNameFn.apply(mixinService))
329335
.build()));
336+
// Sort by method name, to ensure a deterministic method ordering (for tests).
337+
updatedMixinMethods =
338+
updatedMixinMethods.stream()
339+
.sorted((m1, m2) -> m2.name().compareTo(m1.name()))
340+
.collect(Collectors.toList());
330341
outputMixinServiceSet.add(
331342
mixinService.toBuilder().setMethods(updatedMixinMethods).build());
332343
}
@@ -343,7 +354,10 @@ public static List<Service> parseServices(
343354
}
344355

345356
// Use a list to ensure ordering for deterministic tests.
346-
outputMixinServices.addAll(outputMixinServiceSet);
357+
outputMixinServices.addAll(
358+
outputMixinServiceSet.stream()
359+
.sorted((s1, s2) -> s2.name().compareTo(s1.name()))
360+
.collect(Collectors.toSet()));
347361
return services;
348362
}
349363

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,14 @@ documentation:
1616
Manages keys and performs cryptographic operations in a central cloud
1717
service, for direct use by other cloud resources and applications.
1818
rules:
19+
# This RPC shouldn't appear in the proto, since it's been clobered by KMS's definition in the proto.
1920
- selector: google.iam.v1.IAMPolicy.GetIamPolicy
2021
description: |-
2122
Gets the access control policy for a resource. Returns an empty policy
2223
if the resource exists and does not have a policy set.
2324
24-
# This RPC shouldn't appear in the proto even though the documentation field is set.
25+
# This RPC shouldn't appear in the proto, since it's not in the HTTP rules list below,
26+
# even though the documentation field is set.
2527
- selector: google.iam.v1.IAMPolicy.SetIamPolicy
2628
description: |-
2729
Sets the access control policy on the specified resource. Replaces

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -76,12 +76,12 @@ public class KeyManagementServiceClientTest {
7676
@BeforeClass
7777
public static void startStaticServer() {
7878
mockKeyManagementService = new MockKeyManagementService();
79-
mockIAMPolicy = new MockIAMPolicy();
8079
mockLocations = new MockLocations();
80+
mockIAMPolicy = new MockIAMPolicy();
8181
mockServiceHelper =
8282
new MockServiceHelper(
8383
UUID.randomUUID().toString(),
84-
Arrays.<MockGrpcService>asList(mockKeyManagementService, mockIAMPolicy, mockLocations));
84+
Arrays.<MockGrpcService>asList(mockKeyManagementService, mockLocations, mockIAMPolicy));
8585
mockServiceHelper.start();
8686
}
8787

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

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -59,43 +59,43 @@ public void reset() {
5959
}
6060

6161
@Override
62-
public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
62+
public void testIamPermissions(
63+
TestIamPermissionsRequest request,
64+
StreamObserver<TestIamPermissionsResponse> responseObserver) {
6365
Object response = responses.poll();
64-
if (response instanceof Policy) {
66+
if (response instanceof TestIamPermissionsResponse) {
6567
requests.add(request);
66-
responseObserver.onNext(((Policy) response));
68+
responseObserver.onNext(((TestIamPermissionsResponse) response));
6769
responseObserver.onCompleted();
6870
} else if (response instanceof Exception) {
6971
responseObserver.onError(((Exception) response));
7072
} else {
7173
responseObserver.onError(
7274
new IllegalArgumentException(
7375
String.format(
74-
"Unrecognized response type %s for method GetIamPolicy, expected %s or %s",
76+
"Unrecognized response type %s for method TestIamPermissions, expected %s or %s",
7577
response == null ? "null" : response.getClass().getName(),
76-
Policy.class.getName(),
78+
TestIamPermissionsResponse.class.getName(),
7779
Exception.class.getName())));
7880
}
7981
}
8082

8183
@Override
82-
public void testIamPermissions(
83-
TestIamPermissionsRequest request,
84-
StreamObserver<TestIamPermissionsResponse> responseObserver) {
84+
public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
8585
Object response = responses.poll();
86-
if (response instanceof TestIamPermissionsResponse) {
86+
if (response instanceof Policy) {
8787
requests.add(request);
88-
responseObserver.onNext(((TestIamPermissionsResponse) response));
88+
responseObserver.onNext(((Policy) response));
8989
responseObserver.onCompleted();
9090
} else if (response instanceof Exception) {
9191
responseObserver.onError(((Exception) response));
9292
} else {
9393
responseObserver.onError(
9494
new IllegalArgumentException(
9595
String.format(
96-
"Unrecognized response type %s for method TestIamPermissions, expected %s or %s",
96+
"Unrecognized response type %s for method GetIamPolicy, expected %s or %s",
9797
response == null ? "null" : response.getClass().getName(),
98-
TestIamPermissionsResponse.class.getName(),
98+
Policy.class.getName(),
9999
Exception.class.getName())));
100100
}
101101
}

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

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -64,27 +64,29 @@ public void reset() {
6464
}
6565

6666
@Override
67-
public void setIamPolicy(SetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
67+
public void testIamPermissions(
68+
TestIamPermissionsRequest request,
69+
StreamObserver<TestIamPermissionsResponse> responseObserver) {
6870
Object response = responses.poll();
69-
if (response instanceof Policy) {
71+
if (response instanceof TestIamPermissionsResponse) {
7072
requests.add(request);
71-
responseObserver.onNext(((Policy) response));
73+
responseObserver.onNext(((TestIamPermissionsResponse) response));
7274
responseObserver.onCompleted();
7375
} else if (response instanceof Exception) {
7476
responseObserver.onError(((Exception) response));
7577
} else {
7678
responseObserver.onError(
7779
new IllegalArgumentException(
7880
String.format(
79-
"Unrecognized response type %s for method SetIamPolicy, expected %s or %s",
81+
"Unrecognized response type %s for method TestIamPermissions, expected %s or %s",
8082
response == null ? "null" : response.getClass().getName(),
81-
Policy.class.getName(),
83+
TestIamPermissionsResponse.class.getName(),
8284
Exception.class.getName())));
8385
}
8486
}
8587

8688
@Override
87-
public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
89+
public void setIamPolicy(SetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
8890
Object response = responses.poll();
8991
if (response instanceof Policy) {
9092
requests.add(request);
@@ -96,31 +98,29 @@ public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> res
9698
responseObserver.onError(
9799
new IllegalArgumentException(
98100
String.format(
99-
"Unrecognized response type %s for method GetIamPolicy, expected %s or %s",
101+
"Unrecognized response type %s for method SetIamPolicy, expected %s or %s",
100102
response == null ? "null" : response.getClass().getName(),
101103
Policy.class.getName(),
102104
Exception.class.getName())));
103105
}
104106
}
105107

106108
@Override
107-
public void testIamPermissions(
108-
TestIamPermissionsRequest request,
109-
StreamObserver<TestIamPermissionsResponse> responseObserver) {
109+
public void getIamPolicy(GetIamPolicyRequest request, StreamObserver<Policy> responseObserver) {
110110
Object response = responses.poll();
111-
if (response instanceof TestIamPermissionsResponse) {
111+
if (response instanceof Policy) {
112112
requests.add(request);
113-
responseObserver.onNext(((TestIamPermissionsResponse) response));
113+
responseObserver.onNext(((Policy) response));
114114
responseObserver.onCompleted();
115115
} else if (response instanceof Exception) {
116116
responseObserver.onError(((Exception) response));
117117
} else {
118118
responseObserver.onError(
119119
new IllegalArgumentException(
120120
String.format(
121-
"Unrecognized response type %s for method TestIamPermissions, expected %s or %s",
121+
"Unrecognized response type %s for method GetIamPolicy, expected %s or %s",
122122
response == null ? "null" : response.getClass().getName(),
123-
TestIamPermissionsResponse.class.getName(),
123+
Policy.class.getName(),
124124
Exception.class.getName())));
125125
}
126126
}

0 commit comments

Comments
 (0)