SOLR-11456: Fix HealthCheckHandlerTest's bogus asserts and timing/order related flakiness

solr/core/src/test/org/apache/solr/cloud/HealthCheckHandlerTest.java

@@ -18,7 +18,6 @@
 package org.apache.solr.cloud;
 
 import java.io.IOException;
-import java.util.Set;
 
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.SolrResponse;
@@ -31,7 +30,7 @@ import org.apache.solr.client.solrj.response.HealthCheckResponse;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.params.CommonParams;
 import org.apache.solr.common.params.ModifiableSolrParams;
-import org.apache.zookeeper.KeeperException;
+
 import org.junit.BeforeClass;
 import org.junit.Test;
 
@@ -46,33 +45,55 @@ public class HealthCheckHandlerTest extends SolrCloudTestCase {
   }
 
   @Test
-  public void testHealthCheckHandler() throws IOException, SolrServerException, InterruptedException, KeeperException {
+  public void testHealthCheckHandler() throws Exception {
     SolrRequest req = new GenericSolrRequest(SolrRequest.METHOD.GET, HEALTH_CHECK_HANDLER_PATH, new ModifiableSolrParams());
+
+    // positive check that our only existing "healthy" node works with cloud client
+    // NOTE: this is using GenericSolrRequest, not HealthCheckRequest which is why it passes
+    // as compared with testHealthCheckHandlerWithCloudClient
+    // (Not sure if that's actaully a good thing -- but it's how the existing test worked)
+    assertEquals(CommonParams.OK,
+                 req.process(cluster.getSolrClient()).getResponse().get(CommonParams.STATUS));
+    
+    // positive check that our exiting "healthy" node works with direct http client
     try (HttpSolrClient httpSolrClient = getHttpSolrClient(cluster.getJettySolrRunner(0).getBaseUrl().toString())) {
-      SolrResponse response = req.process(cluster.getSolrClient());
+      SolrResponse response = req.process(httpSolrClient);
       assertEquals(CommonParams.OK, response.getResponse().get(CommonParams.STATUS));
-
-      JettySolrRunner jetty = cluster.getJettySolrRunner(0);
-      cluster.expireZkSession(jetty);
-      Set<String> live_nodes = cluster.getSolrClient().getZkStateReader().getClusterState().getLiveNodes();
-
-      int counter = 0;
-      while (live_nodes.size() == 1 && counter++ < 100) {
-        Thread.sleep(100);
-        live_nodes = cluster.getSolrClient().getZkStateReader().getClusterState().getLiveNodes();
-      }
-
-      try {
-        req.process(httpSolrClient);
-      } catch (HttpSolrClient.RemoteSolrException e) {
-        assertTrue(e.getMessage(), e.getMessage().contains("Host Unavailable"));
-        assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, e.code());
-      }
     }
+
+    // add a new node for the purpose of negative testing
+    JettySolrRunner newJetty = cluster.startJettySolrRunner();
+    try (HttpSolrClient httpSolrClient = getHttpSolrClient(newJetty.getBaseUrl().toString())) {
+    
+      // postive check that our (new) "healthy" node works with direct http client
+      assertEquals(CommonParams.OK, req.process(httpSolrClient).getResponse().get(CommonParams.STATUS));
+                  
+      // now "break" our (new) node
+      newJetty.getCoreContainer().getZkController().getZkClient().close();
+      
+      // negative check of our (new) "broken" node that we deliberately put into an unhealth state
+      HttpSolrClient.RemoteSolrException e = expectThrows(HttpSolrClient.RemoteSolrException.class, () ->
+                                                          {
+                                                            req.process(httpSolrClient);
+                                                          });
+      assertTrue(e.getMessage(), e.getMessage().contains("Host Unavailable"));
+      assertEquals(SolrException.ErrorCode.SERVICE_UNAVAILABLE.code, e.code());
+    } finally {
+      newJetty.stop();
+    }
+
+    // (redundent) positive check that our (previously) exiting "healthy" node (still) works
+    // after getting negative results from our broken node
+    try (HttpSolrClient httpSolrClient = getHttpSolrClient(cluster.getJettySolrRunner(0).getBaseUrl().toString())) {
+
+      assertEquals(CommonParams.OK, req.process(httpSolrClient).getResponse().get(CommonParams.STATUS));
+    }
+    
   }
 
   @Test
   public void testHealthCheckHandlerSolrJ() throws IOException, SolrServerException {
+    // positive check of a HealthCheckRequest using http client
     HealthCheckRequest req = new HealthCheckRequest();
     try (HttpSolrClient httpSolrClient = getHttpSolrClient(cluster.getJettySolrRunner(0).getBaseUrl().toString())) {
       HealthCheckResponse rsp = req.process(httpSolrClient);
@@ -82,6 +103,7 @@ public class HealthCheckHandlerTest extends SolrCloudTestCase {
 
   @Test (expected = AssertionError.class)
   public void testHealthCheckHandlerWithCloudClient() throws IOException, SolrServerException {
+    // negative check of a HealthCheckRequest using cloud solr client
     HealthCheckRequest req = new HealthCheckRequest();
     req.process(cluster.getSolrClient());
   }