Refactor: move version, os, features from ClientYamlTestClient to ClientYamlTestExecutionContext (#103560)

Just moving stuff around, no change in behaviour. Moving these fields showed how we are not treating correctly in derived classes where multiple clusters are tested (ex: CCR), but this is for another time.

Co-authored-by: Moritz Mack <moritz@mackmail.net>

docs/src/yamlRestTest/java/org/elasticsearch/smoketest/DocsClientYamlTestSuiteIT.java

@@ -16,7 +16,6 @@ import org.apache.http.HttpHost;
 import org.apache.http.util.EntityUtils;
 import org.apache.lucene.tests.util.TimeUnits;
 import org.apache.lucene.util.BytesRef;
-import org.elasticsearch.Version;
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.RestClient;
 import org.elasticsearch.common.settings.SecureString;
@@ -48,7 +47,6 @@ import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.function.Predicate;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.emptyMap;
@@ -98,20 +96,9 @@ public class DocsClientYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
     protected ClientYamlTestClient initClientYamlTestClient(
         final ClientYamlSuiteRestSpec restSpec,
         final RestClient restClient,
-        final List<HttpHost> hosts,
-        final Version esVersion,
-        final Predicate<String> clusterFeaturesPredicate,
-        final String os
+        final List<HttpHost> hosts
     ) {
-        return new ClientYamlDocsTestClient(
-            restSpec,
-            restClient,
-            hosts,
-            esVersion,
-            clusterFeaturesPredicate,
-            os,
-            this::getClientBuilderWithSniffedHosts
-        );
+        return new ClientYamlDocsTestClient(restSpec, restClient, hosts, this::getClientBuilderWithSniffedHosts);
     }
 
     @Before

qa/ccs-common-rest/src/yamlRestTest/java/org/elasticsearch/test/rest/yaml/CcsCommonYamlTestSuiteIT.java

@@ -25,7 +25,6 @@ import org.elasticsearch.client.RestClientBuilder;
 import org.elasticsearch.common.CheckedSupplier;
 import org.elasticsearch.common.Strings;
 import org.elasticsearch.core.IOUtils;
-import org.elasticsearch.core.Tuple;
 import org.elasticsearch.test.cluster.ElasticsearchCluster;
 import org.elasticsearch.test.cluster.FeatureFlag;
 import org.elasticsearch.test.cluster.local.LocalClusterConfigProvider;
@@ -159,19 +158,7 @@ public class CcsCommonYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
             searchClient = buildClient(restClientSettings(), clusterHosts.toArray(new HttpHost[clusterHosts.size()]));
             adminSearchClient = buildClient(restAdminSettings(), clusterHosts.toArray(new HttpHost[clusterHosts.size()]));
 
-            Tuple<Version, Version> versionVersionTuple = readVersionsFromCatNodes(adminSearchClient);
-            final Version esVersion = versionVersionTuple.v1();
-            final String os = readOsFromNodesInfo(adminSearchClient);
-
-            searchYamlTestClient = new TestCandidateAwareClient(
-                getRestSpec(),
-                searchClient,
-                hosts,
-                esVersion,
-                ESRestTestCase::clusterHasFeature,
-                os,
-                this::getClientBuilderWithSniffedHosts
-            );
+            searchYamlTestClient = new TestCandidateAwareClient(getRestSpec(), searchClient, hosts, this::getClientBuilderWithSniffedHosts);
 
             // check that we have an established CCS connection
             Request request = new Request("GET", "_remote/info");
@@ -298,10 +285,22 @@ public class CcsCommonYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
     @Override
     protected ClientYamlTestExecutionContext createRestTestExecutionContext(
         ClientYamlTestCandidate clientYamlTestCandidate,
-        ClientYamlTestClient clientYamlTestClient
+        ClientYamlTestClient clientYamlTestClient,
+        final Version esVersion,
+        final Predicate<String> clusterFeaturesPredicate,
+        final String os
     ) {
         // depending on the API called, we either return the client running against the "write" or the "search" cluster here
-        return new ClientYamlTestExecutionContext(clientYamlTestCandidate, clientYamlTestClient, randomizeContentType()) {
+
+        // TODO: reconcile and provide unified features, os, version(s), based on both clientYamlTestClient and searchYamlTestClient
+        return new ClientYamlTestExecutionContext(
+            clientYamlTestCandidate,
+            clientYamlTestClient,
+            randomizeContentType(),
+            esVersion,
+            ESRestTestCase::clusterHasFeature,
+            os
+        ) {
             protected ClientYamlTestClient clientYamlTestClient(String apiName) {
                 if (CCS_APIS.contains(apiName)) {
                     return searchYamlTestClient;
@@ -328,12 +327,9 @@ public class CcsCommonYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
             ClientYamlSuiteRestSpec restSpec,
             RestClient restClient,
             List<HttpHost> hosts,
-            Version esVersion,
-            Predicate<String> clusterFeaturesPredicate,
-            String os,
             CheckedSupplier<RestClientBuilder, IOException> clientBuilderWithSniffedNodes
         ) {
-            super(restSpec, restClient, hosts, esVersion, clusterFeaturesPredicate, os, clientBuilderWithSniffedNodes);
+            super(restSpec, restClient, hosts, clientBuilderWithSniffedNodes);
         }
 
         public void setTestCandidate(ClientYamlTestCandidate testCandidate) {

qa/ccs-common-rest/src/yamlRestTest/java/org/elasticsearch/test/rest/yaml/RcsCcsCommonYamlTestSuiteIT.java

@@ -26,7 +26,6 @@ import org.elasticsearch.common.settings.SecureString;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.ThreadContext;
 import org.elasticsearch.core.IOUtils;
-import org.elasticsearch.core.Tuple;
 import org.elasticsearch.test.cluster.ElasticsearchCluster;
 import org.elasticsearch.test.cluster.FeatureFlag;
 import org.elasticsearch.test.cluster.local.LocalClusterConfigProvider;
@@ -46,6 +45,7 @@ import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Predicate;
 
 import static java.util.Collections.unmodifiableList;
 import static org.elasticsearch.test.rest.yaml.CcsCommonYamlTestSuiteIT.CCS_APIS;
@@ -221,19 +221,7 @@ public class RcsCcsCommonYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
                 clusterHosts.toArray(new HttpHost[clusterHosts.size()])
             );
 
-            Tuple<Version, Version> versionVersionTuple = readVersionsFromCatNodes(adminSearchClient);
-            final Version esVersion = versionVersionTuple.v1();
-            final String os = readOsFromNodesInfo(adminSearchClient);
-
-            searchYamlTestClient = new TestCandidateAwareClient(
-                getRestSpec(),
-                searchClient,
-                hosts,
-                esVersion,
-                ESRestTestCase::clusterHasFeature,
-                os,
-                this::getClientBuilderWithSniffedHosts
-            );
+            searchYamlTestClient = new TestCandidateAwareClient(getRestSpec(), searchClient, hosts, this::getClientBuilderWithSniffedHosts);
 
             configureRemoteCluster();
             // check that we have an established CCS connection
@@ -282,10 +270,22 @@ public class RcsCcsCommonYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
     @Override
     protected ClientYamlTestExecutionContext createRestTestExecutionContext(
         ClientYamlTestCandidate clientYamlTestCandidate,
-        ClientYamlTestClient clientYamlTestClient
+        ClientYamlTestClient clientYamlTestClient,
+        final Version esVersion,
+        final Predicate<String> clusterFeaturesPredicate,
+        final String os
     ) {
         // depending on the API called, we either return the client running against the "write" or the "search" cluster here
-        return new ClientYamlTestExecutionContext(clientYamlTestCandidate, clientYamlTestClient, randomizeContentType()) {
+
+        // TODO: reconcile and provide unified features, os, version(s), based on both clientYamlTestClient and searchYamlTestClient
+        return new ClientYamlTestExecutionContext(
+            clientYamlTestCandidate,
+            clientYamlTestClient,
+            randomizeContentType(),
+            esVersion,
+            ESRestTestCase::clusterHasFeature,
+            os
+        ) {
             protected ClientYamlTestClient clientYamlTestClient(String apiName) {
                 if (CCS_APIS.contains(apiName)) {
                     return searchYamlTestClient;

qa/multi-cluster-search/src/test/java/org/elasticsearch/search/MultiClusterSearchYamlTestSuiteIT.java

@@ -14,12 +14,15 @@ import com.carrotsearch.randomizedtesting.annotations.TimeoutSuite;
 
 import org.apache.lucene.tests.util.TimeUnits;
 import org.elasticsearch.Version;
+import org.elasticsearch.test.rest.ESRestTestCase;
 import org.elasticsearch.test.rest.yaml.ClientYamlTestCandidate;
 import org.elasticsearch.test.rest.yaml.ClientYamlTestClient;
 import org.elasticsearch.test.rest.yaml.ClientYamlTestExecutionContext;
 import org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase;
 import org.junit.BeforeClass;
 
+import java.util.function.Predicate;
+
 @TimeoutSuite(millis = 5 * TimeUnits.MINUTE) // to account for slow as hell VMs
 public class MultiClusterSearchYamlTestSuiteIT extends ESClientYamlSuiteTestCase {
 
@@ -33,27 +36,31 @@ public class MultiClusterSearchYamlTestSuiteIT extends ESClientYamlSuiteTestCase
         }
     }
 
+    @Override
     protected ClientYamlTestExecutionContext createRestTestExecutionContext(
         ClientYamlTestCandidate clientYamlTestCandidate,
-        ClientYamlTestClient clientYamlTestClient
+        ClientYamlTestClient clientYamlTestClient,
+        final Version esVersion,
+        final Predicate<String> clusterFeaturesPredicate,
+        final String os
     ) {
-        return new ClientYamlTestExecutionContext(clientYamlTestCandidate, clientYamlTestClient, randomizeContentType()) {
+        /*
+         * Since the esVersion is used to skip tests in ESClientYamlSuiteTestCase, we also take into account the
+         * remote cluster version here and return it if it is lower than the local client version. This is used to
+         * skip tests if some feature isn't available on the remote cluster yet.
+         */
+        final Version commonEsVersion = remoteEsVersion != null && remoteEsVersion.before(esVersion) ? remoteEsVersion : esVersion;
+
+        // TODO: same for os and features
 
-            /**
-             * Since the esVersion is used to skip tests in ESClientYamlSuiteTestCase, we also take into account the
-             * remote cluster version here and return it if it is lower than the local client version. This is used to
-             * skip tests if some feature isn't available on the remote cluster yet.
-             */
-            @Override
-            public Version esVersion() {
-                Version clientEsVersion = clientYamlTestClient.getEsVersion();
-                if (remoteEsVersion == null) {
-                    return clientEsVersion;
-                } else {
-                    return remoteEsVersion.before(clientEsVersion) ? remoteEsVersion : clientEsVersion;
-                }
-            }
-        };
+        return new ClientYamlTestExecutionContext(
+            clientYamlTestCandidate,
+            clientYamlTestClient,
+            randomizeContentType(),
+            commonEsVersion,
+            ESRestTestCase::clusterHasFeature,
+            os
+        );
     }
 
     @Override

test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlDocsTestClient.java

@@ -10,7 +10,6 @@ package org.elasticsearch.test.rest.yaml;
 
 import org.apache.http.HttpEntity;
 import org.apache.http.HttpHost;
-import org.elasticsearch.Version;
 import org.elasticsearch.client.NodeSelector;
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.Response;
@@ -27,7 +26,6 @@ import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.function.BiPredicate;
-import java.util.function.Predicate;
 
 /**
  * Used to execute REST requests according to the docs snippets that need to be tests. Wraps a
@@ -40,12 +38,9 @@ public final class ClientYamlDocsTestClient extends ClientYamlTestClient {
         final ClientYamlSuiteRestSpec restSpec,
         final RestClient restClient,
         final List<HttpHost> hosts,
-        final Version esVersion,
-        final Predicate<String> clusterFeaturesPredicate,
-        final String os,
         final CheckedSupplier<RestClientBuilder, IOException> clientBuilderWithSniffedNodes
     ) {
-        super(restSpec, restClient, hosts, esVersion, clusterFeaturesPredicate, os, clientBuilderWithSniffedNodes);
+        super(restSpec, restClient, hosts, clientBuilderWithSniffedNodes);
     }
 
     @Override

test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java

@@ -20,7 +20,6 @@ import org.apache.http.message.BasicNameValuePair;
 import org.apache.http.util.EntityUtils;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-import org.elasticsearch.Version;
 import org.elasticsearch.client.NodeSelector;
 import org.elasticsearch.client.Request;
 import org.elasticsearch.client.RequestOptions;
@@ -47,7 +46,6 @@ import java.util.Map;
 import java.util.Map.Entry;
 import java.util.Set;
 import java.util.function.BiPredicate;
-import java.util.function.Predicate;
 import java.util.stream.Collectors;
 
 import static com.carrotsearch.randomizedtesting.RandomizedTest.frequently;
@@ -64,44 +62,20 @@ public class ClientYamlTestClient implements Closeable {
 
     private final ClientYamlSuiteRestSpec restSpec;
     private final Map<NodeSelector, RestClient> restClients = new HashMap<>();
-    private final Version esVersion;
-    private final String os;
     private final CheckedSupplier<RestClientBuilder, IOException> clientBuilderWithSniffedNodes;
-    private final Predicate<String> clusterFeaturesPredicate;
 
     ClientYamlTestClient(
         final ClientYamlSuiteRestSpec restSpec,
         final RestClient restClient,
         final List<HttpHost> hosts,
-        final Version esVersion,
-        final Predicate<String> clusterFeaturesPredicate,
-        final String os,
         final CheckedSupplier<RestClientBuilder, IOException> clientBuilderWithSniffedNodes
     ) {
-        this.clusterFeaturesPredicate = clusterFeaturesPredicate;
         assert hosts.size() > 0;
         this.restSpec = restSpec;
         this.restClients.put(NodeSelector.ANY, restClient);
-        this.esVersion = esVersion;
-        this.os = os;
         this.clientBuilderWithSniffedNodes = clientBuilderWithSniffedNodes;
     }
 
-    /**
-     * @return the version of the oldest node in the cluster
-     */
-    public Version getEsVersion() {
-        return esVersion;
-    }
-
-    public boolean clusterHasFeature(String featureId) {
-        return clusterFeaturesPredicate.test(featureId);
-    }
-
-    public String getOs() {
-        return os;
-    }
-
     /**
      * Calls an api with the provided parameters and body
      */

test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContext.java

@@ -31,6 +31,7 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.function.BiPredicate;
+import java.util.function.Predicate;
 
 /**
  * Execution context passed across the REST tests.
@@ -50,26 +51,48 @@ public class ClientYamlTestExecutionContext {
 
     private ClientYamlTestResponse response;
 
+    private final Version esVersion;
+
+    private final String os;
+    private final Predicate<String> clusterFeaturesPredicate;
+
     private final boolean randomizeContentType;
     private final BiPredicate<ClientYamlSuiteRestApi, ClientYamlSuiteRestApi.Path> pathPredicate;
 
     public ClientYamlTestExecutionContext(
         ClientYamlTestCandidate clientYamlTestCandidate,
         ClientYamlTestClient clientYamlTestClient,
-        boolean randomizeContentType
+        boolean randomizeContentType,
+        final Version esVersion,
+        final Predicate<String> clusterFeaturesPredicate,
+        final String os
     ) {
-        this(clientYamlTestCandidate, clientYamlTestClient, randomizeContentType, (ignoreApi, ignorePath) -> true);
+        this(
+            clientYamlTestCandidate,
+            clientYamlTestClient,
+            randomizeContentType,
+            esVersion,
+            clusterFeaturesPredicate,
+            os,
+            (ignoreApi, ignorePath) -> true
+        );
     }
 
     public ClientYamlTestExecutionContext(
         ClientYamlTestCandidate clientYamlTestCandidate,
         ClientYamlTestClient clientYamlTestClient,
         boolean randomizeContentType,
+        final Version esVersion,
+        final Predicate<String> clusterFeaturesPredicate,
+        final String os,
         BiPredicate<ClientYamlSuiteRestApi, ClientYamlSuiteRestApi.Path> pathPredicate
     ) {
         this.clientYamlTestClient = clientYamlTestClient;
         this.clientYamlTestCandidate = clientYamlTestCandidate;
         this.randomizeContentType = randomizeContentType;
+        this.esVersion = esVersion;
+        this.clusterFeaturesPredicate = clusterFeaturesPredicate;
+        this.os = os;
         this.pathPredicate = pathPredicate;
     }
 
@@ -227,11 +250,11 @@ public class ClientYamlTestExecutionContext {
      * @return the version of the oldest node in the cluster
      */
     public Version esVersion() {
-        return clientYamlTestClient.getEsVersion();
+        return esVersion;
     }
 
     public String os() {
-        return clientYamlTestClient.getOs();
+        return os;
     }
 
     public ClientYamlTestCandidate getClientYamlTestCandidate() {
@@ -239,6 +262,6 @@ public class ClientYamlTestExecutionContext {
     }
 
     public boolean clusterHasFeature(String featureId) {
-        return clientYamlTestClient.clusterHasFeature(featureId);
+        return clusterFeaturesPredicate.test(featureId);
     }
 }

test/yaml-rest-runner/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java

@@ -151,9 +151,22 @@ public abstract class ESClientYamlSuiteTestCase extends ESRestTestCase {
                 hosts,
                 os
             );
-            clientYamlTestClient = initClientYamlTestClient(restSpec, client(), hosts, esVersion, ESRestTestCase::clusterHasFeature, os);
-            restTestExecutionContext = createRestTestExecutionContext(testCandidate, clientYamlTestClient);
-            adminExecutionContext = new ClientYamlTestExecutionContext(testCandidate, clientYamlTestClient, false);
+            clientYamlTestClient = initClientYamlTestClient(restSpec, client(), hosts);
+            restTestExecutionContext = createRestTestExecutionContext(
+                testCandidate,
+                clientYamlTestClient,
+                esVersion,
+                ESRestTestCase::clusterHasFeature,
+                os
+            );
+            adminExecutionContext = new ClientYamlTestExecutionContext(
+                testCandidate,
+                clientYamlTestClient,
+                false,
+                esVersion,
+                ESRestTestCase::clusterHasFeature,
+                os
+            );
             final String[] blacklist = resolvePathsProperty(REST_TESTS_BLACKLIST, null);
             blacklistPathMatchers = new ArrayList<>();
             for (final String entry : blacklist) {
@@ -179,30 +192,29 @@ public abstract class ESClientYamlSuiteTestCase extends ESRestTestCase {
      */
     protected ClientYamlTestExecutionContext createRestTestExecutionContext(
         ClientYamlTestCandidate clientYamlTestCandidate,
-        ClientYamlTestClient clientYamlTestClient
-    ) {
-        return new ClientYamlTestExecutionContext(clientYamlTestCandidate, clientYamlTestClient, randomizeContentType());
-    }
-
-    protected ClientYamlTestClient initClientYamlTestClient(
-        final ClientYamlSuiteRestSpec restSpec,
-        final RestClient restClient,
-        final List<HttpHost> hosts,
+        ClientYamlTestClient clientYamlTestClient,
         final Version esVersion,
         final Predicate<String> clusterFeaturesPredicate,
         final String os
     ) {
-        return new ClientYamlTestClient(
-            restSpec,
-            restClient,
-            hosts,
+        return new ClientYamlTestExecutionContext(
+            clientYamlTestCandidate,
+            clientYamlTestClient,
+            randomizeContentType(),
             esVersion,
             clusterFeaturesPredicate,
-            os,
-            this::getClientBuilderWithSniffedHosts
+            os
         );
     }
 
+    protected ClientYamlTestClient initClientYamlTestClient(
+        final ClientYamlSuiteRestSpec restSpec,
+        final RestClient restClient,
+        final List<HttpHost> hosts
+    ) {
+        return new ClientYamlTestClient(restSpec, restClient, hosts, this::getClientBuilderWithSniffedHosts);
+    }
+
     @AfterClass
     public static void closeClient() throws IOException {
         try {

test/yaml-rest-runner/src/test/java/org/elasticsearch/test/rest/yaml/ClientYamlTestExecutionContextTests.java

@@ -27,7 +27,14 @@ public class ClientYamlTestExecutionContextTests extends ESTestCase {
     public void testHeadersSupportStashedValueReplacement() throws IOException {
         final AtomicReference<Map<String, String>> headersRef = new AtomicReference<>();
         final Version version = VersionUtils.randomVersion(random());
-        final ClientYamlTestExecutionContext context = new ClientYamlTestExecutionContext(null, null, randomBoolean()) {
+        final ClientYamlTestExecutionContext context = new ClientYamlTestExecutionContext(
+            null,
+            null,
+            randomBoolean(),
+            version,
+            feature -> true,
+            "os"
+        ) {
             @Override
             ClientYamlTestResponse callApiInternal(
                 String apiName,
@@ -39,11 +46,6 @@ public class ClientYamlTestExecutionContextTests extends ESTestCase {
                 headersRef.set(headers);
                 return null;
             }
-
-            @Override
-            public Version esVersion() {
-                return version;
-            }
         };
         final Map<String, String> headers = new HashMap<>();
         headers.put("foo", "$bar");
@@ -63,7 +65,14 @@ public class ClientYamlTestExecutionContextTests extends ESTestCase {
 
     public void testStashHeadersOnException() throws IOException {
         final Version version = VersionUtils.randomVersion(random());
-        final ClientYamlTestExecutionContext context = new ClientYamlTestExecutionContext(null, null, randomBoolean()) {
+        final ClientYamlTestExecutionContext context = new ClientYamlTestExecutionContext(
+            null,
+            null,
+            randomBoolean(),
+            version,
+            feature -> true,
+            "os"
+        ) {
             @Override
             ClientYamlTestResponse callApiInternal(
                 String apiName,
@@ -74,11 +83,6 @@ public class ClientYamlTestExecutionContextTests extends ESTestCase {
             ) {
                 throw new RuntimeException("boom!");
             }
-
-            @Override
-            public Version esVersion() {
-                return version;
-            }
         };
         final Map<String, String> headers = new HashMap<>();
         headers.put("Accept", "application/json");