-
Notifications
You must be signed in to change notification settings - Fork 25.1k
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
Changes from 1 commit
5cb970a
fa25aee
b0213f4
bca30ef
1356199
db988ef
34d73bc
cdb057b
4ef6a43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,17 +56,38 @@ class PluginPropertiesExtension { | |
|
||
/** A license file that should be included in the built plugin zip. */ | ||
@Input | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this can be @input if it is private? It needs the default visibility to allow groovy/gradle to wrap in such a way that changes can be tracked. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch, thanks! |
||
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 | ||
} | ||
|
||
File getLicenseFile() { | ||
return licenseFile | ||
} | ||
|
||
void setLicenseFile(File licenseFile) { | ||
project.ext.licenseFile = licenseFile | ||
this.licenseFile = licenseFile | ||
} | ||
|
||
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 |
---|---|---|
@@ -0,0 +1,124 @@ | ||
/* | ||
* 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 org.apache.commons.io.FileUtils; | ||
import org.elasticsearch.gradle.test.GradleIntegrationTestCase; | ||
import org.gradle.testkit.runner.GradleRunner; | ||
import org.junit.AfterClass; | ||
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.StandardOpenOption; | ||
import java.util.Arrays; | ||
import java.util.List; | ||
import java.util.Objects; | ||
import java.util.stream.Collectors; | ||
|
||
public class BuildExamplePluginsIT extends GradleIntegrationTestCase { | ||
|
||
private static List<File> EXTERNAL_PROJECTS = 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(); | ||
|
||
@BeforeClass | ||
public static void assertProjectsExist() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Redo this into a loop. |
||
List<File> existing = EXTERNAL_PROJECTS.stream().filter(File::exists).collect(Collectors.toList()); | ||
assertEquals(EXTERNAL_PROJECTS, existing); | ||
} | ||
|
||
@AfterClass | ||
public static void assertEverythingTested() { | ||
assertEquals( | ||
"Some example plugins are not tested: " + EXTERNAL_PROJECTS, | ||
0, EXTERNAL_PROJECTS.size() | ||
); | ||
} | ||
|
||
public void testCustomSettings() throws IOException { | ||
runAndRemove("custom-settings"); | ||
} | ||
|
||
public void testPainlessWhitelist() throws IOException { | ||
runAndRemove("painless-whitelist"); | ||
} | ||
|
||
public void testRescore() throws IOException { | ||
runAndRemove("rescore"); | ||
} | ||
|
||
public void testRestHandler() throws IOException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like these being hardcoded. We will definitely forget to add new examples here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason they are hard coded is that we still run all even if one fails, without rewriting this logic in unit + muting tests is easier too. We will not forget to add new ones as in this case the test will fail. I initially had a loop and changed it to this as soon as I realized we might wan to take decisions based on individual plugins. |
||
runAndRemove("rest-handler"); | ||
} | ||
|
||
public void testScriptExpertScoring() throws IOException { | ||
runAndRemove("script-expert-scoring"); | ||
} | ||
|
||
private void runAndRemove(String name) throws IOException { | ||
List<File> matches = EXTERNAL_PROJECTS.stream().filter(each -> each.getAbsolutePath().contains(name)) | ||
.collect(Collectors.toList()); | ||
assertEquals( | ||
"Expected a single project folder to match `" + name + "` but got: " + matches, | ||
1, matches.size() | ||
); | ||
|
||
EXTERNAL_PROJECTS.remove(matches.get(0)); | ||
|
||
FileUtils.copyDirectory(matches.get(0), tmpDir.getRoot()); | ||
// just get rid of deprecation warnings from now | ||
Files.write( | ||
tmpDir.newFile("settings.gradle").toPath(), "enableFeaturePreview('STABLE_PUBLISHING')\n".getBytes(StandardCharsets.UTF_8) | ||
); | ||
// Add a repositories section to be able to resolve from snapshots. | ||
// !NOTE! that the plugin build will use be using stale artifacts, not the ones produced in the current build | ||
Files.write( | ||
new File(tmpDir.getRoot(), "build.gradle").toPath(), | ||
("\n" + | ||
"repositories {\n" + | ||
" maven {\n" + | ||
" url \"https://snapshots.elastic.co/maven\"\n" + | ||
" mavenLocal()\n" + | ||
" }\n" + | ||
"}\n").getBytes(StandardCharsets.UTF_8), | ||
StandardOpenOption.APPEND | ||
); | ||
Files.write( | ||
tmpDir.newFile("NOTICE.txt").toPath(), | ||
"dummy test nottice".getBytes(StandardCharsets.UTF_8) | ||
); | ||
|
||
GradleRunner.create() | ||
// Todo make a copy, write a settings file enabling stable publishing | ||
.withProjectDir(tmpDir.getRoot()) | ||
.withArguments("clean", "check", "-s", "-i", "--warning-mode=all") | ||
.withPluginClasspath() | ||
.build(); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,15 +17,21 @@ | |
* under the License. | ||
*/ | ||
|
||
apply plugin: 'elasticsearch.esplugin' | ||
plugins { | ||
id 'elasticsearch.esplugin' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still don't like having this using the plugins dsl until we actually have build-tools published to the gradle plugin portal. While these examples currently are missing the buildtools section to make apply work outside the build, if they are copied with the plugins block they will never work, and the purpose of these is to make the examples copyable. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not entirely happy about it either, but it handles the injection of the plugin that was just built, a lot of boilerplate that this DSL and testKit helps us avoid. That being said, I think with what we have now, we could inject the local repo into the build script classpath and easily avoid that boilerplate. I will take a look. |
||
} | ||
|
||
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 { | ||
// Adds a setting in the Elasticsearch keystore before running the integration tests | ||
keystoreSetting 'custom.secured', 'password' | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: extra newlines |
||
|
There was a problem hiding this comment.
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 runcheck
under there and be confident the examples are setup correctly?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.