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

RFC: Test that example plugins build stand-alone #32235

Merged
merged 9 commits into from
Aug 17, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
8 changes: 8 additions & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,15 @@ subprojects {
}
}
}
repositories {
maven {
name = 'localTest'
url = "${rootProject.buildDir}/local-test-repo"
}
}
}
}

plugins.withType(BuildPlugin).whenPluginAdded {
project.licenseFile = project.rootProject.file('licenses/APACHE-LICENSE-2.0.txt')
project.noticeFile = project.rootProject.file('NOTICE.txt')
Expand Down Expand Up @@ -228,6 +235,7 @@ subprojects {
"org.elasticsearch.client:elasticsearch-rest-high-level-client:${version}": ':client:rest-high-level',
"org.elasticsearch.client:test:${version}": ':client:test',
"org.elasticsearch.client:transport:${version}": ':client:transport',
"org.elasticsearch.plugin:elasticsearch-scripting-painless-spi:${version}": ':modules:lang-painless:spi',
"org.elasticsearch.test:framework:${version}": ':test:framework',
"org.elasticsearch.distribution.integ-test-zip:elasticsearch:${version}": ':distribution:archives:integ-test-zip',
"org.elasticsearch.distribution.zip:elasticsearch:${version}": ':distribution:archives:zip',
Expand Down
13 changes: 13 additions & 0 deletions buildSrc/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,24 @@ if (project != rootProject) {
// it's fine as we run them as part of :buildSrc
test.enabled = false
task integTest(type: Test) {
// integration test requires the local testing repo for example plugin builds
dependsOn project.rootProject.allprojects.collect {
it.tasks.matching { it.name == 'publishNebulaPublicationToLocalTestRepository'}
}
exclude "**/*Tests.class"
include "**/*IT.class"
testClassesDirs = sourceSets.test.output.classesDirs
classpath = sourceSets.test.runtimeClasspath
inputs.dir(file("src/testKit"))
// tell BuildExamplePluginsIT where to find the example plugins
systemProperty (
'test.build-tools.plugin.examples',
files(
project(':example-plugins').subprojects.collect { it.projectDir }
Copy link
Member

Choose a reason for hiding this comment

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

I wonder I this test would make more sense inside plugins/examples, so one could just run check under there and be confident the examples are setup correctly?

Copy link
Contributor Author

@alpar-t alpar-t Aug 10, 2018

Choose a reason for hiding this comment

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

I don't think one is better than the other. It's an integration between the too, and right now chances are greater that build-tools will break the test as the plugins barely change. I taught about it too because it's odd to a new example ad have a distant projects tests break. We could always move them around or create a project in qa - which I think would be in-line with how we handle this in general.

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 see this as an integration between the two. The examples are on their own. They use the build-tools, like any other project within the repo. This test is about ensuring those projects build files are correct. Ensuring build-tools is not itself broken is a place other tests here, IMO. Actually, I think it would be even nicer if each example plugin had an additional integration test task in place to check that specific plugin. This would make reproducing failures a lot easier (since it is a task on the project that fails).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with a task on the example plugins, but not entirely convinced it's worth the effort now . I started writing these tests to make sure build-tools can run for other builds too, something that the example plugins helped with. We build these during the regular build so making sure build script and all is correct is already done there to some extent. I propose we do this in a subsequent PR after seeing where this approach takes us, so we can get some tests running and feedback before we invest more effort into it.

).asPath,
)
systemProperty 'test.local-test-repo-path', "${rootProject.buildDir}/local-test-repo"
systemProperty 'test.lucene-snapshot-revision', (versions.lucene =~ /\w+-snapshot-([a-z0-9]+)/)[0][1]
}
check.dependsOn(integTest)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -523,7 +523,7 @@ class BuildPlugin implements Plugin<Project> {
project.publishing {
publications {
nebula(MavenPublication) {
artifact project.tasks.shadowJar
artifacts = [ project.tasks.shadowJar ]
artifactId = project.archivesBaseName
/*
* Configure the pom to include the "shadow" as compile dependencies
Expand Down Expand Up @@ -553,7 +553,6 @@ class BuildPlugin implements Plugin<Project> {
}
}
}

}

/** Adds compiler settings to the project */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import org.elasticsearch.gradle.NoticeTask
import org.elasticsearch.gradle.test.RestIntegTestTask
import org.elasticsearch.gradle.test.RunTask
import org.gradle.api.InvalidUserDataException
import org.gradle.api.JavaVersion
import org.gradle.api.Project
import org.gradle.api.Task
import org.gradle.api.XmlProvider
Expand All @@ -39,7 +38,6 @@ import java.nio.file.Path
import java.nio.file.StandardCopyOption
import java.util.regex.Matcher
import java.util.regex.Pattern

/**
* Encapsulates build configuration for an Elasticsearch plugin.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package org.elasticsearch.gradle.plugin

import org.gradle.api.Project
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.InputFile

/**
* A container for plugin properties that will be written to the plugin descriptor, for easy
Expand Down Expand Up @@ -55,18 +56,39 @@ class PluginPropertiesExtension {
boolean requiresKeystore = false

/** A license file that should be included in the built plugin zip. */
@Input
File licenseFile = null
private File licenseFile = null

/**
* A notice file that should be included in the built plugin zip. This will be
* extended with notices from the {@code licenses/} directory.
*/
@Input
File noticeFile = null
private File noticeFile = null

Project project = null

PluginPropertiesExtension(Project project) {
name = project.name
version = project.version
this.project = project
}

@InputFile
File getLicenseFile() {
return licenseFile
}

void setLicenseFile(File licenseFile) {
project.ext.licenseFile = licenseFile
this.licenseFile = licenseFile
}

@InputFile
File getNoticeFile() {
return noticeFile
}

void setNoticeFile(File noticeFile) {
project.ext.noticeFile = noticeFile
this.noticeFile = noticeFile
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import org.gradle.api.InvalidUserDataException
import org.gradle.api.Task
import org.gradle.api.tasks.Copy
import org.gradle.api.tasks.OutputFile

/**
* Creates a plugin descriptor.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import org.gradle.api.provider.Provider
import org.gradle.api.tasks.Copy
import org.gradle.api.tasks.Input
import org.gradle.api.tasks.TaskState
import org.gradle.plugins.ide.idea.IdeaPlugin

import java.nio.charset.StandardCharsets
import java.nio.file.Files
Expand Down Expand Up @@ -243,10 +244,12 @@ public class RestIntegTestTask extends DefaultTask {
}
}
}
project.idea {
module {
if (scopes.TEST != null) {
scopes.TEST.plus.add(project.configurations.restSpec)
if (project.plugins.hasPlugin(IdeaPlugin)) {
project.idea {
module {
if (scopes.TEST != null) {
scopes.TEST.plus.add(project.configurations.restSpec)
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
/*
* 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.gradle;

import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
import org.apache.commons.io.FileUtils;
import org.elasticsearch.gradle.test.GradleIntegrationTestCase;
import org.gradle.testkit.runner.GradleRunner;
import org.junit.BeforeClass;
import org.junit.Rule;
import org.junit.rules.TemporaryFolder;

import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;

public class BuildExamplePluginsIT extends GradleIntegrationTestCase {

private static List<File> EXAMPLE_PLUGINS = Collections.unmodifiableList(
Arrays.stream(
Objects.requireNonNull(System.getProperty("test.build-tools.plugin.examples"))
.split(File.pathSeparator)
).map(File::new).collect(Collectors.toList())
);

@Rule
public TemporaryFolder tmpDir = new TemporaryFolder();

public final File examplePlugin;

public BuildExamplePluginsIT(File examplePlugin) {
this.examplePlugin = examplePlugin;
}

@BeforeClass
public static void assertProjectsExist() {
assertEquals(
EXAMPLE_PLUGINS,
EXAMPLE_PLUGINS.stream().filter(File::exists).collect(Collectors.toList())
);
}

@ParametersFactory
public static Iterable<Object[]> parameters() {
return EXAMPLE_PLUGINS
.stream()
.map(each -> new Object[] {each})
.collect(Collectors.toList());
}

public void testCurrentExamplePlugin() throws IOException {
FileUtils.copyDirectory(examplePlugin, tmpDir.getRoot());
// just get rid of deprecation warnings
Files.write(
getTempPath("settings.gradle"),
"enableFeaturePreview('STABLE_PUBLISHING')\n".getBytes(StandardCharsets.UTF_8)
);

adaptBuildScriptForTest();

Files.write(
tmpDir.newFile("NOTICE.txt").toPath(),
"dummy test notice".getBytes(StandardCharsets.UTF_8)
);

GradleRunner.create()
.withProjectDir(tmpDir.getRoot())
.withArguments("clean", "check", "-s", "-i", "--warning-mode=all", "--scan")
.withPluginClasspath()
.build();
}

private void adaptBuildScriptForTest() throws IOException {
// Add the local repo as a build script URL so we can pull in build-tools and apply the plugin under test
// + is ok because we have no other repo and just want to pick up latest
writeBuildScript(
"buildscript {\n" +
" repositories {\n" +
" maven {\n" +
" url = '" + getLocalTestRepoPath() + "'\n" +
" }\n" +
" }\n" +
" dependencies {\n" +
" classpath \"org.elasticsearch.gradle:build-tools:+\"\n" +
" }\n" +
"}\n"
);
// get the original file
Files.readAllLines(getTempPath("build.gradle"), StandardCharsets.UTF_8)
.stream()
.map(line -> line + "\n")
.forEach(this::writeBuildScript);
// Add a repositories section to be able to resolve dependencies
String luceneSnapshotRepo = "";
String luceneSnapshotRevision = System.getProperty("test.lucene-snapshot-revision");
if (luceneSnapshotRepo != null) {
luceneSnapshotRepo = " maven {\n" +
" url \"http://s3.amazonaws.com/download.elasticsearch.org/lucenesnapshots/" + luceneSnapshotRevision + "\"\n" +
" }\n";
}
writeBuildScript("\n" +
"repositories {\n" +
" maven {\n" +
" url \"" + getLocalTestRepoPath() + "\"\n" +
" }\n" +
luceneSnapshotRepo +
"}\n"
);
Files.delete(getTempPath("build.gradle"));
Files.move(getTempPath("build.gradle.new"), getTempPath("build.gradle"));
System.err.print("Generated build script is:");
Files.readAllLines(getTempPath("build.gradle")).forEach(System.err::println);
}

private Path getTempPath(String fileName) {
return new File(tmpDir.getRoot(), fileName).toPath();
}

private Path writeBuildScript(String script) {
try {
Path path = getTempPath("build.gradle.new");
return Files.write(
path,
script.getBytes(StandardCharsets.UTF_8),
Files.exists(path) ? StandardOpenOption.APPEND : StandardOpenOption.CREATE_NEW
);
} catch (IOException e) {
throw new RuntimeException(e);
}
}

private String getLocalTestRepoPath() {
String property = System.getProperty("test.local-test-repo-path");
Objects.requireNonNull(property, "test.local-test-repo-path not passed to tests");
File file = new File(property);
assertTrue("Expected " + property + " to exist, but it did not!", file.exists());
return file.getAbsolutePath();
}

}
3 changes: 2 additions & 1 deletion plugins/examples/custom-settings/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/

apply plugin: 'elasticsearch.esplugin'

esplugin {
name 'custom-settings'
description 'An example plugin showing how to register custom settings'
classname 'org.elasticsearch.example.customsettings.ExampleCustomSettingsPlugin'
licenseFile rootProject.file('licenses/APACHE-LICENSE-2.0.txt')
noticeFile rootProject.file('NOTICE.txt')
}

integTestCluster {
Expand Down
5 changes: 3 additions & 2 deletions plugins/examples/painless-whitelist/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/

apply plugin: 'elasticsearch.esplugin'

esplugin {
name 'painless-whitelist'
description 'An example whitelisting additional classes and methods in painless'
classname 'org.elasticsearch.example.painlesswhitelist.MyWhitelistPlugin'
extendedPlugins = ['lang-painless']
licenseFile rootProject.file('licenses/APACHE-LICENSE-2.0.txt')
noticeFile rootProject.file('NOTICE.txt')
}

dependencies {
compileOnly project(':modules:lang-painless')
compileOnly "org.elasticsearch.plugin:elasticsearch-scripting-painless-spi:${versions.elasticsearch}"
}

if (System.getProperty('tests.distribution') == null) {
Expand Down
4 changes: 3 additions & 1 deletion plugins/examples/rescore/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/

apply plugin: 'elasticsearch.esplugin'

esplugin {
name 'example-rescore'
description 'An example plugin implementing rescore and verifying that plugins *can* implement rescore'
classname 'org.elasticsearch.example.rescore.ExampleRescorePlugin'
licenseFile rootProject.file('licenses/APACHE-LICENSE-2.0.txt')
noticeFile rootProject.file('NOTICE.txt')
}

Loading