From cf1457ed22113fe01ff2002392e2d5d4030dc07c Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Wed, 2 Nov 2016 12:24:20 +0100 Subject: [PATCH] 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. --- .../rest/yaml/ESClientYamlSuiteTestCase.java | 19 ++------ .../rest/yaml/parser/SkipSectionParser.java | 3 -- .../test/rest/yaml/section/SkipSection.java | 28 +++++++---- .../yaml/parser/SkipSectionParserTests.java | 14 +++--- .../rest/yaml/section/SkipSectionTests.java | 47 +++++++++++++++++++ 5 files changed, 77 insertions(+), 34 deletions(-) create mode 100644 test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/SkipSectionTests.java diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java index 2e29721f06e..f44558d7567 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java +++ b/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) { diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParser.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParser.java index 31451dee247..b73edf7d2c6 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParser.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParser.java @@ -70,9 +70,6 @@ public class SkipSectionParser implements ClientYamlTestFragmentParser 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 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(); + } } diff --git a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParserTests.java b/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParserTests.java index f5d46cdd3d6..7473e393e5c 100644 --- a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/parser/SkipSectionParserTests.java +++ b/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 { diff --git a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/SkipSectionTests.java b/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/SkipSectionTests.java new file mode 100644 index 00000000000..c8f7b351282 --- /dev/null +++ b/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")); + } +}