Skip to content

Add support for switching distribution for all integration tests #30874

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Jun 26, 2018
Merged

Add support for switching distribution for all integration tests #30874

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
ed0f95c
remove left-over comment
alpar-t May 25, 2018
677cce5
make sure of the property for plugins
alpar-t May 25, 2018
73fa278
skip installing modules if these exist in the distribution
alpar-t May 28, 2018
3364a43
Log the distrbution being ran
alpar-t May 28, 2018
9e457bd
Don't allow running with integ-tests-zip passed externally
alpar-t May 28, 2018
fef4e22
top level x-pack/qa can't run with oss distro
alpar-t May 28, 2018
e9579aa
Merge branch 'master' into test/distributions
alpar-t May 28, 2018
c17a46d
Add support for matching objects in lists
alpar-t May 29, 2018
ae9d375
Update plugin and module tests to work with other distributions
alpar-t May 29, 2018
2b4d2ca
Allow modules to run on other distros as well
alpar-t May 30, 2018
8631413
Add and use a new `contains` assertion
alpar-t May 31, 2018
7ba9499
Merge commit '8e4ab82' into test/distributions
alpar-t May 31, 2018
e0fa121
Merge commit 'fcf1e41' into test/distributions
alpar-t Jun 15, 2018
cc454ec
Merge remote-tracking branch 'origin/master' into test/distributions
alpar-t Jun 15, 2018
b9faa69
Merge commit '86423f9' into test/distributions
alpar-t Jun 22, 2018
80ffc38
Implement PR review comments
alpar-t Jun 22, 2018
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
2 changes: 1 addition & 1 deletion buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginBuildPlugin.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ public class PluginBuildPlugin extends BuildPlugin {
private static void createIntegTestTask(Project project) {
RestIntegTestTask integTest = project.tasks.create('integTest', RestIntegTestTask.class)
integTest.mustRunAfter(project.precommit, project.test)
project.integTestCluster.distribution = 'integ-test-zip'
project.integTestCluster.distribution = System.getProperty('tests.distribution', 'integ-test-zip')
project.check.dependsOn(integTest)
}

Expand Down
3 changes: 0 additions & 3 deletions buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,9 +532,6 @@ class ClusterFormationTasks {
}

static Task configureInstallModuleTask(String name, Project project, Task setup, NodeInfo node, Project module) {
if (node.config.distribution != 'integ-test-zip') {
Copy link
Member

Choose a reason for hiding this comment

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

If using a non integ-test-zip distribution, modules need to not be installed at all. This looks like it allows installing them over what the distribution installs, which could create craziness. I think we need to skip setting up install module tasks if the distribution is not integ-test-zip.

throw new GradleException("Module ${module.path} not allowed be installed distributions other than integ-test-zip because they should already have all modules bundled!")
}
if (module.plugins.hasPlugin(PluginBuildPlugin) == false) {
throw new GradleException("Task ${name} cannot include module ${module.path} which is not an esplugin")
}
Expand Down
2 changes: 1 addition & 1 deletion plugins/discovery-file/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ task setupSeedNodeAndUnicastHostsFile(type: DefaultTask) {
// setup the initial cluster with one node that will serve as the seed node
// for unicast discovery
ClusterConfiguration config = new ClusterConfiguration(project)
config.distribution = 'integ-test-zip'
config.distribution = System.getProperty('tests.distribution', 'integ-test-zip')
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need this line at all given your change to PluginBuildPlugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would take precedence and override that. So it would be set back to integ-test-zip regardless of the property.

Copy link
Member

Choose a reason for hiding this comment

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

I am concerned this is fragile. I don't want to see this logic copied around to other projects. Can you please create an issue for making creating additional cluster fixtures easier, so that the default can be set by PluginBuildPlugin for this project and reused the we create this cluster?

Copy link
Member

Choose a reason for hiding this comment

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

++ on this being fragile. Fixing it in a followup is fine with me though.

config.clusterName = 'discovery-file-test-cluster'
List<NodeInfo> nodes = ClusterFormationTasks.setup(project, 'initialCluster', setupSeedNodeAndUnicastHostsFile, config)
File srcUnicastHostsFile = file('build/cluster/unicast_hosts.txt')
Expand Down
8 changes: 6 additions & 2 deletions plugins/examples/painless-whitelist/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@ dependencies {
compileOnly project(':modules:lang-painless')
}

integTestCluster {
distribution = 'zip'
if (System.getProperty('tests.distribution') == null) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems a bit complicated to add to every project. I'm not 100% sure we need to support -Dtests.distribution=integ-test-zip. Maybe integ-test-zip is a thing we should only use when you don't override the property.

Copy link
Member

Choose a reason for hiding this comment

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

It'd be fairly simple to fail the build if someone sets distribution to integ-test-zip. If we did that then this project could do distribution = System.getProperty('tests.distribution', 'zip).

Copy link
Member

Choose a reason for hiding this comment

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

I might be cleaner to move the PluginBuildPlugin change to a gradle.projectsEvaluated and have it handle the logic all in one place.

Copy link
Contributor Author

@alpar-t alpar-t May 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

setting distro integ-test-zip would offer a way to run some integration tests in a build that would be much quicker as it would skip a lot of the heavier integration tests (probably most of QA). I'm not sure how useful that would be during development, so can't decide if it's worth doing.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that is generally useful. Plugins and modules are the projects which use integ-test-zip, and you can already cd into the plugins or modules dir and run tests from there.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I don't think we should support setting -Dtests.distribution=integ-test-zip. It should be what you get when you run without setting it.

integTestCluster.distribution = 'oss-zip'
}

if (integTestCluster.distribution == 'integ-test-zip') {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this line because we don't support setting the setting to this.

Copy link
Member

Choose a reason for hiding this comment

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

We can add something to cluster formation tasks to make sure we don't set the system property to integ-test-zip.

integTest.enabled = false
}

test.enabled = false
1 change: 0 additions & 1 deletion plugins/repository-hdfs/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,6 @@ for (String fixtureName : ['hdfsFixture', 'haHdfsFixture', 'secureHdfsFixture',
project.afterEvaluate {
for (String integTestTaskName : ['integTestHa', 'integTestSecure', 'integTestSecureHa']) {
ClusterConfiguration cluster = project.extensions.getByName("${integTestTaskName}Cluster") as ClusterConfiguration
cluster.distribution = 'integ-test-zip'
cluster.dependsOn(project.bundlePlugin)

Task restIntegTestTask = project.tasks.getByName(integTestTaskName)
Expand Down
8 changes: 1 addition & 7 deletions qa/smoke-test-rank-eval-with-mustache/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,4 @@ apply plugin: 'elasticsearch.rest-test'
dependencies {
testCompile project(path: ':modules:rank-eval', configuration: 'runtime')
testCompile project(path: ':modules:lang-mustache', configuration: 'runtime')
}

/*
Copy link
Member

Choose a reason for hiding this comment

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

++. This is closed. If you want to just push a commit that removes that comment to the repo without waiting on a PR that'd be awesome.

* 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
*/
}