[TEST] Make sure we restart the suite cluster after each test failure

CurrentTestFailedMarker is a RunListener that gets notified whenever a test fails, and we were using it to be able to restart the suite cluster after each failure. We were checking whether a test had failed in the @after method though, which runs before the listener gets notified, so the failed flag would always be false.

This commit makes sure that the suite cluster gets restarted not only when there are problems in the afterInternal method, but also after each test failure. In order to achieve this, we need to reset the cluster afterwards, when we get to know about both of the events (problem in afterInternal and test failure), and before resetting the currentCluster. Introduced a TestRule that keeps track of test failures and allows to execute arbitrary tasks when a test fails and when a test is completed (regardless of its result). Allows also to force the execution of the failure task (used in case of afterInternal issues rather than actual test failure).

Also updated ElasticsearchRestTests to make sure that the RestClient gets re-initialized in case we restart the suite cluster, otherwise all the subsequent tests fail. Improved this mechanism also to relate it directly to the restart of the cluster instead of checking whether the addresses have changed, which doesn't work anyway as the new cluster will use the same addresses but the client needs to be recreated anyway.

Closes #9015

src/test/java/org/apache/lucene/util/AbstractRandomizedTest.java

@@ -31,7 +31,7 @@
 import org.elasticsearch.common.lucene.Lucene;
 import org.elasticsearch.common.settings.ImmutableSettings;
 import org.elasticsearch.common.util.concurrent.EsExecutors;
-import org.elasticsearch.test.CurrentTestFailedMarker;
+import org.elasticsearch.test.AfterTestRule;
 import org.elasticsearch.test.ElasticsearchIntegrationTest;
 import org.elasticsearch.test.ElasticsearchTestCase;
 import org.elasticsearch.test.junit.listeners.ReproduceInfoPrinter;
@@ -44,7 +44,6 @@
 import org.junit.runner.RunWith;
 
 import java.io.Closeable;
-import java.io.File;
 import java.io.IOException;
 import java.lang.annotation.*;
 import java.lang.reflect.Method;
@@ -61,8 +60,7 @@
 })
 @Listeners({
         ReproduceInfoPrinter.class,
-        FailureMarker.class,
-        CurrentTestFailedMarker.class
+        FailureMarker.class
 })
 @RunWith(value = com.carrotsearch.randomizedtesting.RandomizedRunner.class)
 @SuppressCodecs(value = "Lucene3x")
