Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

fix: InstantiatingGrpcChannelProvider.toBuilder() should carry over all config data #1298

Merged
merged 1 commit into from Feb 12, 2021
Merged

fix: InstantiatingGrpcChannelProvider.toBuilder() should carry over all config data #1298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java
View file
Open in desktop
Expand Up @@ -411,6 +411,7 @@ private Builder(InstantiatingGrpcChannelProvider provider) {
this.credentials = provider.credentials;
this.channelPrimer = provider.channelPrimer;
this.attemptDirectPath = provider.attemptDirectPath;
this.directPathServiceConfig = provider.directPathServiceConfig;
}

/** Sets the number of available CPUs, used internally for testing. */
Expand Down
39 changes: 39 additions & 0 deletions gax-grpc/src/test/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProviderTest.java
View file
Open in desktop
Expand Up @@ -157,6 +157,45 @@ public void testWithPoolSize() throws IOException {
}
}

@Test
public void testToBuilder() {
Duration keepaliveTime = Duration.ofSeconds(1);
Duration keepaliveTimeout = Duration.ofSeconds(2);
ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> channelConfigurator =
new ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder>() {
@Override
public ManagedChannelBuilder apply(ManagedChannelBuilder input) {
throw new UnsupportedOperationException();
}
};
Map<String, ?> directPathServiceConfig = ImmutableMap.of("loadbalancingConfig", "grpclb");

InstantiatingGrpcChannelProvider provider =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this class have a good equals method? that could make for a more robust test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not (resolves to default Object#equals())

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InstantiatingGrpcChannelProvider has complex logic such as relation between PoolSize and ProcessorCount and ChannelsPerCpu, and attributes depending on system properties and env. It might not make much sense to simply implement equals() by comparing its fields, just for testing purpose.

InstantiatingGrpcChannelProvider.newBuilder()
.setProcessorCount(2)
.setEndpoint("fake.endpoint:443")
.setMaxInboundMessageSize(12345678)
.setMaxInboundMetadataSize(4096)
.setKeepAliveTime(keepaliveTime)
.setKeepAliveTimeout(keepaliveTimeout)
.setKeepAliveWithoutCalls(true)
.setChannelConfigurator(channelConfigurator)
.setChannelsPerCpu(2.5)
.setDirectPathServiceConfig(directPathServiceConfig)
.build();

InstantiatingGrpcChannelProvider.Builder builder = provider.toBuilder();

assertThat(builder.getEndpoint()).isEqualTo("fake.endpoint:443");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gratuitous use of Truth. assertEquals is fully adequate here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean org.junit.Assert.assertEquals() as I did't find Truth. assertEquals()? For org.junit.Assert.assertEquals() we tend to put expected value and actual value in wrong order and its error message is not as detailed as Truth. assertEquals().

assertThat(builder.getMaxInboundMessageSize()).isEqualTo(12345678);
assertThat(builder.getMaxInboundMetadataSize()).isEqualTo(4096);
assertThat(builder.getKeepAliveTime()).isEqualTo(keepaliveTime);
assertThat(builder.getKeepAliveTimeout()).isEqualTo(keepaliveTimeout);
assertThat(builder.getChannelConfigurator()).isEqualTo(channelConfigurator);
assertThat(builder.getPoolSize()).isEqualTo(5);
assertThat(builder.build().directPathServiceConfig).isEqualTo(directPathServiceConfig);
}

@Test
public void testWithInterceptors() throws Exception {
testWithInterceptors(1);
Expand Down