Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QA: System property to override distribution #30591

Merged
merged 12 commits into from
May 15, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion qa/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first I thought this PR would be just this line. If only things were that simple....

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And now that I've reverted most of the rest, it is almost the only line!

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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:
-------------------

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
"No templates":
- skip:
features: default_shards
distribution: zip
reason: x-pack includes templates which breaks the output here

- do:
cat.templates: {}

Expand Down Expand Up @@ -178,6 +181,9 @@
"Sort templates":
- skip:
features: default_shards
distribution: zip
reason: x-pack includes templates which breaks the output here

- do:
indices.put_template:
name: test
Expand Down Expand Up @@ -228,6 +234,9 @@
"Multiple template":
- skip:
features: default_shards
distribution: zip
reason: x-pack includes templates which breaks the output here

- do:
indices.put_template:
name: test_1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you override the distribution on the command line then it is important to override it again. If you don't override it on the command line then setting this environment variable does nothing which is fine.

return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> features = new ArrayList<>();
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
if (token == XContentParser.Token.FIELD_NAME) {
Expand All @@ -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(),
Expand All @@ -87,35 +90,46 @@ 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();

private final Version lowerVersion;
private final Version upperVersion;
private final List<String> 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<String> features, String reason) {
public SkipSection(String versionRange, List<String> 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;
}

Expand All @@ -131,6 +145,14 @@ public List<String> 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;
}
Expand All @@ -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() {
Expand Down Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<String> 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 {
Expand Down Expand Up @@ -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());
Expand All @@ -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"));
Expand Down Expand Up @@ -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"));
}
}
3 changes: 0 additions & 3 deletions x-pack/qa/core-rest-tests-with-security/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are skipped by the skip section in the test now which I find much easier to read.

'cat.templates/10_basic/Sort templates',
'cat.templates/10_basic/Multiple template',
].join(',')
}

Expand Down