Browse Source

Fix: remove wrong assertion from `ESRestTestFeatureService#clusterHasFeature` (#104299)

The check is currently broken - feature check always falls back to historical features, and only then if the feature is not one of the historical ones, and exception (assert) is risen.
This is wrong: it could be that we are testing for a non-historical feature, and the cluster simply does not have it.
Lorenzo Dematté 1 year ago
parent
commit
2a84b62efc

+ 19 - 7
test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java

@@ -59,6 +59,7 @@ import org.elasticsearch.core.CheckedRunnable;
 import org.elasticsearch.core.IOUtils;
 import org.elasticsearch.core.Nullable;
 import org.elasticsearch.core.PathUtils;
+import org.elasticsearch.core.SuppressForbidden;
 import org.elasticsearch.core.TimeValue;
 import org.elasticsearch.core.UpdateForV9;
 import org.elasticsearch.features.FeatureSpecification;
@@ -84,6 +85,7 @@ import org.junit.AfterClass;
 import org.junit.Before;
 
 import java.io.BufferedReader;
+import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
@@ -333,19 +335,28 @@ public abstract class ESRestTestCase extends ESTestCase {
         assert nodesVersions != null;
     }
 
-    protected static TestFeatureService createTestFeatureService(
+    protected TestFeatureService createTestFeatureService(
         Map<String, Set<String>> clusterStateFeatures,
         Set<Version> semanticNodeVersions
     ) {
         // Historical features information is unavailable when using legacy test plugins
         boolean hasHistoricalFeaturesInformation = System.getProperty("tests.features.metadata.path") != null;
-        var providers = hasHistoricalFeaturesInformation
-            ? List.of(new RestTestLegacyFeatures(), new ESRestTestCaseHistoricalFeatures())
-            : List.of(new RestTestLegacyFeatures());
+
+        final List<FeatureSpecification> featureSpecifications;
+        if (hasHistoricalFeaturesInformation) {
+            featureSpecifications = List.of(new RestTestLegacyFeatures(), new ESRestTestCaseHistoricalFeatures());
+        } else {
+            logger.warn(
+                "This test is running on the legacy test framework; historical features from production code will not be available. "
+                    + "You need to port the test to the new test plugins in order to use historical features from production code. "
+                    + "If this is a legacy feature used only in tests, you can add it to a test-only FeatureSpecification such as {}.",
+                RestTestLegacyFeatures.class.getCanonicalName()
+            );
+            featureSpecifications = List.of(new RestTestLegacyFeatures());
+        }
 
         return new ESRestTestFeatureService(
-            hasHistoricalFeaturesInformation,
-            providers,
+            featureSpecifications,
             semanticNodeVersions,
             ClusterFeatures.calculateAllNodeFeatures(clusterStateFeatures.values())
         );
@@ -2343,6 +2354,7 @@ public abstract class ESRestTestCase extends ESTestCase {
         private static Map<NodeFeature, Version> historicalFeatures;
 
         @Override
+        @SuppressForbidden(reason = "File#pathSeparator has not equivalent in java.nio.file")
         public Map<NodeFeature, Version> getHistoricalFeatures() {
             if (historicalFeatures == null) {
                 Map<NodeFeature, Version> historicalFeaturesMap = new HashMap<>();
@@ -2353,7 +2365,7 @@ public abstract class ESRestTestCase extends ESTestCase {
                     );
                 }
 
-                String[] metadataFiles = metadataPath.split(System.getProperty("path.separator"));
+                String[] metadataFiles = metadataPath.split(File.pathSeparator);
                 for (String metadataFile : metadataFiles) {
                     try (
                         InputStream in = Files.newInputStream(PathUtils.get(metadataFile));

+ 5 - 21
test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestFeatureService.java

@@ -9,11 +9,11 @@
 package org.elasticsearch.test.rest;
 
 import org.elasticsearch.Version;
-import org.elasticsearch.core.Strings;
 import org.elasticsearch.features.FeatureData;
 import org.elasticsearch.features.FeatureSpecification;
 
 import java.util.Collection;
+import java.util.Comparator;
 import java.util.List;
 import java.util.NavigableMap;
 import java.util.Set;
@@ -24,33 +24,17 @@ class ESRestTestFeatureService implements TestFeatureService {
     private final Set<String> clusterStateFeatures;
 
     ESRestTestFeatureService(
-        boolean hasHistoricalFeaturesInformation,
         List<? extends FeatureSpecification> specs,
         Collection<Version> nodeVersions,
         Set<String> clusterStateFeatures
     ) {
-        var minNodeVersion = nodeVersions.stream().min(Version::compareTo);
+        var minNodeVersion = nodeVersions.stream().min(Comparator.naturalOrder());
         var featureData = FeatureData.createFromSpecifications(specs);
         var historicalFeatures = featureData.getHistoricalFeatures();
-        var allHistoricalFeatures = historicalFeatures.lastEntry() == null ? Set.of() : historicalFeatures.lastEntry().getValue();
 
-        var errorMessage = Strings.format(
-            hasHistoricalFeaturesInformation
-                ? "Check the feature has been added to the correct FeatureSpecification in the relevant module or, if this is a "
-                    + "legacy feature used only in tests, to a test-only FeatureSpecification such as %s."
-                : "This test is running on the legacy test framework; historical features from production code will not be available. "
-                    + "You need to port the test to the new test plugins in order to use historical features from production code. "
-                    + "If this is a legacy feature used only in tests, you can add it to a test-only FeatureSpecification such as %s.",
-            RestTestLegacyFeatures.class.getCanonicalName()
-        );
-        this.historicalFeaturesPredicate = minNodeVersion.<Predicate<String>>map(v -> featureId -> {
-            assert allHistoricalFeatures.contains(featureId) : Strings.format("Unknown historical feature %s: %s", featureId, errorMessage);
-            return hasHistoricalFeature(historicalFeatures, v, featureId);
-        }).orElse(featureId -> {
-            // We can safely assume that new non-semantic versions (serverless) support all historical features
-            assert allHistoricalFeatures.contains(featureId) : Strings.format("Unknown historical feature %s: %s", featureId, errorMessage);
-            return true;
-        });
+        this.historicalFeaturesPredicate = minNodeVersion.<Predicate<String>>map(
+            v -> featureId -> hasHistoricalFeature(historicalFeatures, v, featureId)
+        ).orElse(featureId -> true); // We can safely assume that new non-semantic versions (serverless) support all historical features
         this.clusterStateFeatures = clusterStateFeatures;
     }