From f7077a565aa71f111006e24b50bfe2976646aedd Mon Sep 17 00:00:00 2001 From: ZHANG Dapeng Date: Mon, 10 Jun 2019 14:19:27 -0700 Subject: [PATCH] xds: cleanup XdsLbStateTest The test case `XdsLbStateTest.handleSubchannelState()` was introduced before `LocalityStore` refactored out of `XdsLbState`. After `LocalityStore` refactored out, the test case should not be in `XdsLbStateTest` anymore. The test case is already covered in `LocalityStoreTest`. --- .../java/io/grpc/xds/LocalityStoreTest.java | 76 ++++---- .../test/java/io/grpc/xds/XdsLbStateTest.java | 165 ------------------ 2 files changed, 37 insertions(+), 204 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/LocalityStoreTest.java b/xds/src/test/java/io/grpc/xds/LocalityStoreTest.java index 8377533129..d0eaced113 100644 --- a/xds/src/test/java/io/grpc/xds/LocalityStoreTest.java +++ b/xds/src/test/java/io/grpc/xds/LocalityStoreTest.java @@ -30,11 +30,11 @@ import static org.mockito.Mockito.verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import io.grpc.Attributes; import io.grpc.ChannelLogger; import io.grpc.ConnectivityState; import io.grpc.EquivalentAddressGroup; import io.grpc.LoadBalancer; +import io.grpc.LoadBalancer.CreateSubchannelArgs; import io.grpc.LoadBalancer.Helper; import io.grpc.LoadBalancer.PickResult; import io.grpc.LoadBalancer.PickSubchannelArgs; @@ -51,7 +51,7 @@ import io.grpc.xds.XdsComms.LbEndpoint; import io.grpc.xds.XdsComms.Locality; import io.grpc.xds.XdsComms.LocalityInfo; import java.net.InetSocketAddress; -import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import org.junit.Before; @@ -60,7 +60,6 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; import org.mockito.ArgumentCaptor; -import org.mockito.ArgumentMatchers; import org.mockito.Mock; import org.mockito.junit.MockitoJUnit; import org.mockito.junit.MockitoRule; @@ -69,8 +68,6 @@ import org.mockito.junit.MockitoRule; * Tests for {@link LocalityStore}. */ @RunWith(JUnit4.class) -// TODO(dapengzhang0): remove this after switching to Subchannel.start(). -@SuppressWarnings("deprecation") public class LocalityStoreTest { @Rule public final MockitoRule mockitoRule = MockitoJUnit.rule(); @@ -90,15 +87,11 @@ public class LocalityStoreTest { } }; } - - void setNextIndex(int nextIndex) { - this.nextIndex = nextIndex; - } } private final LoadBalancerRegistry lbRegistry = new LoadBalancerRegistry(); - private final List loadBalancers = new ArrayList<>(); - private final List helpers = new ArrayList<>(); + private final Map loadBalancers = new HashMap<>(); + private final Map childHelpers = new HashMap<>(); private final LoadBalancerProvider lbProvider = new LoadBalancerProvider() { @@ -120,8 +113,8 @@ public class LocalityStoreTest { @Override public LoadBalancer newLoadBalancer(Helper helper) { LoadBalancer fakeLb = mock(LoadBalancer.class); - loadBalancers.add(fakeLb); - helpers.add(helper); + loadBalancers.put(helper.getAuthority(), fakeLb); + childHelpers.put(helper.getAuthority(), helper); return fakeLb; } }; @@ -171,8 +164,7 @@ public class LocalityStoreTest { @Before public void setUp() { doReturn(mock(ChannelLogger.class)).when(helper).getChannelLogger(); - doReturn(mock(Subchannel.class)).when(helper).createSubchannel( - ArgumentMatchers.anyList(), any(Attributes.class)); + doReturn(mock(Subchannel.class)).when(helper).createSubchannel(any(CreateSubchannelArgs.class)); lbRegistry.register(lbProvider); localityStore = new LocalityStoreImpl(helper, pickerFactory, lbRegistry, random); } @@ -191,33 +183,36 @@ public class LocalityStoreTest { localityStore.updateLocalityStore(localityInfoMap); assertThat(loadBalancers).hasSize(3); + assertThat(loadBalancers.keySet()).containsExactly("sz1", "sz2", "sz3"); ArgumentCaptor resolvedAddressesCaptor1 = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(loadBalancers.get(0)).handleResolvedAddresses(resolvedAddressesCaptor1.capture()); + verify(loadBalancers.get("sz1")).handleResolvedAddresses(resolvedAddressesCaptor1.capture()); assertThat(resolvedAddressesCaptor1.getValue().getAddresses()).containsExactly(eag11, eag12); ArgumentCaptor resolvedAddressesCaptor2 = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(loadBalancers.get(1)).handleResolvedAddresses(resolvedAddressesCaptor2.capture()); + verify(loadBalancers.get("sz2")).handleResolvedAddresses(resolvedAddressesCaptor2.capture()); assertThat(resolvedAddressesCaptor2.getValue().getAddresses()).containsExactly(eag21, eag22); ArgumentCaptor resolvedAddressesCaptor3 = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(loadBalancers.get(2)).handleResolvedAddresses(resolvedAddressesCaptor3.capture()); + verify(loadBalancers.get("sz3")).handleResolvedAddresses(resolvedAddressesCaptor3.capture()); assertThat(resolvedAddressesCaptor3.getValue().getAddresses()).containsExactly(eag31, eag32); assertThat(pickerFactory.totalReadyLocalities).isEqualTo(0); verify(helper, never()).updateBalancingState( any(ConnectivityState.class), any(SubchannelPicker.class)); // subchannel12 goes to CONNECTING + CreateSubchannelArgs createSubchannelArgs = + CreateSubchannelArgs.newBuilder().setAddresses(ImmutableList.of(eag12)).build(); final Subchannel subchannel12 = - helpers.get(0).createSubchannel(ImmutableList.of(eag12), Attributes.EMPTY); - verify(helper).createSubchannel(ImmutableList.of(eag12), Attributes.EMPTY); + childHelpers.get("sz1").createSubchannel(createSubchannelArgs); + verify(helper).createSubchannel(createSubchannelArgs); SubchannelPicker subchannelPicker12 = new SubchannelPicker() { @Override public PickResult pickSubchannel(PickSubchannelArgs args) { return PickResult.withSubchannel(subchannel12); } }; - helpers.get(0).updateBalancingState(CONNECTING, subchannelPicker12); + childHelpers.get("sz1").updateBalancingState(CONNECTING, subchannelPicker12); ArgumentCaptor subchannelPickerCaptor12 = ArgumentCaptor.forClass(SubchannelPicker.class); verify(helper).updateBalancingState(same(CONNECTING), subchannelPickerCaptor12.capture()); @@ -226,16 +221,18 @@ public class LocalityStoreTest { .isEqualTo(PickResult.withNoResult()); // subchannel31 goes to READY + createSubchannelArgs = + CreateSubchannelArgs.newBuilder().setAddresses(ImmutableList.of(eag31)).build(); final Subchannel subchannel31 = - helpers.get(2).createSubchannel(ImmutableList.of(eag31), Attributes.EMPTY); - verify(helper).createSubchannel(ImmutableList.of(eag31), Attributes.EMPTY); + childHelpers.get("sz3").createSubchannel(createSubchannelArgs); + verify(helper).createSubchannel(createSubchannelArgs); SubchannelPicker subchannelPicker31 = new SubchannelPicker() { @Override public PickResult pickSubchannel(PickSubchannelArgs args) { return PickResult.withSubchannel(subchannel31); } }; - helpers.get(2).updateBalancingState(READY, subchannelPicker31); + childHelpers.get("sz3").updateBalancingState(READY, subchannelPicker31); ArgumentCaptor subchannelPickerCaptor31 = ArgumentCaptor.forClass(SubchannelPicker.class); verify(helper).updateBalancingState(same(READY), subchannelPickerCaptor31.capture()); @@ -245,7 +242,7 @@ public class LocalityStoreTest { .isEqualTo(subchannel31); // subchannel12 goes to READY - helpers.get(0).updateBalancingState(READY, subchannelPicker12); + childHelpers.get("sz1").updateBalancingState(READY, subchannelPicker12); verify(helper, times(2)).updateBalancingState(same(READY), subchannelPickerCaptor12.capture()); assertThat(pickerFactory.totalReadyLocalities).isEqualTo(2); pickerFactory.nextIndex = 0; @@ -263,15 +260,15 @@ public class LocalityStoreTest { localityStore.updateLocalityStore(localityInfoMap); assertThat(loadBalancers).hasSize(4); - verify(loadBalancers.get(2)).shutdown(); - verify(loadBalancers.get(1), times(2)) + verify(loadBalancers.get("sz3")).shutdown(); + verify(loadBalancers.get("sz2"), times(2)) .handleResolvedAddresses(resolvedAddressesCaptor2.capture()); assertThat(resolvedAddressesCaptor2.getValue().getAddresses()).containsExactly(eag21, eag22); ArgumentCaptor resolvedAddressesCaptor4 = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(loadBalancers.get(3)).handleResolvedAddresses(resolvedAddressesCaptor4.capture()); + verify(loadBalancers.get("sz4")).handleResolvedAddresses(resolvedAddressesCaptor4.capture()); assertThat(resolvedAddressesCaptor4.getValue().getAddresses()).containsExactly(eag41, eag42); - verify(loadBalancers.get(0), times(2)) + verify(loadBalancers.get("sz1"), times(2)) .handleResolvedAddresses(resolvedAddressesCaptor1.capture()); assertThat(resolvedAddressesCaptor1.getValue().getAddresses()).containsExactly(eag11); assertThat(pickerFactory.totalReadyLocalities).isEqualTo(1); @@ -297,15 +294,15 @@ public class LocalityStoreTest { assertThat(loadBalancers).hasSize(3); ArgumentCaptor resolvedAddressesCaptor1 = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(loadBalancers.get(0)).handleResolvedAddresses(resolvedAddressesCaptor1.capture()); + verify(loadBalancers.get("sz1")).handleResolvedAddresses(resolvedAddressesCaptor1.capture()); assertThat(resolvedAddressesCaptor1.getValue().getAddresses()).containsExactly(eag11, eag12); ArgumentCaptor resolvedAddressesCaptor2 = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(loadBalancers.get(1)).handleResolvedAddresses(resolvedAddressesCaptor2.capture()); + verify(loadBalancers.get("sz2")).handleResolvedAddresses(resolvedAddressesCaptor2.capture()); assertThat(resolvedAddressesCaptor2.getValue().getAddresses()).containsExactly(eag21, eag22); ArgumentCaptor resolvedAddressesCaptor3 = ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(loadBalancers.get(2)).handleResolvedAddresses(resolvedAddressesCaptor3.capture()); + verify(loadBalancers.get("sz3")).handleResolvedAddresses(resolvedAddressesCaptor3.capture()); assertThat(resolvedAddressesCaptor3.getValue().getAddresses()).containsExactly(eag31, eag32); assertThat(pickerFactory.totalReadyLocalities).isEqualTo(0); ArgumentCaptor subchannelPickerCaptor = @@ -334,16 +331,17 @@ public class LocalityStoreTest { verify(random, times(times += 2)).nextInt(1000_000); // subchannel12 goes to READY - final Subchannel subchannel12 = - helpers.get(0).createSubchannel(ImmutableList.of(eag12), Attributes.EMPTY); - verify(helper).createSubchannel(ImmutableList.of(eag12), Attributes.EMPTY); + CreateSubchannelArgs createSubchannelArgs = + CreateSubchannelArgs.newBuilder().setAddresses(ImmutableList.of(eag12)).build(); + final Subchannel subchannel12 = childHelpers.get("sz1").createSubchannel(createSubchannelArgs); + verify(helper).createSubchannel(createSubchannelArgs); SubchannelPicker subchannelPicker12 = new SubchannelPicker() { @Override public PickResult pickSubchannel(PickSubchannelArgs args) { return PickResult.withSubchannel(subchannel12); } }; - helpers.get(0).updateBalancingState(READY, subchannelPicker12); + childHelpers.get("sz1").updateBalancingState(READY, subchannelPicker12); ArgumentCaptor subchannelPickerCaptor12 = ArgumentCaptor.forClass(SubchannelPicker.class); verify(helper).updateBalancingState(same(READY), subchannelPickerCaptor12.capture()); @@ -366,7 +364,7 @@ public class LocalityStoreTest { doReturn(365, 1233).when(random).nextInt(1000_000); assertThat(subchannelPickerCaptor12.getValue().pickSubchannel(pickSubchannelArgs).isDrop()) .isTrue(); - verify(random, times(times += 2)).nextInt(1000_000); + verify(random, times(times + 2)).nextInt(1000_000); } @Test @@ -407,7 +405,7 @@ public class LocalityStoreTest { localityStore.reset(); - verify(loadBalancers.get(0)).shutdown(); - verify(loadBalancers.get(1)).shutdown(); + verify(loadBalancers.get("sz1")).shutdown(); + verify(loadBalancers.get("sz2")).shutdown(); } } diff --git a/xds/src/test/java/io/grpc/xds/XdsLbStateTest.java b/xds/src/test/java/io/grpc/xds/XdsLbStateTest.java index 78d950a3a0..30cc1588a4 100644 --- a/xds/src/test/java/io/grpc/xds/XdsLbStateTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsLbStateTest.java @@ -17,16 +17,9 @@ package io.grpc.xds; import static com.google.common.truth.Truth.assertThat; -import static io.grpc.ConnectivityState.CONNECTING; -import static io.grpc.ConnectivityState.READY; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyNoMoreInteractions; import com.google.common.collect.ImmutableList; import io.envoyproxy.envoy.api.v2.DiscoveryRequest; @@ -34,17 +27,8 @@ import io.envoyproxy.envoy.api.v2.DiscoveryResponse; import io.envoyproxy.envoy.service.discovery.v2.AggregatedDiscoveryServiceGrpc.AggregatedDiscoveryServiceImplBase; import io.grpc.Attributes; import io.grpc.ChannelLogger; -import io.grpc.ConnectivityState; import io.grpc.EquivalentAddressGroup; -import io.grpc.LoadBalancer; import io.grpc.LoadBalancer.Helper; -import io.grpc.LoadBalancer.PickResult; -import io.grpc.LoadBalancer.PickSubchannelArgs; -import io.grpc.LoadBalancer.ResolvedAddresses; -import io.grpc.LoadBalancer.Subchannel; -import io.grpc.LoadBalancer.SubchannelPicker; -import io.grpc.LoadBalancerProvider; -import io.grpc.LoadBalancerRegistry; import io.grpc.ManagedChannel; import io.grpc.SynchronizationContext; import io.grpc.inprocess.InProcessChannelBuilder; @@ -53,26 +37,12 @@ import io.grpc.internal.FakeClock; import io.grpc.internal.testing.StreamRecorder; import io.grpc.stub.StreamObserver; import io.grpc.testing.GrpcCleanupRule; -import io.grpc.xds.InterLocalityPicker.WeightedChildPicker; -import io.grpc.xds.LocalityStore.LocalityStoreImpl; -import io.grpc.xds.LocalityStore.LocalityStoreImpl.PickerFactory; import io.grpc.xds.XdsComms.AdsStreamCallback; -import io.grpc.xds.XdsComms.LbEndpoint; -import io.grpc.xds.XdsComms.Locality; -import io.grpc.xds.XdsComms.LocalityInfo; -import java.net.InetSocketAddress; -import java.util.HashMap; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; -import org.mockito.ArgumentCaptor; -import org.mockito.Captor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -88,46 +58,6 @@ public class XdsLbStateTest { private Helper helper; @Mock private AdsStreamCallback adsStreamCallback; - @Mock - private PickSubchannelArgs pickSubchannelArgs; - @Mock - private ThreadSafeRandom random; - @Captor - private ArgumentCaptor subchannelPickerCaptor; - @Captor - private ArgumentCaptor resolvedAddressesCaptor; - - private final LoadBalancerRegistry lbRegistry = new LoadBalancerRegistry(); - private final Map loadBalancers = new HashMap<>(); - private final Map childHelpers = new HashMap<>(); - - private final LoadBalancerProvider childLbProvider = new LoadBalancerProvider() { - @Override - public boolean isAvailable() { - return true; - } - - @Override - public int getPriority() { - return 5; - } - - @Override - public String getPolicyName() { - return "round_robin"; - } - - @Override - public LoadBalancer newLoadBalancer(Helper helper) { - if (loadBalancers.containsKey(helper.getAuthority())) { - return loadBalancers.get(helper.getAuthority()); - } - LoadBalancer loadBalancer = mock(LoadBalancer.class); - loadBalancers.put(helper.getAuthority(), loadBalancer); - childHelpers.put(helper.getAuthority(), helper); - return loadBalancer; - } - }; private LocalityStore localityStore; @@ -144,29 +74,6 @@ public class XdsLbStateTest { private final StreamRecorder streamRecorder = StreamRecorder.create(); private ManagedChannel channel; - private static final class FakeInterLocalityPickerFactory implements PickerFactory { - int totalReadyLocalities; - int nextIndex; - - @Override - public SubchannelPicker picker(final List childPickers) { - totalReadyLocalities = childPickers.size(); - - return new SubchannelPicker() { - @Override - public PickResult pickSubchannel(PickSubchannelArgs args) { - return childPickers.get(nextIndex).getPicker().pickSubchannel(args); - } - }; - } - - void setNextIndex(int nextIndex) { - this.nextIndex = nextIndex; - } - } - - private final FakeInterLocalityPickerFactory interLocalityPickerFactory - = new FakeInterLocalityPickerFactory(); @Before public void setUp() throws Exception { @@ -175,8 +82,6 @@ public class XdsLbStateTest { doReturn(fakeClock.getScheduledExecutorService()).when(helper).getScheduledExecutorService(); doReturn("fake_authority").when(helper).getAuthority(); doReturn(mock(ChannelLogger.class)).when(helper).getChannelLogger(); - lbRegistry.register(childLbProvider); - localityStore = new LocalityStoreImpl(helper, interLocalityPickerFactory, lbRegistry, random); String serverName = InProcessServerBuilder.generateName(); @@ -217,11 +122,6 @@ public class XdsLbStateTest { doReturn(channel).when(helper).createResolvingOobChannel(BALANCER_NAME); } - @After - public void tearDown() { - verifyNoMoreInteractions(random); - } - @Test public void shutdownAndReleaseXdsCommsDoesShutdown() { XdsLbState xdsLbState = mock(XdsLbState.class); @@ -229,71 +129,6 @@ public class XdsLbStateTest { verify(xdsLbState).shutdown(); } - @Test - public void handleSubchannelState() { - assertThat(loadBalancers).isEmpty(); - - Locality locality1 = new Locality("r1", "z1", "sz1"); - EquivalentAddressGroup eag11 = new EquivalentAddressGroup(new InetSocketAddress("addr11", 11)); - EquivalentAddressGroup eag12 = new EquivalentAddressGroup(new InetSocketAddress("addr12", 12)); - - LbEndpoint lbEndpoint11 = new LbEndpoint(eag11, 11); - LbEndpoint lbEndpoint12 = new LbEndpoint(eag12, 12); - LocalityInfo localityInfo1 = new LocalityInfo(ImmutableList.of(lbEndpoint11, lbEndpoint12), 1); - - Locality locality2 = new Locality("r2", "z2", "sz2"); - EquivalentAddressGroup eag21 = new EquivalentAddressGroup(new InetSocketAddress("addr21", 21)); - EquivalentAddressGroup eag22 = new EquivalentAddressGroup(new InetSocketAddress("addr22", 22)); - - LbEndpoint lbEndpoint21 = new LbEndpoint(eag21, 21); - LbEndpoint lbEndpoint22 = new LbEndpoint(eag22, 22); - LocalityInfo localityInfo2 = new LocalityInfo(ImmutableList.of(lbEndpoint21, lbEndpoint22), 2); - - Map localityInfoMap = new LinkedHashMap<>(); - localityInfoMap.put(locality1, localityInfo1); - localityInfoMap.put(locality2, localityInfo2); - - verify(helper, never()).updateBalancingState( - any(ConnectivityState.class), any(SubchannelPicker.class)); - - localityStore.updateLocalityStore(localityInfoMap); - - assertThat(loadBalancers).hasSize(2); - assertThat(loadBalancers.keySet()).containsExactly("sz1", "sz2"); - assertThat(childHelpers).hasSize(2); - assertThat(childHelpers.keySet()).containsExactly("sz1", "sz2"); - - verify(loadBalancers.get("sz1")).handleResolvedAddresses(resolvedAddressesCaptor.capture()); - assertThat(resolvedAddressesCaptor.getValue().getAddresses()) - .containsExactly(eag11, eag12).inOrder(); - verify(loadBalancers.get("sz2")).handleResolvedAddresses(resolvedAddressesCaptor.capture()); - assertThat(resolvedAddressesCaptor.getValue().getAddresses()) - .containsExactly(eag21, eag22).inOrder(); - verify(helper, never()).updateBalancingState( - any(ConnectivityState.class), any(SubchannelPicker.class)); - - SubchannelPicker childPicker1 = mock(SubchannelPicker.class); - PickResult pickResult1 = PickResult.withSubchannel(mock(Subchannel.class)); - doReturn(pickResult1).when(childPicker1).pickSubchannel(any(PickSubchannelArgs.class)); - childHelpers.get("sz1").updateBalancingState(READY, childPicker1); - verify(helper).updateBalancingState(eq(READY), subchannelPickerCaptor.capture()); - - assertThat(interLocalityPickerFactory.totalReadyLocalities).isEqualTo(1); - interLocalityPickerFactory.setNextIndex(0); - assertThat(subchannelPickerCaptor.getValue().pickSubchannel(pickSubchannelArgs)) - .isSameInstanceAs(pickResult1); - - SubchannelPicker childPicker2 = mock(SubchannelPicker.class); - PickResult pickResult2 = PickResult.withSubchannel(mock(Subchannel.class)); - doReturn(pickResult2).when(childPicker2).pickSubchannel(any(PickSubchannelArgs.class)); - childHelpers.get("sz2").updateBalancingState(CONNECTING, childPicker2); - verify(helper, times(2)).updateBalancingState(eq(READY), subchannelPickerCaptor.capture()); - - assertThat(interLocalityPickerFactory.totalReadyLocalities).isEqualTo(1); - assertThat(subchannelPickerCaptor.getValue().pickSubchannel(pickSubchannelArgs)) - .isSameInstanceAs(pickResult1); - } - @Test public void handleResolvedAddressGroupsThenShutdown() throws Exception { localityStore = mock(LocalityStore.class);