Fix ServerStatusChangeListenerTest race condition.

The changeServerStatusByPingShouldBeReceivedByListener() test
was inconsistent due to the BaseLoadBalancer triggering a ping
task in the constructor. That ping task interferes with testing
listeners in two ways:
1. it may cause a forced ping to get dropped because the task may actively
   be pinging at the moment.
2. even if our forced ping is active, nothing guarantees that the ping task
   will not alter the state of the listener before we assert() our invariants.

ribbon-loadbalancer/src/main/java/com/netflix/loadbalancer/BaseLoadBalancer.java

@@ -234,7 +234,7 @@ private boolean canSkipPing() {
         }
     }
 
-    private void setupPingTask() {
+    void setupPingTask() {
         if (canSkipPing()) {
             return;
         }
@@ -630,13 +630,11 @@ public void run() {
     class Pinger {
 
         public void runPinger() {
-
             if (pingInProgress.get()) {
                 return; // Ping in progress - nothing to do
             } else {
                 pingInProgress.set(true);
             }
-
             // we are "in" - we get to Ping
 
             Object[] allServers = null;
@@ -710,7 +708,6 @@ public void runPinger() {
                         newUpList.add(svr);
                     }
                 }
-                // System.out.println(count + " servers alive");
                 upLock = upServerLock.writeLock();
                 upLock.lock();
                 upServerList = newUpList;

ribbon-loadbalancer/src/test/java/com/netflix/loadbalancer/ServerStatusChangeListenerTest.java

@@ -20,17 +20,33 @@
 import static org.hamcrest.CoreMatchers.hasItem;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.core.AllOf.allOf;
-import static org.junit.Assert.assertThat;
+import static org.junit.Assert.*;
 
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
 
 import org.junit.Before;
 import org.junit.Test;
 
 public class ServerStatusChangeListenerTest {
+
+    /**
+     * A load balancer that has all the functionality of the Base one except it does not
+     * schedule any ping tasks that can concurrently update the state of the LB by either
+     * causing forced pings to be dropped or changing the expected state of the test before
+     * we assert our invariants.
+     */
+    private class NoPingTaskLoadBalancer extends BaseLoadBalancer {
+        @Override
+        void setupPingTask() {}
+    }
+
     private final Server server1 = new Server("server1");
     private final Server server2 = new Server("server2");
 
@@ -39,7 +55,7 @@ public class ServerStatusChangeListenerTest {
 
     @Before
     public void setupLoadbalancerAndListener() {
-        lb = new BaseLoadBalancer();
+        lb = new NoPingTaskLoadBalancer();
         lb.setServersList(asList(server1, server2));
         serversReceivedByListener = new AtomicReference<List<Server>>();
         lb.addServerStatusChangeListener(new ServerStatusChangeListener() {
@@ -66,18 +82,22 @@ public void markServerDownByObjectShouldBeReceivedByListener() {
         assertThat(serversReceivedByListener.get(), is(singletonList(server2)));
     }
 
-
     @Test
-    public void changeServerStatusByPingShouldBeReceivedByListener() {
+    public void changeServerStatusByPingShouldBeReceivedByListener() throws InterruptedException {
         final PingConstant ping = new PingConstant();
-        lb.setPing(ping);
-
+        // Start with a ping where both servers are down.
         ping.setConstant(false);
+        lb.setPing(ping);
         lb.forceQuickPing();
+        // We should see that the servers that changed status are both server1 and 2.
         assertThat(serversReceivedByListener.get(), allOf(hasItem(server1), hasItem(server2)));
 
+        // Bring both servers back up.
         ping.setConstant(true);
+        // Clear the list so we can see the change-list is non-empty after the listener is called.
+        serversReceivedByListener.set(null);
         lb.forceQuickPing();
+        assertFalse(serversReceivedByListener.get().isEmpty());
         assertThat(serversReceivedByListener.get(), allOf(hasItem(server1), hasItem(server2)));
     }