Conversation
google-cla
Bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1272 +/- ##
============================================
+ Coverage 79.16% 79.23% +0.06%
- Complexity 1228 1236 +8
============================================
Files 209 209
Lines 5362 5378 +16
Branches 448 454 +6
============================================
+ Hits 4245 4261 +16
+ Misses 939 938 -1
- Partials 178 179 +1
Continue to review full report at Codecov.
|
chingor13
left a comment
chingor13
left a comment
There was a problem hiding this comment.
This is less important as it's a test dependency.
| private static class FakeSettings extends ClientSettings { | ||
|
|
||
| public UnaryCallSettings<Integer, Integer> fakeMethodSimple() { | ||
| return ((FakeStubSettings) getStubSettings()).fakeMethodSimple(); | ||
| } | ||
|
|
||
| public PagedCallSettings<Integer, Integer, FakePagedListResponse> fakePagedMethod() { | ||
| return ((FakeStubSettings) getStubSettings()).fakePagedMethod(); | ||
| } | ||
|
|
||
| public BatchingCallSettings<Integer, Integer> fakeMethodBatching() { | ||
| return ((FakeStubSettings) getStubSettings()).fakeMethodBatching(); | ||
| } | ||
|
|
||
| public static Builder newBuilder() { | ||
| return Builder.createDefault(); | ||
| } | ||
|
|
||
| public Builder toBuilder() { | ||
| return new Builder(this); | ||
| } | ||
|
|
||
| private FakeSettings(Builder settingsBuilder) throws IOException { | ||
| super(settingsBuilder); | ||
| } | ||
|
|
||
| private static class Builder extends ClientSettings.Builder { | ||
|
|
||
| protected Builder() throws IOException { | ||
| this((ClientContext) null); | ||
| } | ||
|
|
||
| protected Builder(ClientContext clientContext) { | ||
| super(FakeStubSettings.newBuilder(clientContext)); | ||
| } | ||
|
|
||
| private static Builder createDefault() { | ||
| return new Builder(FakeStubSettings.newBuilder()); | ||
| } | ||
|
|
||
| protected Builder(FakeSettings settings) { | ||
| super(settings.getStubSettings().toBuilder()); | ||
| } | ||
|
|
||
| protected Builder(FakeStubSettings.Builder stubSettings) { | ||
| super(stubSettings); | ||
| } | ||
|
|
||
| public FakeStubSettings.Builder getStubSettingsBuilder() { | ||
| return ((FakeStubSettings.Builder) getStubSettings()); | ||
| } | ||
|
|
||
| public UnaryCallSettings.Builder<Integer, Integer> fakeMethodSimple() { | ||
| return getStubSettingsBuilder().fakeMethodSimple(); | ||
| } | ||
|
|
||
| public PagedCallSettings.Builder<Integer, Integer, FakePagedListResponse> fakePagedMethod() { | ||
| return getStubSettingsBuilder().fakePagedMethod(); | ||
| } | ||
|
|
||
| public BatchingCallSettings.Builder<Integer, Integer> fakeMethodBatching() { | ||
| return getStubSettingsBuilder().fakeMethodBatching(); | ||
| } | ||
|
|
||
| @Override | ||
| public FakeSettings build() throws IOException { | ||
| return new FakeSettings(this); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think these are used to simulate a gapic generated client
There was a problem hiding this comment.
Can you elaborate? It looks to me like this is simply testing that a fake is equal to a fake. I don't see where it tests any model code. What am I not seeing?
There was a problem hiding this comment.
TestSettingsTest#callSettingsBuildFromTimeoutNoRetries() seems like the only legit test here (that one which is not testing fake vs fake), and that one is not completely removed. So the cleanup looks legit to me.
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
FYI, although commons-lang is only a test dependency it seems to interfere with bazel builds when different hashes get pulled in from transitive dependencies so I really would like to get rid of this.
| private static class FakeSettings extends ClientSettings { | ||
|
|
||
| public UnaryCallSettings<Integer, Integer> fakeMethodSimple() { | ||
| return ((FakeStubSettings) getStubSettings()).fakeMethodSimple(); | ||
| } | ||
|
|
||
| public PagedCallSettings<Integer, Integer, FakePagedListResponse> fakePagedMethod() { | ||
| return ((FakeStubSettings) getStubSettings()).fakePagedMethod(); | ||
| } | ||
|
|
||
| public BatchingCallSettings<Integer, Integer> fakeMethodBatching() { | ||
| return ((FakeStubSettings) getStubSettings()).fakeMethodBatching(); | ||
| } | ||
|
|
||
| public static Builder newBuilder() { | ||
| return Builder.createDefault(); | ||
| } | ||
|
|
||
| public Builder toBuilder() { | ||
| return new Builder(this); | ||
| } | ||
|
|
||
| private FakeSettings(Builder settingsBuilder) throws IOException { | ||
| super(settingsBuilder); | ||
| } | ||
|
|
||
| private static class Builder extends ClientSettings.Builder { | ||
|
|
||
| protected Builder() throws IOException { | ||
| this((ClientContext) null); | ||
| } | ||
|
|
||
| protected Builder(ClientContext clientContext) { | ||
| super(FakeStubSettings.newBuilder(clientContext)); | ||
| } | ||
|
|
||
| private static Builder createDefault() { | ||
| return new Builder(FakeStubSettings.newBuilder()); | ||
| } | ||
|
|
||
| protected Builder(FakeSettings settings) { | ||
| super(settings.getStubSettings().toBuilder()); | ||
| } | ||
|
|
||
| protected Builder(FakeStubSettings.Builder stubSettings) { | ||
| super(stubSettings); | ||
| } | ||
|
|
||
| public FakeStubSettings.Builder getStubSettingsBuilder() { | ||
| return ((FakeStubSettings.Builder) getStubSettings()); | ||
| } | ||
|
|
||
| public UnaryCallSettings.Builder<Integer, Integer> fakeMethodSimple() { | ||
| return getStubSettingsBuilder().fakeMethodSimple(); | ||
| } | ||
|
|
||
| public PagedCallSettings.Builder<Integer, Integer, FakePagedListResponse> fakePagedMethod() { | ||
| return getStubSettingsBuilder().fakePagedMethod(); | ||
| } | ||
|
|
||
| public BatchingCallSettings.Builder<Integer, Integer> fakeMethodBatching() { | ||
| return getStubSettingsBuilder().fakeMethodBatching(); | ||
| } | ||
|
|
||
| @Override | ||
| public FakeSettings build() throws IOException { | ||
| return new FakeSettings(this); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Can you elaborate? It looks to me like this is simply testing that a fake is equal to a fake. I don't see where it tests any model code. What am I not seeing?
vam-google
left a comment
vam-google
left a comment
There was a problem hiding this comment.
LGTM.
I never liked that dependency anyways. And the removed testing logic seems really testing fake vs fake (except TestSettingsTest#callSettingsBuildFromTimeoutNoRetries(), but that test is not removed, so it is all good).
(I'm not sure about the transitive dependency issue note, since bazel does not have transitive dependencies and everything is pulled explicitly, but whatever.)
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { |
There was a problem hiding this comment.
Are equals/hascode manually written? Not a big deal but it is better to have equals/hashcode implementation in gax consistent with the others.
| private static class FakeSettings extends ClientSettings { | ||
|
|
||
| public UnaryCallSettings<Integer, Integer> fakeMethodSimple() { | ||
| return ((FakeStubSettings) getStubSettings()).fakeMethodSimple(); | ||
| } | ||
|
|
||
| public PagedCallSettings<Integer, Integer, FakePagedListResponse> fakePagedMethod() { | ||
| return ((FakeStubSettings) getStubSettings()).fakePagedMethod(); | ||
| } | ||
|
|
||
| public BatchingCallSettings<Integer, Integer> fakeMethodBatching() { | ||
| return ((FakeStubSettings) getStubSettings()).fakeMethodBatching(); | ||
| } | ||
|
|
||
| public static Builder newBuilder() { | ||
| return Builder.createDefault(); | ||
| } | ||
|
|
||
| public Builder toBuilder() { | ||
| return new Builder(this); | ||
| } | ||
|
|
||
| private FakeSettings(Builder settingsBuilder) throws IOException { | ||
| super(settingsBuilder); | ||
| } | ||
|
|
||
| private static class Builder extends ClientSettings.Builder { | ||
|
|
||
| protected Builder() throws IOException { | ||
| this((ClientContext) null); | ||
| } | ||
|
|
||
| protected Builder(ClientContext clientContext) { | ||
| super(FakeStubSettings.newBuilder(clientContext)); | ||
| } | ||
|
|
||
| private static Builder createDefault() { | ||
| return new Builder(FakeStubSettings.newBuilder()); | ||
| } | ||
|
|
||
| protected Builder(FakeSettings settings) { | ||
| super(settings.getStubSettings().toBuilder()); | ||
| } | ||
|
|
||
| protected Builder(FakeStubSettings.Builder stubSettings) { | ||
| super(stubSettings); | ||
| } | ||
|
|
||
| public FakeStubSettings.Builder getStubSettingsBuilder() { | ||
| return ((FakeStubSettings.Builder) getStubSettings()); | ||
| } | ||
|
|
||
| public UnaryCallSettings.Builder<Integer, Integer> fakeMethodSimple() { | ||
| return getStubSettingsBuilder().fakeMethodSimple(); | ||
| } | ||
|
|
||
| public PagedCallSettings.Builder<Integer, Integer, FakePagedListResponse> fakePagedMethod() { | ||
| return getStubSettingsBuilder().fakePagedMethod(); | ||
| } | ||
|
|
||
| public BatchingCallSettings.Builder<Integer, Integer> fakeMethodBatching() { | ||
| return getStubSettingsBuilder().fakeMethodBatching(); | ||
| } | ||
|
|
||
| @Override | ||
| public FakeSettings build() throws IOException { | ||
| return new FakeSettings(this); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
TestSettingsTest#callSettingsBuildFromTimeoutNoRetries() seems like the only legit test here (that one which is not testing fake vs fake), and that one is not completely removed. So the cleanup looks legit to me.
4 participants
@chingor13 fixes #1271