@@ -256,6 +254,8 @@ public abstract class AbstractRandomizedTest extends RandomizedTest {
     private static final AtomicReference<TestRuleIgnoreAfterMaxFailures> ignoreAfterMaxFailuresDelegate;
     private static final TestRule ignoreAfterMaxFailures;
 
+    private static final AfterTestRule.Task noOpAfterRuleTask = new AfterTestRule.Task();
+
     static {
         int maxFailures = systemPropertyAsInt(SYSPROP_MAXFAILURES, Integer.MAX_VALUE);
         boolean failFast = systemPropertyAsBoolean(SYSPROP_FAILFAST, false);
@@ -360,6 +360,8 @@ protected boolean verify(Method key) {
      */
     private TestRuleMarkFailure testFailureMarker = new TestRuleMarkFailure(suiteFailureMarker);
 
+    protected AfterTestRule afterTestRule = new AfterTestRule(afterTestTask());
+
     /**
      * This controls how individual test rules are nested. It is important that
      * _all_ rules declared in {@link LuceneTestCase} are executed in proper order
@@ -372,7 +374,8 @@ protected boolean verify(Method key) {
             .around(threadAndTestNameRule)
             .around(new SystemPropertiesInvariantRule(IGNORED_INVARIANT_PROPERTIES))
             .around(new TestRuleSetupAndRestoreInstanceEnv())
-            .around(parentChainCallRule);
+            .around(parentChainCallRule)
+            .around(afterTestRule);
 
     // -----------------------------------------------------------------
     // Suite and test case setup/ cleanup.
@@ -438,4 +441,8 @@ public static Class<?> getTestClass() {
     public String getTestName() {
         return threadAndTestNameRule.testMethodName;
     }
+
+    protected AfterTestRule.Task afterTestTask() {
+        return noOpAfterRuleTask;
+    }
 }

src/test/java/org/elasticsearch/test/AfterTestRule.java

@@ -0,0 +1,80 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor
+ * license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright
+ * ownership. Elasticsearch licenses this file to you under
+ * the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.elasticsearch.test;
+
+import org.junit.rules.TestWatcher;
+import org.junit.runner.Description;
+
+import java.util.concurrent.atomic.AtomicBoolean;
+
+/**
+ * A {@link org.junit.rules.TestRule} that detects test failures and allows to run an arbitrary task after a test failed.
+ * Allows also to run an arbitrary task in any case, regardless of the test result.
+ * It is possible to force running the first arbitrary task from the outside, as if the test was failed, when e.g. it needs
+ * to be performed based on external events.
+ *
+ * We need it to be able to reset the suite level cluster after each failure, or if there is a problem
+ * during the after test operations.
+ */
+public class AfterTestRule extends TestWatcher {
+
+    private final AtomicBoolean failed = new AtomicBoolean(false);
+
+    private final Task task;
+
+    public AfterTestRule(Task task) {
+        this.task = task;
+    }
+
+    void forceFailure() {
+        failed.set(true);
+    }
+
+    @Override
+    protected void failed(Throwable e, Description description) {
+        failed.set(true);
+    }
+
+    @Override
+    protected void finished(Description description) {
+        if (failed.compareAndSet(true, false)) {
+            task.onTestFailed();
+        }
+        task.onTestFinished();
+    }
+
+    /**
+     * Task to be executed after each test if required, no-op by default
+     */
+    public static class Task {
+        /**
+         * The task that needs to be executed after a test fails
+         */
+        void onTestFailed() {
+
+        }
+
+        /**
+         * The task that needs to be executed when a test is completed, regardless of its result
+         */
+        void onTestFinished() {
+
+        }
+    }
+}

src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java

@@ -566,7 +566,7 @@ private TestCluster buildAndPutCluster(Scope currentClusterScope, long seed) thr
         return testCluster;
     }
 
-   private static void clearClusters() throws IOException {
+    private static void clearClusters() throws IOException {
         if (!clusters.isEmpty()) {
             IOUtils.close(clusters.values());
             clusters.clear();
@@ -603,22 +603,58 @@ protected final void afterInternal(boolean afterClass) throws Exception {
             logger.info("[{}#{}]: cleaned up after test", getTestClass().getSimpleName(), getTestName());
             success = true;
         } finally {
-            // TODO: it looks like CurrentTestFailedMarker is never set at this point, so testFailed() will always be false?
-            if (!success || CurrentTestFailedMarker.testFailed()) {
-                // if we failed that means that something broke horribly so we should
-                // clear all clusters. we also reset everything in the case we had a failure
-                // in the suite to make sure subsequent tests get a new / clean cluster
-                clearClusters();
-                currentCluster = null;
-            }
-            if (currentCluster != null) {
-                // this can be null if the test fails due to static initialization ie. missing parameter on the cmd
-                currentCluster.afterTest();
-                currentCluster = null;
+            if (!success) {
+                // if we failed here that means that something broke horribly so we should clear all clusters
+                afterTestRule.forceFailure();
             }
         }
     }
 
+    @Override
+    protected final AfterTestRule.Task afterTestTask() {
+        return new AfterTestRule.Task() {
+            @Override
+            public void onTestFailed () {
+                //we can't clear clusters after failure when using suite scoped tests, as we would need to call again
+                //initializeSuiteScope but that is static and can only be called from beforeClass
+                if (runTestScopeLifecycle()) {
+                    // If there was a problem during the afterTest, we clear all clusters.
+                    // We do the same in case we just had a test failure to make sure subsequent
+                    // tests get a new / clean cluster
+                    try {
+                        clearClusters();
+                    } catch (IOException e) {
+                        throw new RuntimeException("unable to clear clusters", e);
+                    }
+                    afterTestFailed();
+                    currentCluster = null;
+                }
+            }
+
+            @Override
+            public void onTestFinished () {
+                if (runTestScopeLifecycle()) {
+                    if (currentCluster != null) {
+                        // this can be null if the test fails due to static initialization ie. missing parameter on the cmd
+                        try {
+                            currentCluster.afterTest();
+                        } catch (IOException e) {
+                            throw new RuntimeException("error during afterTest", e);
+                        }
+                        currentCluster = null;
+                    }
+                }
+            }
+        };
+    }
+
+    /**
+     * Allows to execute some additional task after a test is failed, right after we cleared the clusters
+     */
+    protected void afterTestFailed() {
+
+    }
+
     public static TestCluster cluster() {
         return currentCluster;
     }

src/test/java/org/elasticsearch/test/rest/ElasticsearchRestTests.java

@@ -254,7 +254,7 @@ public void reset() throws IOException, RestException {
             assumeFalse("[" + testCandidate.getTestPath() + "] skipped, reason: blacklisted", blacklistedPathMatcher.matches(Paths.get(testPath)));
         }
         //The client needs non static info to get initialized, therefore it can't be initialized in the before class
-        restTestExecutionContext.resetClient(cluster().httpAddresses(), restClientSettings());
+        restTestExecutionContext.initClient(cluster().httpAddresses(), restClientSettings());
         restTestExecutionContext.clear();
 
         //skip test if the whole suite (yaml file) is disabled
@@ -265,6 +265,12 @@ public void reset() throws IOException, RestException {
                 testCandidate.getTestSection().getSkipSection().skip(restTestExecutionContext.esVersion()));
     }
 
+    @Override
+    protected void afterTestFailed() {
+        //after we reset the global cluster, we have to make sure the client gets re-initialized too
+        restTestExecutionContext.resetClient();
+    }
+
     private static String buildSkipMessage(String description, SkipSection skipSection) {
         StringBuilder messageBuilder = new StringBuilder();
         if (skipSection.isVersionCheck()) {

src/test/java/org/elasticsearch/test/rest/RestTestExecutionContext.java

@@ -19,7 +19,6 @@
 package org.elasticsearch.test.rest;
 
 import com.google.common.collect.Maps;
-import com.google.common.collect.Sets;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.logging.Loggers;
 import org.elasticsearch.common.settings.Settings;
@@ -35,7 +34,6 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 /**
  * Execution context passed across the REST tests.
@@ -118,23 +116,19 @@ public Object response(String path) throws IOException {
     }
 
     /**
-     * Creates or updates the embedded REST client when needed. Needs to be called before each test.
+     * Creates the embedded REST client when needed. Needs to be called before each test.
      */
-    public void resetClient(InetSocketAddress[] addresses, Settings settings) throws IOException, RestException {
+    public void initClient(InetSocketAddress[] addresses, Settings settings) throws IOException, RestException {
         if (restClient == null) {
             restClient = new RestClient(restSpec, settings, addresses);
-        } else {
-            //re-initialize the REST client if the addresses have changed
-            //happens if there's a failure since we restart the suite cluster due to that
-            Set<InetSocketAddress> newAddresses = Sets.newHashSet(addresses);
-            Set<InetSocketAddress> previousAddresses = Sets.newHashSet(restClient.httpAddresses());
-            if (!newAddresses.equals(previousAddresses)) {
-                restClient.close();
-                restClient = new RestClient(restSpec, settings, addresses);
-            }
         }
     }
 
+    public void resetClient() {
+        restClient.close();
+        restClient = null;
+    }
+
     /**
      * Clears the last obtained response and the stashed fields
      */

src/test/java/org/elasticsearch/test/rest/client/RestClient.java

@@ -224,10 +224,6 @@ protected CloseableHttpClient createHttpClient() {
         return HttpClients.createMinimal(new PoolingHttpClientConnectionManager(15, TimeUnit.SECONDS));
     }
 
-    public InetSocketAddress[] httpAddresses() {
-        return addresses;
-    }
-
     /**
      * Closes the REST client and the underlying http client
      */