Allow skip test by version OR feature (#21240)

Today these two are considered mutual exclusive but they are not in
practice. For instance a mixed version cluster might not return a
given warning depending on which node we talk to but on the other hand
some runners might not even support warnings at all so the test might be
skipped either by version or by feature.

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java

@@ -108,7 +108,7 @@ public abstract class ESClientYamlSuiteTestCase extends ESRestTestCase {
         for (String entry : blacklist) {
             this.blacklistPathMatchers.add(new BlacklistedPathPatternMatcher(entry));
         }
-        
+
     }
 
     @Override
@@ -267,27 +267,16 @@ public abstract class ESClientYamlSuiteTestCase extends ESRestTestCase {
         restTestExecutionContext.clear();
 
         //skip test if the whole suite (yaml file) is disabled
-        assumeFalse(buildSkipMessage(testCandidate.getSuitePath(), testCandidate.getSetupSection().getSkipSection()),
+        assumeFalse(testCandidate.getSetupSection().getSkipSection().getSkipMessage(testCandidate.getSuitePath()),
                 testCandidate.getSetupSection().getSkipSection().skip(restTestExecutionContext.esVersion()));
         //skip test if the whole suite (yaml file) is disabled
-        assumeFalse(buildSkipMessage(testCandidate.getSuitePath(), testCandidate.getTeardownSection().getSkipSection()),
+        assumeFalse(testCandidate.getTeardownSection().getSkipSection().getSkipMessage(testCandidate.getSuitePath()),
             testCandidate.getTeardownSection().getSkipSection().skip(restTestExecutionContext.esVersion()));
         //skip test if test section is disabled
-        assumeFalse(buildSkipMessage(testCandidate.getTestPath(), testCandidate.getTestSection().getSkipSection()),
+        assumeFalse(testCandidate.getTestSection().getSkipSection().getSkipMessage(testCandidate.getTestPath()),
                 testCandidate.getTestSection().getSkipSection().skip(restTestExecutionContext.esVersion()));
     }
 
-    private static String buildSkipMessage(String description, SkipSection skipSection) {
-        StringBuilder messageBuilder = new StringBuilder();
-        if (skipSection.isVersionCheck()) {
-            messageBuilder.append("[").append(description).append("] skipped, reason: [").append(skipSection.getReason()).append("] ");
-        } else {
-            messageBuilder.append("[").append(description).append("] skipped, reason: features ")
-                    .append(skipSection.getFeatures()).append(" not supported");
-        }
-        return messageBuilder.toString();
-    }
-
     public void test() throws IOException {
         //let's check that there is something to run, otherwise there might be a problem with the test section
         if (testCandidate.getTestSection().getExecutableSections().size() == 0) {

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParser.java

@@ -70,9 +70,6 @@ public class SkipSectionParser implements ClientYamlTestFragmentParser<SkipSecti
         if (!Strings.hasLength(version) && features.isEmpty()) {
             throw new ClientYamlTestParseException("version or features is mandatory within skip section");
         }
-        if (Strings.hasLength(version) && !features.isEmpty()) {
-            throw new ClientYamlTestParseException("version or features are mutually exclusive");
-        }
         if (Strings.hasLength(version) && !Strings.hasLength(reason)) {
             throw new ClientYamlTestParseException("reason is mandatory within skip version section");
         }

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java

@@ -39,7 +39,7 @@ public class SkipSection {
     private final Version upperVersion;
     private final List<String> features;
     private final String reason;
-    
+
     private SkipSection() {
         this.lowerVersion = null;
         this.upperVersion = null;
@@ -49,7 +49,6 @@ public class SkipSection {
 
     public SkipSection(String versionRange, List<String> features, String reason) {
         assert features != null;
-        assert versionRange != null && features.isEmpty() || versionRange == null && features.isEmpty() == false;
         Version[] versions = parseVersionRange(versionRange);
         this.lowerVersion = versions[0];
         this.upperVersion = versions[1];
@@ -60,7 +59,7 @@ public class SkipSection {
     public Version getLowerVersion() {
         return lowerVersion;
     }
-    
+
     public Version getUpperVersion() {
         return upperVersion;
     }
@@ -77,11 +76,10 @@ public class SkipSection {
         if (isEmpty()) {
             return false;
         }
-        if (isVersionCheck()) {
-            return currentVersion.onOrAfter(lowerVersion) && currentVersion.onOrBefore(upperVersion);
-        } else {
-            return Features.areAllSupported(features) == false;
-        }
+        boolean skip = lowerVersion != null && upperVersion != null && currentVersion.onOrAfter(lowerVersion)
+            && currentVersion.onOrBefore(upperVersion);
+        skip |= Features.areAllSupported(features) == false;
+        return skip;
     }
 
     public boolean isVersionCheck() {
@@ -91,7 +89,7 @@ public class SkipSection {
     public boolean isEmpty() {
         return EMPTY.equals(this);
     }
-    
+
     private Version[] parseVersionRange(String versionRange) {
         if (versionRange == null) {
             return new Version[] { null, null };
@@ -111,4 +109,16 @@ public class SkipSection {
             upper.isEmpty() ? Version.CURRENT : Version.fromString(upper)
         };
     }
+
+    public String getSkipMessage(String description) {
+        StringBuilder messageBuilder = new StringBuilder();
+        messageBuilder.append("[").append(description).append("] skipped,");
+        if (reason != null) {
+             messageBuilder.append(" reason: [").append(getReason()).append("]");
+        }
+        if (features.isEmpty() == false) {
+            messageBuilder.append(" unsupported features ").append(getFeatures());
+        }
+        return messageBuilder.toString();
+    }
 }

test/framework/src/test/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParserTests.java

@@ -26,6 +26,8 @@ import org.elasticsearch.test.rest.yaml.parser.ClientYamlTestSuiteParseContext;
 import org.elasticsearch.test.rest.yaml.parser.SkipSectionParser;
 import org.elasticsearch.test.rest.yaml.section.SkipSection;
 
+import java.util.Arrays;
+
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
@@ -108,13 +110,11 @@ public class SkipSectionParserTests extends AbstractParserTestCase {
         );
 
         SkipSectionParser skipSectionParser = new SkipSectionParser();
-
-        try {
-            skipSectionParser.parse(new ClientYamlTestSuiteParseContext("api", "suite", parser));
-            fail("Expected RestTestParseException");
-        } catch (ClientYamlTestParseException e) {
-            assertThat(e.getMessage(), is("version or features are mutually exclusive"));
-        }
+        SkipSection parse = skipSectionParser.parse(new ClientYamlTestSuiteParseContext("api", "suite", parser));
+        assertEquals(VersionUtils.getFirstVersion(), parse.getLowerVersion());
+        assertEquals(Version.fromString("0.90.2"), parse.getUpperVersion());
+        assertEquals(Arrays.asList("regex"), parse.getFeatures());
+        assertEquals("Delete ignores the parent param", parse.getReason());
     }
 
     public void testParseSkipSectionNoReason() throws Exception {

test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/SkipSectionTests.java

@@ -0,0 +1,47 @@
+/*
+ * 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.rest.yaml.section;
+
+import org.elasticsearch.Version;
+import org.elasticsearch.test.ESTestCase;
+
+import java.util.Arrays;
+import java.util.Collections;
+
+public class SkipSectionTests extends ESTestCase {
+
+    public void testSkip() {
+        SkipSection section = new SkipSection("2.0.0 - 2.1.0", randomBoolean() ? Collections.emptyList() :
+        Arrays.asList("warnings"), "foobar");
+        assertFalse(section.skip(Version.CURRENT));
+        assertTrue(section.skip(Version.V_2_0_0));
+        section = new SkipSection(randomBoolean() ? null : "2.0.0 - 2.1.0", Arrays.asList("boom"), "foobar");
+        assertTrue(section.skip(Version.CURRENT));
+    }
+
+    public void testMessage() {
+        SkipSection section = new SkipSection("2.0.0 - 2.1.0", Arrays.asList("warnings"), "foobar");
+        assertEquals("[FOOBAR] skipped, reason: [foobar] unsupported features [warnings]", section.getSkipMessage("FOOBAR"));
+        section = new SkipSection(null, Arrays.asList("warnings"), "foobar");
+        assertEquals("[FOOBAR] skipped, reason: [foobar] unsupported features [warnings]", section.getSkipMessage("FOOBAR"));
+        section = new SkipSection(null, Arrays.asList("warnings"), null);
+        assertEquals("[FOOBAR] skipped, unsupported features [warnings]", section.getSkipMessage("FOOBAR"));
+    }
+}