From ae01a51b44c1bc4f0bbcfc8904b00bf48f450c96 Mon Sep 17 00:00:00 2001 From: Luca Cavanna Date: Wed, 21 Dec 2016 11:44:24 +0100 Subject: [PATCH] [TEST] make ESSingleNodeTestCase tests repeatable (#22283) If we conditionally do random things, e.g. initialize a node only after the first test, we have to make sure that we unconditionally create a new seed calling random.nextLong(), then initialize the node under a private randomness context. This makes sure that any random usage through Randomness.get() will retrieve the proper random instance through RandomizedContext.current().getRandom(). When running under private randomness, the context will return the Random instance that was created with the provided seed (forked from the main random instance) rather than the main Random that's exposed to tests as well. Otherwise tests become non repeatable because that initialization part happens only before the first executed test. --- .../resources/checkstyle_suppressions.xml | 4 - .../test/ESSingleNodeTestCase.java | 85 ++++++++----------- .../org/elasticsearch/test/ESTestCase.java | 3 +- 3 files changed, 39 insertions(+), 53 deletions(-) diff --git a/buildSrc/src/main/resources/checkstyle_suppressions.xml b/buildSrc/src/main/resources/checkstyle_suppressions.xml index dba0529923f61..02097315eaab1 100644 --- a/buildSrc/src/main/resources/checkstyle_suppressions.xml +++ b/buildSrc/src/main/resources/checkstyle_suppressions.xml @@ -993,13 +993,9 @@ - - - - diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java index 32dac0fd8375f..449e959372c3b 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESSingleNodeTestCase.java @@ -18,13 +18,7 @@ */ package org.elasticsearch.test; -import java.io.IOException; -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; -import java.util.Collections; - +import com.carrotsearch.randomizedtesting.RandomizedContext; import org.apache.lucene.util.IOUtils; import org.elasticsearch.action.admin.cluster.health.ClusterHealthResponse; import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder; @@ -44,6 +38,7 @@ import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.env.Environment; +import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.index.Index; import org.elasticsearch.index.IndexService; import org.elasticsearch.indices.IndicesService; @@ -56,11 +51,16 @@ import org.elasticsearch.test.discovery.TestZenDiscovery; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.MockTcpTransportPlugin; -import org.junit.After; import org.junit.AfterClass; -import org.junit.Before; import org.junit.BeforeClass; +import java.io.IOException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; + import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.lessThanOrEqualTo; @@ -73,62 +73,56 @@ public abstract class ESSingleNodeTestCase extends ESTestCase { private static Node NODE = null; - private void reset() throws IOException { - assert NODE != null; - stopNode(); - startNode(); - } - - protected void startNode() { + protected void startNode(long seed) throws Exception { assert NODE == null; - NODE = newNode(); - // we must wait for the node to actually be up and running. otherwise the node might have started, elected itself master but might not yet have removed the + NODE = RandomizedContext.current().runWithPrivateRandomness(seed, this::newNode); + // we must wait for the node to actually be up and running. otherwise the node might have started, + // elected itself master but might not yet have removed the // SERVICE_UNAVAILABLE/1/state not recovered / initialized block ClusterHealthResponse clusterHealthResponse = client().admin().cluster().prepareHealth().setWaitForGreenStatus().get(); assertFalse(clusterHealthResponse.isTimedOut()); client().admin().indices() - .preparePutTemplate("random_index_template") + .preparePutTemplate("one_shard_index_template") .setPatterns(Collections.singletonList("*")) .setOrder(0) .setSettings(Settings.builder().put(IndexMetaData.SETTING_NUMBER_OF_SHARDS, 1) .put(IndexMetaData.SETTING_NUMBER_OF_REPLICAS, 0)).get(); } - protected static void stopNode() throws IOException { + private static void stopNode() throws IOException { Node node = NODE; NODE = null; IOUtils.close(node); } - private void cleanup(boolean resetNode) throws IOException { - assertAcked(client().admin().indices().prepareDelete("*").get()); - if (resetNode) { - reset(); - } - MetaData metaData = client().admin().cluster().prepareState().get().getState().getMetaData(); - assertThat("test leaves persistent cluster metadata behind: " + metaData.persistentSettings().getAsMap(), - metaData.persistentSettings().getAsMap().size(), equalTo(0)); - assertThat("test leaves transient cluster metadata behind: " + metaData.transientSettings().getAsMap(), - metaData.transientSettings().getAsMap().size(), equalTo(0)); - } - - @Before @Override public void setUp() throws Exception { super.setUp(); + //the seed has to be created regardless of whether it will be used or not, for repeatability + long seed = random().nextLong(); // Create the node lazily, on the first test. This is ok because we do not randomize any settings, // only the cluster name. This allows us to have overridden properties for plugins and the version to use. if (NODE == null) { - startNode(); + startNode(seed); } } - @After @Override public void tearDown() throws Exception { logger.info("[{}#{}]: cleaning up after test", getTestClass().getSimpleName(), getTestName()); super.tearDown(); - cleanup(resetNodeAfterTest()); + assertAcked(client().admin().indices().prepareDelete("*").get()); + MetaData metaData = client().admin().cluster().prepareState().get().getState().getMetaData(); + assertThat("test leaves persistent cluster metadata behind: " + metaData.persistentSettings().getAsMap(), + metaData.persistentSettings().getAsMap().size(), equalTo(0)); + assertThat("test leaves transient cluster metadata behind: " + metaData.transientSettings().getAsMap(), + metaData.transientSettings().getAsMap().size(), equalTo(0)); + if (resetNodeAfterTest()) { + assert NODE != null; + stopNode(); + //the seed can be created within this if as it will either be executed before every test method or will never be. + startNode(random().nextLong()); + } } @BeforeClass @@ -150,7 +144,6 @@ protected boolean resetNodeAfterTest() { return false; } - /** The plugin classes that should be added to the node. */ protected Collection> getPlugins() { return Collections.emptyList(); @@ -171,13 +164,13 @@ protected Settings nodeSettings() { private Node newNode() { final Path tempDir = createTempDir(); Settings settings = Settings.builder() - .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", randomLong())) + .put(ClusterName.CLUSTER_NAME_SETTING.getKey(), InternalTestCluster.clusterName("single-node-cluster", random().nextLong())) .put(Environment.PATH_HOME_SETTING.getKey(), tempDir) .put(Environment.PATH_REPO_SETTING.getKey(), tempDir.resolve("repo")) // TODO: use a consistent data path for custom paths // This needs to tie into the ESIntegTestCase#indexSettings() method .put(Environment.PATH_SHARED_DATA_SETTING.getKey(), createTempDir().getParent()) - .put("node.name", nodeName()) + .put("node.name", "node_s_0") .put("script.inline", "true") .put("script.stored", "true") .put(ScriptService.SCRIPT_MAX_COMPILATIONS_PER_MINUTE.getKey(), 1000) @@ -185,6 +178,7 @@ private Node newNode() { .put(NetworkModule.HTTP_ENABLED.getKey(), false) .put("transport.type", MockTcpTransportPlugin.MOCK_TCP_TRANSPORT_NAME) .put(Node.NODE_DATA_SETTING.getKey(), true) + .put(NodeEnvironment.NODE_ID_SEED_SETTING.getKey(), random().nextLong()) .put(nodeSettings()) // allow test cases to provide their own settings or override these .build(); Collection> plugins = getPlugins(); @@ -212,13 +206,6 @@ public Client client() { return NODE.client(); } - /** - * Returns the single test nodes name. - */ - public String nodeName() { - return "node_s_0"; - } - /** * Return a reference to the singleton node. */ @@ -274,7 +261,8 @@ protected IndexService createIndex(String index, CreateIndexRequestBuilder creat // Wait for the index to be allocated so that cluster state updates don't override // changes that would have been done locally ClusterHealthResponse health = client().admin().cluster() - .health(Requests.clusterHealthRequest(index).waitForYellowStatus().waitForEvents(Priority.LANGUID).waitForNoRelocatingShards(true)).actionGet(); + .health(Requests.clusterHealthRequest(index).waitForYellowStatus().waitForEvents(Priority.LANGUID) + .waitForNoRelocatingShards(true)).actionGet(); assertThat(health.getStatus(), lessThanOrEqualTo(ClusterHealthStatus.YELLOW)); assertThat("Cluster must be a single node cluster", health.getNumberOfDataNodes(), equalTo(1)); IndicesService instanceFromNode = getInstanceFromNode(IndicesService.class); @@ -316,7 +304,8 @@ public ClusterHealthStatus ensureGreen(String... indices) { */ public ClusterHealthStatus ensureGreen(TimeValue timeout, String... indices) { ClusterHealthResponse actionGet = client().admin().cluster() - .health(Requests.clusterHealthRequest(indices).timeout(timeout).waitForGreenStatus().waitForEvents(Priority.LANGUID).waitForNoRelocatingShards(true)).actionGet(); + .health(Requests.clusterHealthRequest(indices).timeout(timeout).waitForGreenStatus().waitForEvents(Priority.LANGUID) + .waitForNoRelocatingShards(true)).actionGet(); if (actionGet.isTimedOut()) { logger.info("ensureGreen timed out, cluster state:\n{}\n{}", client().admin().cluster().prepareState().get().getState(), client().admin().cluster().preparePendingClusterTasks().get()); diff --git a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java index a169274c172c6..9a198ade50ae2 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/ESTestCase.java @@ -795,7 +795,8 @@ public static List randomSubsetOf(Collection collection) { */ public static List randomSubsetOf(int size, Collection collection) { if (size > collection.size()) { - throw new IllegalArgumentException("Can\'t pick " + size + " random objects from a collection of " + collection.size() + " objects"); + throw new IllegalArgumentException("Can\'t pick " + size + " random objects from a collection of " + + collection.size() + " objects"); } List tempList = new ArrayList<>(collection); Collections.shuffle(tempList, random());