fix: InstantiatingGrpcChannelProvider.toBuilder() should carry over all config data #1298
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1298 +/- ##
============================================
+ Coverage 79.34% 79.51% +0.17%
- Complexity 1235 1236 +1
============================================
Files 209 209
Lines 5398 5399 +1
Branches 454 454
============================================
+ Hits 4283 4293 +10
+ Misses 936 928 -8
+ Partials 179 178 -1
Continue to review full report at Codecov.
|
|
||
InstantiatingGrpcChannelProvider.Builder builder = provider.toBuilder(); | ||
|
||
assertThat(builder.getEndpoint()).isEqualTo("fake.endpoint:443"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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()
.
}; | ||
Map<String, ?> directPathServiceConfig = ImmutableMap.of("loadbalancingConfig", "grpclb"); | ||
|
||
InstantiatingGrpcChannelProvider provider = |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please address @elharo comments first
}; | ||
Map<String, ?> directPathServiceConfig = ImmutableMap.of("loadbalancingConfig", "grpclb"); | ||
|
||
InstantiatingGrpcChannelProvider provider = |
There was a problem hiding this comment.
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()
)
#1235 introduced a bug that InstantiatingGrpcChannelProvider.toBuilder() is missing the config data.
@vam-google