From 29570668553f934a567042ab90c04903547557cd Mon Sep 17 00:00:00 2001 From: javanna Date: Fri, 19 Dec 2014 16:10:05 +0100 Subject: [PATCH] [TEST] Make sure we restart the global 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 global 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 global 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 global 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 --- .../lucene/util/AbstractRandomizedTest.java | 17 ++-- .../org/elasticsearch/test/AfterTestRule.java | 80 +++++++++++++++++++ .../test/CurrentTestFailedMarker.java | 52 ------------ .../test/ElasticsearchIntegrationTest.java | 62 +++++++++++--- .../test/rest/ElasticsearchRestTests.java | 8 +- .../test/rest/RestTestExecutionContext.java | 20 ++--- .../test/rest/client/RestClient.java | 4 - 7 files changed, 155 insertions(+), 88 deletions(-) create mode 100644 src/test/java/org/elasticsearch/test/AfterTestRule.java delete mode 100644 src/test/java/org/elasticsearch/test/CurrentTestFailedMarker.java diff --git a/src/test/java/org/apache/lucene/util/AbstractRandomizedTest.java b/src/test/java/org/apache/lucene/util/AbstractRandomizedTest.java index 695e367a1ee4d..d5cbdf8b0763f 100644 --- a/src/test/java/org/apache/lucene/util/AbstractRandomizedTest.java +++ b/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 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; + } } diff --git a/src/test/java/org/elasticsearch/test/AfterTestRule.java b/src/test/java/org/elasticsearch/test/AfterTestRule.java new file mode 100644 index 0000000000000..e1a25690c5801 --- /dev/null +++ b/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() { + + } + } +} diff --git a/src/test/java/org/elasticsearch/test/CurrentTestFailedMarker.java b/src/test/java/org/elasticsearch/test/CurrentTestFailedMarker.java deleted file mode 100644 index 24e1e6034f43b..0000000000000 --- a/src/test/java/org/elasticsearch/test/CurrentTestFailedMarker.java +++ /dev/null @@ -1,52 +0,0 @@ -/* - * 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.runner.Description; -import org.junit.runner.notification.Failure; -import org.junit.runner.notification.RunListener; - -import java.util.concurrent.atomic.AtomicBoolean; - -/** - * A {@link RunListener} that detects test failures. We need it because we need - * to reset the suite level cluster if a test fails but don't wanna reset it - * for every subsequent test. - */ -public class CurrentTestFailedMarker extends RunListener { - - private static final AtomicBoolean failed = new AtomicBoolean(false); - - @Override - public void testFailure(Failure failure) throws Exception { - failed.set(true); - } - - @Override - public void testStarted(Description description) throws Exception { - failed.set(false); - } - - /** - * Returns true iff the previously executed test failed. Otherwise false - */ - public static boolean testFailed() { - return failed.get(); - } -} diff --git a/src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java b/src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java index cb660bb4ed1cc..c21a81db58565 100644 --- a/src/test/java/org/elasticsearch/test/ElasticsearchIntegrationTest.java +++ b/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; } diff --git a/src/test/java/org/elasticsearch/test/rest/ElasticsearchRestTests.java b/src/test/java/org/elasticsearch/test/rest/ElasticsearchRestTests.java index b7a44f785a6ca..0267b3d9cc893 100644 --- a/src/test/java/org/elasticsearch/test/rest/ElasticsearchRestTests.java +++ b/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()) { diff --git a/src/test/java/org/elasticsearch/test/rest/RestTestExecutionContext.java b/src/test/java/org/elasticsearch/test/rest/RestTestExecutionContext.java index 1b022b552bae9..26dbf7547b808 100644 --- a/src/test/java/org/elasticsearch/test/rest/RestTestExecutionContext.java +++ b/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 newAddresses = Sets.newHashSet(addresses); - Set 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 */ diff --git a/src/test/java/org/elasticsearch/test/rest/client/RestClient.java b/src/test/java/org/elasticsearch/test/rest/client/RestClient.java index 1de7b24cb91c3..4ac0cc270d3e9 100644 --- a/src/test/java/org/elasticsearch/test/rest/client/RestClient.java +++ b/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 */