From 1787e428cf10dd4bee3afd886440178608ca2916 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 May 2018 10:10:48 -0400 Subject: [PATCH 01/11] QA: System property to override distribution This configures all `qa` projects to use the distribution contained in the `tests.distribution` system property if it is set. The goal is to create a simple way to run tests against the default distribution which has x-pack basic features enabled while not forcing these tests on all contributors. You run these tests by doing something like: ``` ./gradlew -p qa -Dtests.distribution=zip check ``` or ``` ./gradlew -p qa -Dtests.distribution=zip bwcTest ``` x-pack basic *shouldn't* get in the way of any of these tests but nothing is ever perfect so this also adds support for skipping tests based on the distribution. For the yaml tests this looks like: ``` - skip: distribution: zip reason: x-pack includes templates which breaks the output here ``` For java tests it looks something like: ``` assumeFalse("zip".equals(System.getProperty("tests.distribution")), "some reason"); ``` Gradle makes sure to set the `tests.distribution` environment variable when running tests whether or not you override the distribution on the command line. This allows us to remove some `tests.rest.blacklist` settings in `x-pack/qa` because those tests skip themselves based on the distribution. I think this sort of thing substantially reduces the "surprise factor" while working on x-pack. --- .../gradle/test/RestIntegTestTask.groovy | 4 +- qa/build.gradle | 2 +- .../rest-api-spec/test/README.asciidoc | 10 ++++ .../test/cat.templates/10_basic.yml | 11 ++++ .../junit/listeners/ReproduceInfoPrinter.java | 1 + .../test/rest/yaml/section/SkipSection.java | 41 ++++++++++++-- .../section/ClientYamlTestSectionTests.java | 4 +- .../rest/yaml/section/SkipSectionTests.java | 56 ++++++++++++++++--- .../build.gradle | 3 - 9 files changed, 110 insertions(+), 22 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy index 242ed45eee86e..d14cd48d63d92 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy @@ -78,6 +78,8 @@ public class RestIntegTestTask extends DefaultTask { // that sets up the test cluster and passes this transport uri instead of http uri. Until then, we pass // both as separate sysprops runner.systemProperty('tests.cluster', "${-> nodes[0].transportUri()}") + // always pass in the distribution so we can see it in things like the yaml `skip` section + runner.systemProperty('tests.distribution', "${-> clusterConfig.distribution}") // dump errors and warnings from cluster log on failure TaskExecutionAdapter logDumpListener = new TaskExecutionAdapter() { @@ -100,7 +102,7 @@ public class RestIntegTestTask extends DefaultTask { // copy the rest spec/tests into the test resources Task copyRestSpec = createCopyRestSpecTask(project, includePackaged) runner.dependsOn(copyRestSpec) - + // this must run after all projects have been configured, so we know any project // references can be accessed as a fully configured project.gradle.projectsEvaluated { diff --git a/qa/build.gradle b/qa/build.gradle index 494f6e3cd94b7..709c309359ecf 100644 --- a/qa/build.gradle +++ b/qa/build.gradle @@ -4,7 +4,7 @@ import org.elasticsearch.gradle.test.RestIntegTestTask subprojects { Project subproj -> subproj.tasks.withType(RestIntegTestTask) { subproj.extensions.configure("${it.name}Cluster") { cluster -> - cluster.distribution = 'oss-zip' + cluster.distribution = System.getProperty('tests.distribution', 'oss-zip') } } } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc b/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc index c93873a5be429..df3c7e787a256 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc @@ -132,6 +132,16 @@ by default, thus the related skip sections can be removed from the tests. The `features` field can either be a string or an array of strings. The skip section requires to specify either a `version` or a `features` list. +The skip section can also skip tests when the running against a particular +distribution. This is useful for tests that are fine running against oss-zip +or integ-test-zip but not when x-pack is present. Example: + +.... + - skip: + distribution: zip + reason: x-pack includes templates which breaks the output here +.... + Required operators: ------------------- diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml index 403b0b740c533..58c85ed41963c 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml @@ -14,6 +14,9 @@ --- "No templates": + - skip: + distribution: zip + reason: x-pack includes templates which breaks the output here - do: cat.templates: {} @@ -174,6 +177,10 @@ --- "Sort templates": + - skip: + distribution: zip + reason: x-pack includes templates which breaks the output here + - do: indices.put_template: name: test @@ -222,6 +229,10 @@ --- "Multiple template": + - skip: + distribution: zip + reason: x-pack includes templates which breaks the output here + - do: indices.put_template: name: test_1 diff --git a/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java b/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java index ca16ac6204a90..877a6f2e98a67 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java +++ b/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java @@ -149,6 +149,7 @@ public ReproduceErrorMessageBuilder appendESProperties() { } appendOpt("tests.locale", Locale.getDefault().toLanguageTag()); appendOpt("tests.timezone", TimeZone.getDefault().getID()); + appendOpt("tests.distribution", System.getProperty("tests.distribution")); return this; } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java index eb1fea4b79aed..493f7a1fcf2c5 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java @@ -60,6 +60,7 @@ public static SkipSection parse(XContentParser parser) throws IOException { XContentParser.Token token; String version = null; String reason = null; + String distribution = null; List features = new ArrayList<>(); while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { @@ -71,6 +72,8 @@ public static SkipSection parse(XContentParser parser) throws IOException { reason = parser.text(); } else if ("features".equals(currentFieldName)) { features.add(parser.text()); + } else if ("distribution".equals(currentFieldName)) { + distribution = parser.text(); } else { throw new ParsingException(parser.getTokenLocation(), @@ -87,13 +90,17 @@ public static SkipSection parse(XContentParser parser) throws IOException { parser.nextToken(); - if (!Strings.hasLength(version) && features.isEmpty()) { - throw new ParsingException(parser.getTokenLocation(), "version or features is mandatory within skip section"); + if (!Strings.hasLength(version) && features.isEmpty() && distribution == null) { + throw new ParsingException(parser.getTokenLocation(), + "version or features or distribution is mandatory within skip section"); } if (Strings.hasLength(version) && !Strings.hasLength(reason)) { throw new ParsingException(parser.getTokenLocation(), "reason is mandatory within skip version section"); } - return new SkipSection(version, features, reason); + if (Strings.hasLength(distribution) && !Strings.hasLength(reason)) { + throw new ParsingException(parser.getTokenLocation(), "reason is mandatory within skip distribution section"); + } + return new SkipSection(version, features, distribution, reason); } public static final SkipSection EMPTY = new SkipSection(); @@ -101,21 +108,28 @@ public static SkipSection parse(XContentParser parser) throws IOException { private final Version lowerVersion; private final Version upperVersion; private final List features; + /** + * Skip the test if the distribution has been overridden to a + * particular value. + */ + private final String distribution; private final String reason; private SkipSection() { this.lowerVersion = null; this.upperVersion = null; this.features = new ArrayList<>(); + this.distribution = null; this.reason = null; } - public SkipSection(String versionRange, List features, String reason) { + public SkipSection(String versionRange, List features, String distribution, String reason) { assert features != null; Version[] versions = parseVersionRange(versionRange); this.lowerVersion = versions[0]; this.upperVersion = versions[1]; this.features = features; + this.distribution = distribution; this.reason = reason; } @@ -131,6 +145,14 @@ public List getFeatures() { return features; } + /** + * If non-null then this will skip the test when the distribution + * is overridden to this value. + */ + public String getDistribution() { + return distribution; + } + public String getReason() { return reason; } @@ -142,11 +164,15 @@ public boolean skip(Version currentVersion) { boolean skip = lowerVersion != null && upperVersion != null && currentVersion.onOrAfter(lowerVersion) && currentVersion.onOrBefore(upperVersion); skip |= Features.areAllSupported(features) == false; + skip |= distribution != null && distribution.equals(getDistributionOverrideValue()); return skip; } - public boolean isVersionCheck() { - return features.isEmpty(); + /** + * Get the value that the distribution was overridden to. + */ + protected String getDistributionOverrideValue() { + return System.getProperty("tests.distribution"); } public boolean isEmpty() { @@ -182,6 +208,9 @@ public String getSkipMessage(String description) { if (features.isEmpty() == false) { messageBuilder.append(" unsupported features ").append(getFeatures()); } + if (distribution != null) { + messageBuilder.append(" unsupported distribution [").append(getDistribution()).append(']'); + } return messageBuilder.toString(); } } diff --git a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSectionTests.java b/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSectionTests.java index ecee131c7a28e..5f4e551f8eed7 100644 --- a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSectionTests.java +++ b/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSectionTests.java @@ -46,7 +46,7 @@ public void testAddingDoWithoutWarningWithoutSkip() { public void testAddingDoWithWarningWithSkip() { int lineNumber = between(1, 10000); ClientYamlTestSection section = new ClientYamlTestSection(new XContentLocation(0, 0), "test"); - section.setSkipSection(new SkipSection(null, singletonList("warnings"), null)); + section.setSkipSection(new SkipSection(null, singletonList("warnings"), null, null)); DoSection doSection = new DoSection(new XContentLocation(lineNumber, 0)); doSection.setExpectedWarningHeaders(singletonList("foo")); section.addExecutableSection(doSection); @@ -55,7 +55,7 @@ public void testAddingDoWithWarningWithSkip() { public void testAddingDoWithWarningWithSkipButNotWarnings() { int lineNumber = between(1, 10000); ClientYamlTestSection section = new ClientYamlTestSection(new XContentLocation(0, 0), "test"); - section.setSkipSection(new SkipSection(null, singletonList("yaml"), null)); + section.setSkipSection(new SkipSection(null, singletonList("yaml"), null, null)); DoSection doSection = new DoSection(new XContentLocation(lineNumber, 0)); doSection.setExpectedWarningHeaders(singletonList("foo")); Exception e = expectThrows(IllegalArgumentException.class, () -> section.addExecutableSection(doSection)); 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 index 3ab9583335e7c..66531fcee670b 100644 --- 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 @@ -25,6 +25,7 @@ import org.elasticsearch.test.VersionUtils; import java.util.Collections; +import java.util.concurrent.atomic.AtomicReference; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -35,22 +36,39 @@ public class SkipSectionTests extends AbstractClientYamlTestFragmentParserTestCa public void testSkip() { SkipSection section = new SkipSection("5.0.0 - 5.1.0", - randomBoolean() ? Collections.emptyList() : Collections.singletonList("warnings"), "foobar"); + randomBoolean() ? Collections.emptyList() : Collections.singletonList("warnings"), "foobar", "foobaz"); assertFalse(section.skip(Version.CURRENT)); assertTrue(section.skip(Version.V_5_0_0)); section = new SkipSection(randomBoolean() ? null : "5.0.0 - 5.1.0", - Collections.singletonList("boom"), "foobar"); + Collections.singletonList("boom"), "foobar", "foobaz"); + assertTrue(section.skip(Version.CURRENT)); + + AtomicReference distribution = new AtomicReference<>(null); + section = new SkipSection(null, Collections.emptyList(), "zip", "reason") { + @Override + protected String getDistributionOverrideValue() { + return distribution.get(); + } + }; + assertFalse(section.skip(Version.CURRENT)); + distribution.set("something"); + assertFalse(section.skip(Version.CURRENT)); + distribution.set("zip"); assertTrue(section.skip(Version.CURRENT)); } public void testMessage() { SkipSection section = new SkipSection("5.0.0 - 5.1.0", - Collections.singletonList("warnings"), "foobar"); - assertEquals("[FOOBAR] skipped, reason: [foobar] unsupported features [warnings]", section.getSkipMessage("FOOBAR")); - section = new SkipSection(null, Collections.singletonList("warnings"), "foobar"); + Collections.singletonList("warnings"), "zip", "foobar"); + assertEquals("[FOOBAR] skipped, reason: [foobar] unsupported features [warnings] unsupported distribution [zip]", + section.getSkipMessage("FOOBAR")); + section = new SkipSection(null, Collections.singletonList("warnings"), null, "foobar"); assertEquals("[FOOBAR] skipped, reason: [foobar] unsupported features [warnings]", section.getSkipMessage("FOOBAR")); - section = new SkipSection(null, Collections.singletonList("warnings"), null); + section = new SkipSection(null, Collections.singletonList("warnings"), null, null); assertEquals("[FOOBAR] skipped, unsupported features [warnings]", section.getSkipMessage("FOOBAR")); + section = new SkipSection(null, Collections.emptyList(), "blah-distro", "blah-distro is lame"); + assertEquals("[FOOBAR] skipped, reason: [blah-distro is lame] unsupported distribution [blah-distro]", + section.getSkipMessage("FOOBAR")); } public void testParseSkipSectionVersionNoFeature() throws Exception { @@ -88,7 +106,6 @@ public void testParseSkipSectionFeatureNoVersion() throws Exception { SkipSection skipSection = SkipSection.parse(parser); assertThat(skipSection, notNullValue()); - assertThat(skipSection.isVersionCheck(), equalTo(false)); assertThat(skipSection.getFeatures().size(), equalTo(1)); assertThat(skipSection.getFeatures().get(0), equalTo("regex")); assertThat(skipSection.getReason(), nullValue()); @@ -101,7 +118,6 @@ public void testParseSkipSectionFeaturesNoVersion() throws Exception { SkipSection skipSection = SkipSection.parse(parser); assertThat(skipSection, notNullValue()); - assertThat(skipSection.isVersionCheck(), equalTo(false)); assertThat(skipSection.getFeatures().size(), equalTo(3)); assertThat(skipSection.getFeatures().get(0), equalTo("regex1")); assertThat(skipSection.getFeatures().get(1), equalTo("regex2")); @@ -138,6 +154,28 @@ public void testParseSkipSectionNoVersionNorFeature() throws Exception { ); Exception e = expectThrows(ParsingException.class, () -> SkipSection.parse(parser)); - assertThat(e.getMessage(), is("version or features is mandatory within skip section")); + assertThat(e.getMessage(), is("version or features or distribution is mandatory within skip section")); + } + + public void testParseSkipSectionDistribution() throws Exception { + parser = createParser(YamlXContent.yamlXContent, + "distribution: zip\n" + + "reason: Zip distribution differs because XYZ\n" + ); + + SkipSection skipSection = SkipSection.parse(parser); + assertThat(skipSection, notNullValue()); + assertThat(skipSection.getFeatures(), equalTo(Collections.emptyList())); + assertThat(skipSection.getDistribution(), equalTo("zip")); + assertThat(skipSection.getReason(), equalTo("Zip distribution differs because XYZ")); + } + + public void testParseSkipSectionDistributionNoReason() throws Exception { + parser = createParser(YamlXContent.yamlXContent, + "distribution: zip\n" + ); + + Exception e = expectThrows(ParsingException.class, () -> SkipSection.parse(parser)); + assertThat(e.getMessage(), is("reason is mandatory within skip distribution section")); } } diff --git a/x-pack/qa/core-rest-tests-with-security/build.gradle b/x-pack/qa/core-rest-tests-with-security/build.gradle index c23432e5127f7..2c39ff45b88ac 100644 --- a/x-pack/qa/core-rest-tests-with-security/build.gradle +++ b/x-pack/qa/core-rest-tests-with-security/build.gradle @@ -15,9 +15,6 @@ integTestRunner { ['cat.aliases/10_basic/Empty cluster', 'index/10_with_id/Index with ID', 'indices.get_alias/10_basic/Get alias against closed indices', - 'cat.templates/10_basic/No templates', - 'cat.templates/10_basic/Sort templates', - 'cat.templates/10_basic/Multiple template', ].join(',') } From d2d384e1d3f9403d31994733bbbec1690501429b Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 May 2018 15:24:16 -0400 Subject: [PATCH 02/11] Update message --- .../resources/rest-api-spec/test/cat.templates/10_basic.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml index 43fec2d5f87d0..ef0d7e35c10bc 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml @@ -17,7 +17,7 @@ - skip: features: default_shards distribution: zip - reason: x-pack includes templates which breaks the output here and default_shards adds a template which confuses this + reason: x-pack includes templates which breaks the output here - do: cat.templates: {} @@ -182,7 +182,7 @@ - skip: features: default_shards distribution: zip - reason: x-pack includes templates which breaks the output here and default_shards adds a template which confuses this + reason: x-pack includes templates which breaks the output here - do: indices.put_template: @@ -235,7 +235,7 @@ - skip: features: default_shards distribution: zip - reason: x-pack includes templates which breaks the output here and default_shards adds a template which confuses this + reason: x-pack includes templates which breaks the output here - do: indices.put_template: From 5b64f0ead3d3050b9e442d88ff945f35acbdbca8 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 May 2018 16:52:55 -0400 Subject: [PATCH 03/11] Drop skip change It'd be nice to have it but it'd break clients. --- .../junit/listeners/ReproduceInfoPrinter.java | 1 - .../test/rest/yaml/section/SkipSection.java | 41 ++------------ .../section/ClientYamlTestSectionTests.java | 4 +- .../rest/yaml/section/SkipSectionTests.java | 56 +++---------------- 4 files changed, 17 insertions(+), 85 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java b/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java index 877a6f2e98a67..ca16ac6204a90 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java +++ b/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java @@ -149,7 +149,6 @@ public ReproduceErrorMessageBuilder appendESProperties() { } appendOpt("tests.locale", Locale.getDefault().toLanguageTag()); appendOpt("tests.timezone", TimeZone.getDefault().getID()); - appendOpt("tests.distribution", System.getProperty("tests.distribution")); return this; } diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java index 493f7a1fcf2c5..eb1fea4b79aed 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java @@ -60,7 +60,6 @@ public static SkipSection parse(XContentParser parser) throws IOException { XContentParser.Token token; String version = null; String reason = null; - String distribution = null; List features = new ArrayList<>(); while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { @@ -72,8 +71,6 @@ public static SkipSection parse(XContentParser parser) throws IOException { reason = parser.text(); } else if ("features".equals(currentFieldName)) { features.add(parser.text()); - } else if ("distribution".equals(currentFieldName)) { - distribution = parser.text(); } else { throw new ParsingException(parser.getTokenLocation(), @@ -90,17 +87,13 @@ public static SkipSection parse(XContentParser parser) throws IOException { parser.nextToken(); - if (!Strings.hasLength(version) && features.isEmpty() && distribution == null) { - throw new ParsingException(parser.getTokenLocation(), - "version or features or distribution is mandatory within skip section"); + if (!Strings.hasLength(version) && features.isEmpty()) { + throw new ParsingException(parser.getTokenLocation(), "version or features is mandatory within skip section"); } if (Strings.hasLength(version) && !Strings.hasLength(reason)) { throw new ParsingException(parser.getTokenLocation(), "reason is mandatory within skip version section"); } - if (Strings.hasLength(distribution) && !Strings.hasLength(reason)) { - throw new ParsingException(parser.getTokenLocation(), "reason is mandatory within skip distribution section"); - } - return new SkipSection(version, features, distribution, reason); + return new SkipSection(version, features, reason); } public static final SkipSection EMPTY = new SkipSection(); @@ -108,28 +101,21 @@ public static SkipSection parse(XContentParser parser) throws IOException { private final Version lowerVersion; private final Version upperVersion; private final List features; - /** - * Skip the test if the distribution has been overridden to a - * particular value. - */ - private final String distribution; private final String reason; private SkipSection() { this.lowerVersion = null; this.upperVersion = null; this.features = new ArrayList<>(); - this.distribution = null; this.reason = null; } - public SkipSection(String versionRange, List features, String distribution, String reason) { + public SkipSection(String versionRange, List features, String reason) { assert features != null; Version[] versions = parseVersionRange(versionRange); this.lowerVersion = versions[0]; this.upperVersion = versions[1]; this.features = features; - this.distribution = distribution; this.reason = reason; } @@ -145,14 +131,6 @@ public List getFeatures() { return features; } - /** - * If non-null then this will skip the test when the distribution - * is overridden to this value. - */ - public String getDistribution() { - return distribution; - } - public String getReason() { return reason; } @@ -164,15 +142,11 @@ public boolean skip(Version currentVersion) { boolean skip = lowerVersion != null && upperVersion != null && currentVersion.onOrAfter(lowerVersion) && currentVersion.onOrBefore(upperVersion); skip |= Features.areAllSupported(features) == false; - skip |= distribution != null && distribution.equals(getDistributionOverrideValue()); return skip; } - /** - * Get the value that the distribution was overridden to. - */ - protected String getDistributionOverrideValue() { - return System.getProperty("tests.distribution"); + public boolean isVersionCheck() { + return features.isEmpty(); } public boolean isEmpty() { @@ -208,9 +182,6 @@ public String getSkipMessage(String description) { if (features.isEmpty() == false) { messageBuilder.append(" unsupported features ").append(getFeatures()); } - if (distribution != null) { - messageBuilder.append(" unsupported distribution [").append(getDistribution()).append(']'); - } return messageBuilder.toString(); } } diff --git a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSectionTests.java b/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSectionTests.java index 5f4e551f8eed7..ecee131c7a28e 100644 --- a/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSectionTests.java +++ b/test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSectionTests.java @@ -46,7 +46,7 @@ public void testAddingDoWithoutWarningWithoutSkip() { public void testAddingDoWithWarningWithSkip() { int lineNumber = between(1, 10000); ClientYamlTestSection section = new ClientYamlTestSection(new XContentLocation(0, 0), "test"); - section.setSkipSection(new SkipSection(null, singletonList("warnings"), null, null)); + section.setSkipSection(new SkipSection(null, singletonList("warnings"), null)); DoSection doSection = new DoSection(new XContentLocation(lineNumber, 0)); doSection.setExpectedWarningHeaders(singletonList("foo")); section.addExecutableSection(doSection); @@ -55,7 +55,7 @@ public void testAddingDoWithWarningWithSkip() { public void testAddingDoWithWarningWithSkipButNotWarnings() { int lineNumber = between(1, 10000); ClientYamlTestSection section = new ClientYamlTestSection(new XContentLocation(0, 0), "test"); - section.setSkipSection(new SkipSection(null, singletonList("yaml"), null, null)); + section.setSkipSection(new SkipSection(null, singletonList("yaml"), null)); DoSection doSection = new DoSection(new XContentLocation(lineNumber, 0)); doSection.setExpectedWarningHeaders(singletonList("foo")); Exception e = expectThrows(IllegalArgumentException.class, () -> section.addExecutableSection(doSection)); 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 index 66531fcee670b..3ab9583335e7c 100644 --- 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 @@ -25,7 +25,6 @@ import org.elasticsearch.test.VersionUtils; import java.util.Collections; -import java.util.concurrent.atomic.AtomicReference; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -36,39 +35,22 @@ public class SkipSectionTests extends AbstractClientYamlTestFragmentParserTestCa public void testSkip() { SkipSection section = new SkipSection("5.0.0 - 5.1.0", - randomBoolean() ? Collections.emptyList() : Collections.singletonList("warnings"), "foobar", "foobaz"); + randomBoolean() ? Collections.emptyList() : Collections.singletonList("warnings"), "foobar"); assertFalse(section.skip(Version.CURRENT)); assertTrue(section.skip(Version.V_5_0_0)); section = new SkipSection(randomBoolean() ? null : "5.0.0 - 5.1.0", - Collections.singletonList("boom"), "foobar", "foobaz"); - assertTrue(section.skip(Version.CURRENT)); - - AtomicReference distribution = new AtomicReference<>(null); - section = new SkipSection(null, Collections.emptyList(), "zip", "reason") { - @Override - protected String getDistributionOverrideValue() { - return distribution.get(); - } - }; - assertFalse(section.skip(Version.CURRENT)); - distribution.set("something"); - assertFalse(section.skip(Version.CURRENT)); - distribution.set("zip"); + Collections.singletonList("boom"), "foobar"); assertTrue(section.skip(Version.CURRENT)); } public void testMessage() { SkipSection section = new SkipSection("5.0.0 - 5.1.0", - Collections.singletonList("warnings"), "zip", "foobar"); - assertEquals("[FOOBAR] skipped, reason: [foobar] unsupported features [warnings] unsupported distribution [zip]", - section.getSkipMessage("FOOBAR")); - section = new SkipSection(null, Collections.singletonList("warnings"), null, "foobar"); + Collections.singletonList("warnings"), "foobar"); + assertEquals("[FOOBAR] skipped, reason: [foobar] unsupported features [warnings]", section.getSkipMessage("FOOBAR")); + section = new SkipSection(null, Collections.singletonList("warnings"), "foobar"); assertEquals("[FOOBAR] skipped, reason: [foobar] unsupported features [warnings]", section.getSkipMessage("FOOBAR")); - section = new SkipSection(null, Collections.singletonList("warnings"), null, null); + section = new SkipSection(null, Collections.singletonList("warnings"), null); assertEquals("[FOOBAR] skipped, unsupported features [warnings]", section.getSkipMessage("FOOBAR")); - section = new SkipSection(null, Collections.emptyList(), "blah-distro", "blah-distro is lame"); - assertEquals("[FOOBAR] skipped, reason: [blah-distro is lame] unsupported distribution [blah-distro]", - section.getSkipMessage("FOOBAR")); } public void testParseSkipSectionVersionNoFeature() throws Exception { @@ -106,6 +88,7 @@ public void testParseSkipSectionFeatureNoVersion() throws Exception { SkipSection skipSection = SkipSection.parse(parser); assertThat(skipSection, notNullValue()); + assertThat(skipSection.isVersionCheck(), equalTo(false)); assertThat(skipSection.getFeatures().size(), equalTo(1)); assertThat(skipSection.getFeatures().get(0), equalTo("regex")); assertThat(skipSection.getReason(), nullValue()); @@ -118,6 +101,7 @@ public void testParseSkipSectionFeaturesNoVersion() throws Exception { SkipSection skipSection = SkipSection.parse(parser); assertThat(skipSection, notNullValue()); + assertThat(skipSection.isVersionCheck(), equalTo(false)); assertThat(skipSection.getFeatures().size(), equalTo(3)); assertThat(skipSection.getFeatures().get(0), equalTo("regex1")); assertThat(skipSection.getFeatures().get(1), equalTo("regex2")); @@ -154,28 +138,6 @@ public void testParseSkipSectionNoVersionNorFeature() throws Exception { ); Exception e = expectThrows(ParsingException.class, () -> SkipSection.parse(parser)); - assertThat(e.getMessage(), is("version or features or distribution is mandatory within skip section")); - } - - public void testParseSkipSectionDistribution() throws Exception { - parser = createParser(YamlXContent.yamlXContent, - "distribution: zip\n" + - "reason: Zip distribution differs because XYZ\n" - ); - - SkipSection skipSection = SkipSection.parse(parser); - assertThat(skipSection, notNullValue()); - assertThat(skipSection.getFeatures(), equalTo(Collections.emptyList())); - assertThat(skipSection.getDistribution(), equalTo("zip")); - assertThat(skipSection.getReason(), equalTo("Zip distribution differs because XYZ")); - } - - public void testParseSkipSectionDistributionNoReason() throws Exception { - parser = createParser(YamlXContent.yamlXContent, - "distribution: zip\n" - ); - - Exception e = expectThrows(ParsingException.class, () -> SkipSection.parse(parser)); - assertThat(e.getMessage(), is("reason is mandatory within skip distribution section")); + assertThat(e.getMessage(), is("version or features is mandatory within skip section")); } } From 9cc15bd050efaab941ea5dfa8478d7ad0be1aa71 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 May 2018 16:53:55 -0400 Subject: [PATCH 04/11] Drop skip changes --- .../org/elasticsearch/gradle/test/RestIntegTestTask.groovy | 2 -- qa/mixed-cluster/build.gradle | 5 +++++ .../resources/rest-api-spec/test/cat.templates/10_basic.yml | 6 ------ x-pack/qa/core-rest-tests-with-security/build.gradle | 3 +++ 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy index d14cd48d63d92..10d147f838ebd 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy @@ -78,8 +78,6 @@ public class RestIntegTestTask extends DefaultTask { // that sets up the test cluster and passes this transport uri instead of http uri. Until then, we pass // both as separate sysprops runner.systemProperty('tests.cluster', "${-> nodes[0].transportUri()}") - // always pass in the distribution so we can see it in things like the yaml `skip` section - runner.systemProperty('tests.distribution', "${-> clusterConfig.distribution}") // dump errors and warnings from cluster log on failure TaskExecutionAdapter logDumpListener = new TaskExecutionAdapter() { diff --git a/qa/mixed-cluster/build.gradle b/qa/mixed-cluster/build.gradle index d335ac982fd8a..c397e3416f726 100644 --- a/qa/mixed-cluster/build.gradle +++ b/qa/mixed-cluster/build.gradle @@ -57,6 +57,11 @@ for (Version version : bwcVersions.wireCompatible) { /* To support taking index snapshots, we have to set path.repo setting */ tasks.getByName("${baseName}#mixedClusterTestRunner").configure { systemProperty 'tests.path.repo', new File(buildDir, "cluster/shared/repo") + systemProperty 'tests.rest.blacklist', [ + 'cat.templates/10_basic/No templates', + 'cat.templates/10_basic/Sort templates', + 'cat.templates/10_basic/Multiple template', + ].join(',') } } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml index ef0d7e35c10bc..2607b28c16df0 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml @@ -16,8 +16,6 @@ "No templates": - skip: features: default_shards - distribution: zip - reason: x-pack includes templates which breaks the output here - do: cat.templates: {} @@ -181,8 +179,6 @@ "Sort templates": - skip: features: default_shards - distribution: zip - reason: x-pack includes templates which breaks the output here - do: indices.put_template: @@ -234,8 +230,6 @@ "Multiple template": - skip: features: default_shards - distribution: zip - reason: x-pack includes templates which breaks the output here - do: indices.put_template: diff --git a/x-pack/qa/core-rest-tests-with-security/build.gradle b/x-pack/qa/core-rest-tests-with-security/build.gradle index 2c39ff45b88ac..c23432e5127f7 100644 --- a/x-pack/qa/core-rest-tests-with-security/build.gradle +++ b/x-pack/qa/core-rest-tests-with-security/build.gradle @@ -15,6 +15,9 @@ integTestRunner { ['cat.aliases/10_basic/Empty cluster', 'index/10_with_id/Index with ID', 'indices.get_alias/10_basic/Get alias against closed indices', + 'cat.templates/10_basic/No templates', + 'cat.templates/10_basic/Sort templates', + 'cat.templates/10_basic/Multiple template', ].join(',') } From 39440afb6a34c781f04fc65fb370637e2a7af5dc Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 May 2018 17:08:16 -0400 Subject: [PATCH 05/11] revert more --- .../org/elasticsearch/gradle/test/RestIntegTestTask.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy index 10d147f838ebd..242ed45eee86e 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy @@ -100,7 +100,7 @@ public class RestIntegTestTask extends DefaultTask { // copy the rest spec/tests into the test resources Task copyRestSpec = createCopyRestSpecTask(project, includePackaged) runner.dependsOn(copyRestSpec) - + // this must run after all projects have been configured, so we know any project // references can be accessed as a fully configured project.gradle.projectsEvaluated { From 6d9e59043302696bc4f27a67ae1c81b6d834abed Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 May 2018 17:08:51 -0400 Subject: [PATCH 06/11] Revert yet more --- .../main/resources/rest-api-spec/test/README.asciidoc | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc b/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc index df3c7e787a256..c93873a5be429 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/README.asciidoc @@ -132,16 +132,6 @@ by default, thus the related skip sections can be removed from the tests. The `features` field can either be a string or an array of strings. The skip section requires to specify either a `version` or a `features` list. -The skip section can also skip tests when the running against a particular -distribution. This is useful for tests that are fine running against oss-zip -or integ-test-zip but not when x-pack is present. Example: - -.... - - skip: - distribution: zip - reason: x-pack includes templates which breaks the output here -.... - Required operators: ------------------- From f3ef5bf5cdbe2cf545b23f6c608e83d4cd37bd76 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 May 2018 17:09:23 -0400 Subject: [PATCH 07/11] Revert remaining --- .../resources/rest-api-spec/test/cat.templates/10_basic.yml | 3 --- 1 file changed, 3 deletions(-) diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml index 2607b28c16df0..78b7a4277570a 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/cat.templates/10_basic.yml @@ -16,7 +16,6 @@ "No templates": - skip: features: default_shards - - do: cat.templates: {} @@ -179,7 +178,6 @@ "Sort templates": - skip: features: default_shards - - do: indices.put_template: name: test @@ -230,7 +228,6 @@ "Multiple template": - skip: features: default_shards - - do: indices.put_template: name: test_1 From 84655587f2c8a45ea3de9ca92637baea20ffc693 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Mon, 14 May 2018 18:17:16 -0400 Subject: [PATCH 08/11] Skip only if distribution is wrong --- qa/mixed-cluster/build.gradle | 14 ++++++++------ qa/smoke-test-multinode/build.gradle | 10 ++++++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/qa/mixed-cluster/build.gradle b/qa/mixed-cluster/build.gradle index c397e3416f726..da99bbb4c8036 100644 --- a/qa/mixed-cluster/build.gradle +++ b/qa/mixed-cluster/build.gradle @@ -54,14 +54,16 @@ for (Version version : bwcVersions.wireCompatible) { bwcTest.dependsOn(versionBwcTest) } - /* To support taking index snapshots, we have to set path.repo setting */ tasks.getByName("${baseName}#mixedClusterTestRunner").configure { + /* To support taking index snapshots, we have to set path.repo setting */ systemProperty 'tests.path.repo', new File(buildDir, "cluster/shared/repo") - systemProperty 'tests.rest.blacklist', [ - 'cat.templates/10_basic/No templates', - 'cat.templates/10_basic/Sort templates', - 'cat.templates/10_basic/Multiple template', - ].join(',') + if ('zip'.equals(extension.distribution)) { + systemProperty 'tests.rest.blacklist', [ + 'cat.templates/10_basic/No templates', + 'cat.templates/10_basic/Sort templates', + 'cat.templates/10_basic/Multiple template', + ].join(',') + } } } diff --git a/qa/smoke-test-multinode/build.gradle b/qa/smoke-test-multinode/build.gradle index 5df77bd0d9513..9d299e16f0210 100644 --- a/qa/smoke-test-multinode/build.gradle +++ b/qa/smoke-test-multinode/build.gradle @@ -27,3 +27,13 @@ integTest { integTestCluster { numNodes = 2 } + +integTestRunner { + if ('zip'.equals(integTestCluster.distribution)) { + systemProperty 'tests.rest.blacklist', [ + 'cat.templates/10_basic/No templates', + 'cat.templates/10_basic/Sort templates', + 'cat.templates/10_basic/Multiple template', + ].join(',') + } +} From 62b05005338a3d744150a957c7d19b24931563a9 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 15 May 2018 07:53:44 -0400 Subject: [PATCH 09/11] fix rank eval --- qa/smoke-test-rank-eval-with-mustache/build.gradle | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/qa/smoke-test-rank-eval-with-mustache/build.gradle b/qa/smoke-test-rank-eval-with-mustache/build.gradle index 7274e65f4e1bd..3e5d534f4b88c 100644 --- a/qa/smoke-test-rank-eval-with-mustache/build.gradle +++ b/qa/smoke-test-rank-eval-with-mustache/build.gradle @@ -26,3 +26,10 @@ dependencies { testCompile project(path: ':modules:lang-mustache', configuration: 'runtime') } +/* + * One of the integration tests doesn't work with the zip distribution + * and will be fixed later. + */ +if ("zip".equals(integTestCluster.distribution)) { + integTestRunner.enabled = false +} From 25c81a350ed3e268e7c012f7c76ee7840c107de7 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 15 May 2018 10:17:00 -0400 Subject: [PATCH 10/11] Add reproduce info --- .../elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java b/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java index ca16ac6204a90..877a6f2e98a67 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java +++ b/test/framework/src/main/java/org/elasticsearch/test/junit/listeners/ReproduceInfoPrinter.java @@ -149,6 +149,7 @@ public ReproduceErrorMessageBuilder appendESProperties() { } appendOpt("tests.locale", Locale.getDefault().toLanguageTag()); appendOpt("tests.timezone", TimeZone.getDefault().getID()); + appendOpt("tests.distribution", System.getProperty("tests.distribution")); return this; } From d38e00a94bda4bc26ff0fbe32756fbe78e4dd11a Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 15 May 2018 17:09:38 -0400 Subject: [PATCH 11/11] track --- qa/smoke-test-rank-eval-with-mustache/build.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/qa/smoke-test-rank-eval-with-mustache/build.gradle b/qa/smoke-test-rank-eval-with-mustache/build.gradle index 3e5d534f4b88c..122c2603719a0 100644 --- a/qa/smoke-test-rank-eval-with-mustache/build.gradle +++ b/qa/smoke-test-rank-eval-with-mustache/build.gradle @@ -29,6 +29,7 @@ dependencies { /* * One of the integration tests doesn't work with the zip distribution * and will be fixed later. + * Tracked by https://github.com/elastic/elasticsearch/issues/30628 */ if ("zip".equals(integTestCluster.distribution)) { integTestRunner.enabled = false