Fix test cluster behavior @ClusterScope(scope = Scope.SUITE) and @Ope…

…nSearchIntegTestCase.SuiteScopeTestCase for parameterized test cases (opensearch-project#12000) * Fix test cluster behavior @ClusterScope(scope = Scope.SUITE) and @OpenSearchIntegTestCase.SuiteScopeTestCase for parameterized test cases Signed-off-by: Andriy Redko <[email protected]> * Address code review comments Signed-off-by: Andriy Redko <[email protected]> * Address code review comments Signed-off-by: Andriy Redko <[email protected]> * Added test cases for all supported test cluster scopes Signed-off-by: Andriy Redko <[email protected]> --------- Signed-off-by: Andriy Redko <[email protected]>
andrross · Jan 24, 2024 · b1f8b16 · b1f8b16
1 parent 7da8628
commit b1f8b16
Show file tree

Hide file tree

Showing 8 changed files with 344 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -209,7 +209,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Use slice_size == shard_size heuristic in terms aggs for concurrent segment search and properly calculate the doc_count_error ([#11732](https://github.com/opensearch-project/OpenSearch/pull/11732))
 - Added Support for dynamically adding SearchRequestOperationsListeners with SearchRequestOperationsCompositeListenerFactory ([#11526](https://github.com/opensearch-project/OpenSearch/pull/11526))
 - Ensure Jackson default maximums introduced in 2.16.0 do not conflict with OpenSearch settings ([#11890](https://github.com/opensearch-project/OpenSearch/pull/11890))
-- Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase ([#11877](https://github.com/opensearch-project/OpenSearch/pull/11877))
+- Extract cluster management for integration tests into JUnit test rule out of OpenSearchIntegTestCase ([#11877](https://github.com/opensearch-project/OpenSearch/pull/11877)), ([#12000](https://github.com/opensearch-project/OpenSearch/pull/12000))
 - Workaround for https://bugs.openjdk.org/browse/JDK-8323659 regression, introduced in JDK-21.0.2 ([#11968](https://github.com/opensearch-project/OpenSearch/pull/11968))
 
 ### Deprecated

diff --git a/test/framework/src/main/java/org/opensearch/test/OpenSearchTestClusterRule.java b/test/framework/src/main/java/org/opensearch/test/OpenSearchTestClusterRule.java
@@ -56,6 +56,8 @@
  * on the way cluster settings are being managed.
  */
 class OpenSearchTestClusterRule implements MethodRule {
+    // Maps each TestCluster instance to the exact test suite instance that triggered its creation
+    private final Map<TestCluster, OpenSearchIntegTestCase> suites = new IdentityHashMap<>();
     private final Map<Class<?>, TestCluster> clusters = new IdentityHashMap<>();
     private final Logger logger = LogManager.getLogger(getClass());
 
@@ -86,7 +88,13 @@ void afterClass() throws Exception {
                 printTestMessage("cleaning up after");
                 afterInternal(true, null);
                 OpenSearchTestCase.checkStaticState(true);
-                clusters.remove(getTestClass());
+                synchronized (clusters) {
+                    final TestCluster cluster = clusters.remove(getTestClass());
+                    IOUtils.closeWhileHandlingException(cluster);
+                    if (cluster != null) {
+                        suites.remove(cluster);
+                    }
+                }
             }
             StrictCheckSpanProcessor.validateTracingStateOnShutdown();
         } finally {
@@ -226,8 +234,11 @@ private static boolean isSuiteScopedTest(Class<?> clazz) {
         return clazz.getAnnotation(SuiteScopeTestCase.class) != null;
     }
 
-    private boolean hasParametersChanged(final ParameterizedOpenSearchIntegTestCase target) {
-        return !((ParameterizedOpenSearchIntegTestCase) suiteInstance).hasSameParametersAs(target);
+    private static boolean hasParametersChanged(
+        final ParameterizedOpenSearchIntegTestCase instance,
+        final ParameterizedOpenSearchIntegTestCase target
+    ) {
+        return !instance.hasSameParametersAs(target);
     }
 
     private boolean runTestScopeLifecycle() {
@@ -242,8 +253,24 @@ private TestCluster buildAndPutCluster(Scope currentClusterScope, long seed, Ope
             clearClusters(); // all leftovers are gone by now... this is really just a double safety if we miss something somewhere
             switch (currentClusterScope) {
                 case SUITE:
+                    if (testCluster != null && target instanceof ParameterizedOpenSearchIntegTestCase) {
+                        final OpenSearchIntegTestCase instance = suites.get(testCluster);
+                        if (instance != null) {
+                            assert instance instanceof ParameterizedOpenSearchIntegTestCase;
+                            if (hasParametersChanged(
+                                (ParameterizedOpenSearchIntegTestCase) instance,
+                                (ParameterizedOpenSearchIntegTestCase) target
+                            )) {
+                                IOUtils.closeWhileHandlingException(testCluster);
+                                printTestMessage("new instance of parameterized test class, recreating test cluster for suite");
+                                testCluster = null;
+                            }
+                        }
+                    }
+
                     if (testCluster == null) { // only build if it's not there yet
                         testCluster = buildWithPrivateContext(currentClusterScope, seed, target);
+                        suites.put(testCluster, target);
                     }
                     break;
                 case TEST:
@@ -310,6 +337,7 @@ private void clearClusters() throws Exception {
         synchronized (clusters) {
             if (!clusters.isEmpty()) {
                 IOUtils.close(clusters.values());
+                suites.clear();
                 clusters.clear();
             }
         }
@@ -363,7 +391,10 @@ private void initializeSuiteScope(OpenSearchIntegTestCase target, FrameworkMetho
             // Catching the case when parameterized test cases are run: the test class stays the same but the test instances changes.
             if (target instanceof ParameterizedOpenSearchIntegTestCase) {
                 assert suiteInstance instanceof ParameterizedOpenSearchIntegTestCase;
-                if (hasParametersChanged((ParameterizedOpenSearchIntegTestCase) target)) {
+                if (hasParametersChanged(
+                    (ParameterizedOpenSearchIntegTestCase) suiteInstance,
+                    (ParameterizedOpenSearchIntegTestCase) target
+                )) {
                     printTestMessage("new instance of parameterized test class, recreating cluster scope", method);
                     afterClass();
                     beforeClass();

diff --git a/...k/src/test/java/org/opensearch/test/ParameterizedDynamicSettingsOpenSearchIntegTests.java b/...k/src/test/java/org/opensearch/test/ParameterizedDynamicSettingsOpenSearchIntegTests.java
@@ -0,0 +1,49 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.test;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+
+import org.opensearch.action.admin.cluster.state.ClusterStateResponse;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.common.util.FeatureFlags;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+
+import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
+import static org.hamcrest.CoreMatchers.equalTo;
+
+public class ParameterizedDynamicSettingsOpenSearchIntegTests extends ParameterizedDynamicSettingsOpenSearchIntegTestCase {
+    public ParameterizedDynamicSettingsOpenSearchIntegTests(Settings dynamicSettings) {
+        super(dynamicSettings);
+    }
+
+    @ParametersFactory
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false).build() },
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build() }
+        );
+    }
+
+    @Override
+    protected Settings featureFlagSettings() {
+        return Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.CONCURRENT_SEGMENT_SEARCH, "true").build();
+    }
+
+    public void testSettings() throws IOException {
+        final ClusterStateResponse cluster = client().admin().cluster().prepareState().all().get();
+        assertThat(
+            CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(cluster.getState().getMetadata().settings()),
+            equalTo(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(settings))
+        );
+    }
+}
diff --git a/...rk/src/test/java/org/opensearch/test/ParameterizedStaticSettingsOpenSearchIntegTests.java b/...rk/src/test/java/org/opensearch/test/ParameterizedStaticSettingsOpenSearchIntegTests.java
@@ -0,0 +1,53 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.test;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+
+import org.opensearch.action.admin.cluster.node.info.NodeInfo;
+import org.opensearch.action.admin.cluster.node.info.NodesInfoResponse;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.common.util.FeatureFlags;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+
+import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
+import static org.hamcrest.CoreMatchers.equalTo;
+
+public class ParameterizedStaticSettingsOpenSearchIntegTests extends ParameterizedStaticSettingsOpenSearchIntegTestCase {
+
+    public ParameterizedStaticSettingsOpenSearchIntegTests(Settings staticSettings) {
+        super(staticSettings);
+    }
+
+    @ParametersFactory
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false).build() },
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build() }
+        );
+    }
+
+    @Override
+    protected Settings featureFlagSettings() {
+        return Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.CONCURRENT_SEGMENT_SEARCH, "true").build();
+    }
+
+    public void testSettings() throws IOException {
+        final NodesInfoResponse nodes = client().admin().cluster().prepareNodesInfo().get();
+        for (final NodeInfo node : nodes.getNodes()) {
+            assertThat(
+                CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(node.getSettings()),
+                equalTo(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(settings))
+            );
+        }
+    }
+}
diff --git a/...java/org/opensearch/test/SuiteScopedParameterizedDynamicSettingsOpenSearchIntegTests.java b/...java/org/opensearch/test/SuiteScopedParameterizedDynamicSettingsOpenSearchIntegTests.java
@@ -0,0 +1,50 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.test;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+
+import org.opensearch.action.admin.cluster.state.ClusterStateResponse;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.common.util.FeatureFlags;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+
+import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
+import static org.hamcrest.CoreMatchers.equalTo;
+
+@OpenSearchIntegTestCase.SuiteScopeTestCase
+public class SuiteScopedParameterizedDynamicSettingsOpenSearchIntegTests extends ParameterizedDynamicSettingsOpenSearchIntegTestCase {
+    public SuiteScopedParameterizedDynamicSettingsOpenSearchIntegTests(Settings dynamicSettings) {
+        super(dynamicSettings);
+    }
+
+    @ParametersFactory
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false).build() },
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build() }
+        );
+    }
+
+    @Override
+    protected Settings featureFlagSettings() {
+        return Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.CONCURRENT_SEGMENT_SEARCH, "true").build();
+    }
+
+    public void testSettings() throws IOException {
+        final ClusterStateResponse cluster = client().admin().cluster().prepareState().all().get();
+        assertThat(
+            CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(cluster.getState().getMetadata().settings()),
+            equalTo(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(settings))
+        );
+    }
+}
diff --git a/.../java/org/opensearch/test/SuiteScopedParameterizedStaticSettingsOpenSearchIntegTests.java b/.../java/org/opensearch/test/SuiteScopedParameterizedStaticSettingsOpenSearchIntegTests.java
@@ -0,0 +1,53 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.test;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+
+import org.opensearch.action.admin.cluster.node.info.NodeInfo;
+import org.opensearch.action.admin.cluster.node.info.NodesInfoResponse;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.common.util.FeatureFlags;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+
+import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
+import static org.hamcrest.CoreMatchers.equalTo;
+
+@OpenSearchIntegTestCase.SuiteScopeTestCase
+public class SuiteScopedParameterizedStaticSettingsOpenSearchIntegTests extends ParameterizedStaticSettingsOpenSearchIntegTestCase {
+    public SuiteScopedParameterizedStaticSettingsOpenSearchIntegTests(Settings staticSettings) {
+        super(staticSettings);
+    }
+
+    @ParametersFactory
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false).build() },
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build() }
+        );
+    }
+
+    @Override
+    protected Settings featureFlagSettings() {
+        return Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.CONCURRENT_SEGMENT_SEARCH, "true").build();
+    }
+
+    public void testSettings() throws IOException {
+        final NodesInfoResponse nodes = client().admin().cluster().prepareNodesInfo().get();
+        for (final NodeInfo node : nodes.getNodes()) {
+            assertThat(
+                CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(node.getSettings()),
+                equalTo(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(settings))
+            );
+        }
+    }
+}
diff --git a/.../java/org/opensearch/test/TestScopedParameterizedDynamicSettingsOpenSearchIntegTests.java b/.../java/org/opensearch/test/TestScopedParameterizedDynamicSettingsOpenSearchIntegTests.java
@@ -0,0 +1,50 @@
+/*
+ * SPDX-License-Identifier: Apache-2.0
+ *
+ * The OpenSearch Contributors require contributions made to
+ * this file be licensed under the Apache-2.0 license or a
+ * compatible open source license.
+ */
+
+package org.opensearch.test;
+
+import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
+
+import org.opensearch.action.admin.cluster.state.ClusterStateResponse;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.common.util.FeatureFlags;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collection;
+
+import static org.opensearch.search.SearchService.CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING;
+import static org.hamcrest.CoreMatchers.equalTo;
+
+@OpenSearchIntegTestCase.ClusterScope(scope = OpenSearchIntegTestCase.Scope.TEST)
+public class TestScopedParameterizedDynamicSettingsOpenSearchIntegTests extends ParameterizedDynamicSettingsOpenSearchIntegTestCase {
+    public TestScopedParameterizedDynamicSettingsOpenSearchIntegTests(Settings dynamicSettings) {
+        super(dynamicSettings);
+    }
+
+    @ParametersFactory
+    public static Collection<Object[]> parameters() {
+        return Arrays.asList(
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), false).build() },
+            new Object[] { Settings.builder().put(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.getKey(), true).build() }
+        );
+    }
+
+    @Override
+    protected Settings featureFlagSettings() {
+        return Settings.builder().put(super.featureFlagSettings()).put(FeatureFlags.CONCURRENT_SEGMENT_SEARCH, "true").build();
+    }
+
+    public void testSettings() throws IOException {
+        final ClusterStateResponse cluster = client().admin().cluster().prepareState().all().get();
+        assertThat(
+            CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(cluster.getState().getMetadata().settings()),
+            equalTo(CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.get(settings))
+        );
+    }
+}