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

alpar-t · 2018-07-20T11:33:17Z

The initial goal was to write a test to make sure that build-tools can
be used by plugin authors outside of our build. Example projects seemed
like a good fit rather than writing dummy ones just for the test.

Add an integration test based on testKit to build the example plugins
like stand alone projects
the tests injects the snapshots repo right now
- an alternative would be to inject the last release version to cause
  dependency resolution against that. It wouldn't be 100% realistic
  as we would build using old dependencies and current build-tools
  which never happens in practice, but it would result in fewer
  moving parts.
- the snapshots does cover more ground at the cost of some confusion
  as these are delayed from the current build, so any new issues will
  introduced will surface in the next build only.
there are also some changes for making it easier to copy these
examples, but it won't quite work yet, e.x. applying the plugin
only works because of testKit. Applying the project like this won't work.
It might work if we add a version number and publish it to the
Gradle plugin portal.
testKit can have the gradle version configured, so it would be easy to
add similar tests for different Gradle versions.
The tests are failing due to 2 reasons:
- elasticsearch-nio is not published. I am looking to fix this.
- painless-whitelist references project(':modules:lang-painless')
  so this isn't really an example that will work stand-alone.
  Not sure what we should do about this one? Not test it or publish
  the plugin jar ?

@nik9000 @rjernst @jasontedor Do you think this something we should pursue ? What should we do about painless-whitelist ? Will the delayed failures due to the snapshots repo be too problematic ?

The initial goal was to write a test to make sure that build-tools can be used by plugin authors outside of our build. Example projects seemed like a good fit rather than writing dummy ones just for the test. - Add an integration test based on testKit to build the example plugins like stand alone projects - the tests injects the snapshots repo right now - an alternative would be to inject the last release version to cause dependency resolution against that. It wouldn't be 100% realistic as we would build using old dependencies and current build-tools which never happens in practice, but it would result in fewer moving parts. - the snapshots does cover more ground at the cost of some confusion as these are delayed from the current build, so any new issues will introduced will surface in the next build only. - there are also some changes for making it easier to copy these examples, but it won't quite work yet, e.x. applying the plugin only works because of testKit. Applying the project like this won't work. It might work if we add a version number and publish it to the Gradle plugin portal. - testKit can have the gradle version configured, so it would be easy to add similar tests for different Gradle versions. - The tests are failing due to 2 reasons: - elasticsearch-nio is not published. I am looking to fix this. - `painless-whitelist` references `project(':modules:lang-painless')` so this isn't really an example that will work stand-alone. Not sure what we should do about this one? Not test it or publish the plugin jar ?

elasticmachine · 2018-07-20T11:33:18Z

Pinging @elastic/es-core-infra

nik9000

I like the idea of adding something to the build to test the build. I'm worried that using stale artifacts will cause spurious failures when we update the API.

rjernst

Thanks for taking this on! It is a sorely needed part of our testing to be complete. I am apprehensive about this approach, though, for a couple reasons:

We've spent a lot of time moving away from depending on snapshots. If I understand this correctly, the example plugins would now be using a different version of build-tools (the latest published), and that would also depend on us publishing to gradle's plugin portal (which we should do, but needs to be done separately and automated with the rest of the release process).
Currently, using our build plugin requires adding build-tools as a build dep in buildscript, and a specific version is what we recommend, since plugins must be built against a specific version of elasticsearch. The examples here have no version specified, so users copying them would just always pickup the latest we published to gradle's plugin portal. However, I don't think we want to have the version copy/pasted in each file since that would make version bumping much more error prone.
The testing here is lower level than I think we want (test kit) for checking this very high level integration (can a basic project using our build plugin work).

I've thought about this in the past and have 2 other possible ideas.

Have a test which invokes a new gradle invocation, specifying each project independently, but first copying the build file into a temporary location where it can be prefixed with the appropriate buildscript block to include the locally built buildSrc.
Have a test which has a custom settings.gradle (again, invoked by a gradle build task) which uses pluginManagement to point to our local build of build-tools, so that the plugin block resolves completely locally. We would need this in both our normal settings.gradle and the one used by the test.

In either of these cases, I think we should discuss with the @esdocs team about a new top level area for plugin development docs. We should "publish" all of our example plugins (in addition to a lot of other basic how to type stuff for plugins). Part of that publishing can inject the appropriate beginning part of the build file whether it uses buildscript or plugin blocks. This way either case can have the exact version of elasticsearch the docs are built for. Obviously this is a broader task outside the scope of simply ensuring build-tools work at all with standalone plugins, but I wanted to point it out as a desirable future state that we should consider in how this all works.

rjernst · 2018-07-20T18:00:14Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/plugin/PluginPropertiesExtension.groovy

@@ -56,17 +56,38 @@ class PluginPropertiesExtension {

    /** A license file that should be included in the built plugin zip. */
    @Input


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.

good catch, thanks!
The annotation needs to be on the getter, then we can keep this private to make sure it goes trough the setter.

rjernst · 2018-07-20T18:02:27Z

buildSrc/src/test/java/org/elasticsearch/gradle/BuildExamplePluginsIT.java

+        runAndRemove("rescore");
+    }
+
+    public void testRestHandler() throws IOException {


I don't like these being hardcoded. We will definitely forget to add new examples here.

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.
Look at @AfterClass i'm not happy about how it changes that static state of the test class, but since it's a test I think it's ok. Essentially we pop the plugin from the list at the start of each test and assert that each plugin has one.

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.

alpar-t · 2018-07-21T12:16:07Z

@nik9000 @rjernst thank you for your helpful though! I think I might have slightly misled you and need to do some clarification.

If I understand this correctly, the example plugins would now be using a different version of build-tools >(the latest published), and that would also depend on us publishing to gradle's plugin portal (which we >should do, but needs to be done separately and automated with the rest of the release process).

That is not correct. The test does not depend on any repo for build-tools, snapshot or otherwise.
java-gradle-plugin takes care of this, it injects the pluginUnderTest into testKit. Gradle runs as a separate process from the tests, so this Gradle plugin saves the build-tools classapth to a meta file that is red by Gradles plugin loading logic, but only if the new apply plugin syntax is used (that's why I'm changing it in this PR).
So build-tools will always be the currently built one for sure!
The part about the snapshot repository refers to the plugin build itself.
When examples are built inside the build having something like dependencies { testCompile 'org.elasticsearch.test:framework:7.0.0-alpha1-SNAPSHOT' } works, but only because we alias that to project(':test:fraework') so it's not really resolved using an external repo.
When building as a stand-alone project, those aliases are gone, so the quick way to get it working was to append repositories { maven { url 'https://snapshots.elastic.co/maven' } } to the end of the build file
and have these resolve against that.
Trouble will arise, say when we break the pom of the dependency, we'll notice only in build+1, if we add a dependency that is not yet published the test will fail until it is, creating a chicken-egg if publishing requires running the tests.

As I was writing this I just realized:

the plugins don't add a dependency on the test FW explicitly, the plugin does that for them.
That seems like a strong assumption, but I think that's part of a broader discussion of how opinionated our Gradle plugins should be. I think the Gradle approach of base + opinionated as separate plugins works well.
we could leverage the fact that we don't write out project dependencies explicitly. TBH this seemed odd, I did not see it before, but it's super powerful, kudos for coming up with it. We could take these artifacts and collect them to a local dir, then alias the jar files directly. I'm not sure if we can include the poms as well like this, but worst case we'll have to mirror a maven directory structure. This is similar to crating a local repo as release manager would, except that it doesn't rely on information from release manager. The downside is that we would not spot problems like we have with elasticsearch-nio as we would not check against what is actually published, we would just include it in our local repo and everything would work. Makes me think about why the knowledge about what needs to be published lives in release manager, and not individual builds ? Perhaps it would make sense to keep the "how?" in RM and the "what" inside the builds ? Do we want to delay having these tests until we do this extra work, or just go with snapshots for now and see our mileage using that as input to prioritize ?

Currently, using our build plugin requires adding build-tools as a build dep in buildscript, and a specific >version is what we recommend, since plugins must be built against a specific version of elasticsearch. >The examples here have no version specified, so users copying them would just always pickup the l>atest we published to gradle's plugin portal. However, I don't think we want to have the version >copy/pasted in each file since that would make version bumping much more error prone.

My understanding is that a copy paste would result in an error since the version is not specified.
This is not different from before, when users still had to add the build-script.
Right now they would have to revert to using the old syntax and add the build script, but if we publish the plugin to Gradle, then they would only have to add the version.

The new syntax can load plugins from:

the classpath, but only for the org.gradle namespace, so these will be core plugins, like java, so one can write plugins { id 'java' }
the plugin portal, if the plugin is published and a version is specified. I never tested it but I would expect the version to work as for dependencies, so you would have to write something like 6+ to get dynamic versions.
plugins under test passed by testKit, this is what I was mentioning earlier and this is what makes it all work.
In other words, building the example plugins as they are written only works because we run them with testKit and build them with java-gradle-plugin. It would actually be tricky to have a version in the examples, maybe something like version: elasticsearchVersion and than just make elasticsearchVersion a null - or similar way of suggesting that users need to add aversion when copying this code, that if we want to have something in the code.

The java-gradle-plugin + testKit magic in Gradle is a bit much for sure, but not that bad after a bit of getting used to. At least parts of it are still incubating.

The testing here is lower level than I think we want (test kit) for checking this very high level integration >(can a basic project using our build plugin work).

I'm not sure what you mean here. AFAIK testKit uses the Gradle Tooling API, the same used by Idea and build ship to get Gradle configurations and run builds.
It runs Gradle in a separate in a separate project pretty much how a user would, except for the variations I mentioned above earlier to get the plugin under test injected. It also covers managing wrappers, so we could easily implement tests on different Gradle versions. I feel like doing this kind of setup would make us jump trough the same hoops as solved by testKit, and to me it sounds very similar to what you are describing in your proposals, maybe just implemented in a slightly different way. AFAIK the gradlebuild task uses the tooling api as well.

The only problem I have with testKit is that it passes in class and resource directories of the plugin under tast, rather than the Jar. I imagine this was done since it would be awkward in Gradle terms to start having a dependency between test and jar, but we can always add that dependency and patch the generated meta-info file, I tested that and it works, but wanted to limit the size of this PR. It's not that important, as relying on something like rootProject.file("buildSrc/...") would not work anyhow, but it wouldn't catch code that calls getResource() and expects to be able to translate the URL to a File.
Maybe there's some other way to prevent that using forbidden apis ?

++ on finding a home for docs and documenting it.
I'm not sure if we should start that right away. We may want to make the plugins less opinionated first, maybe carve out just a subset of build-tools that is meant for external consumption and only publish and document that part.

alpar-t · 2018-07-21T12:17:44Z

I would like feedback on this as well

painless-whitelist references project(':modules:lang-painless')

How would a user go about copying this ? Given that AFAIK project(':modules:lang-painless') is not published ?

rjernst · 2018-07-24T20:43:22Z

How would a user go about copying this ? Given that AFAIK project(':modules:lang-painless') is not published ?

This is leftover from a first iteration of painless whitelists and can actually be updated to use the spi jar that we do publish for painless.

alpar-t · 2018-08-07T07:08:39Z

elastic/release-manager#401 fixes elasticsearch-nio, and I replaced the project reference with spi.
While doing that I discovered to other issues:

luceene snapshot repo is needed because we now use a snapshot luceene release. We can add this from the tests.
the tests failed with a version conflict for log4j because the version was bumped on master and snapshots were generated with it. I think this is is a deal breaker as things like this will cause too much noise on branches and will just waste time, so we'll need to take these tests off the snapshot repo

…-build

Will publish everything that has publishing configured to a local repo

alpar-t · 2018-08-07T13:13:22Z

@nik9000 @rjernst This is ready for a final review, in a less controversial form.

I solved the problem with the dependencies by configuring a maven repository backed by a local directory for every publication we have, and then pass that to the tests to use as a repo.
This simple approach eluded me so far. It works well and avoid the drawbacks of using the snapshot repo.

The tests now essentially simulate a repo of what was just built and use it to build the plugins stand alone. Other changes in the PR are to make that pass.

The problem of configuring a publishing in the build that will not actually be published in release manager, e.x. the problem we had with elasticsearch-nio will not be hit by these tests, but we can deal with it separately as it has broader implications than just the plugin builds.

Ideally we should only have publications for what we actually publish, and make sure we actually publish everything we have a publication for. I think the RM could check this.

alpar-t · 2018-08-08T20:20:43Z

buildSrc/src/test/java/org/elasticsearch/gradle/BuildExamplePluginsIT.java

+    public TemporaryFolder tmpDir = new TemporaryFolder();
+
+    @BeforeClass
+    public static void assertProjectsExist() {


Redo this into a loop.

rjernst · 2018-08-08T22:20:55Z

This generally looks good, but I think it is important to not require new example plugins remember to add a test. Instead, the test should iterate over all example plugins.

alpar-t · 2018-08-10T08:29:57Z

I realized that this is similar to how we run yaml tests, so I found the randomized testing fw has @ParametersFactory this gives the best of both words.

alpar-t · 2018-08-10T08:37:20Z

@nik9000 @rjernst ready for the final review

rjernst

Thanks for the update, I have few more comments

rjernst · 2018-08-10T17:24:08Z

build.gradle

+      repositories {
+        maven {
+          name = 'localTest'
+          url = "$rootProject.buildDir/local-test-repo"


Please use ${rootProject.buildDir} as it is more clear where the substitution ends.

This still seems outstanding?

rjernst · 2018-08-10T17:25:18Z

build.gradle

+  // Make nebula and shadow play nice together, Gradle doesn't like it how one jar task replaces the output of another
+  // if both are part of published artifacts
+  project.afterEvaluate {
+    if (plugins.hasPlugin(MavenPublishPlugin) && plugins.hasPlugin(ShadowPlugin)) {


Is this something that should be in BuildPlugin?

I don't have a strong preference. I'm thinking build plugin will be implemented in Java and perhaps the bulk of it split into smaller focused classes. In this respect small scripts and glue could live and would be easier to write in the groovy script.

It seems wrong to me to have the shadow plugin and publishing be something we support via BuildPlugin, yet require additional hacks within our build files.

rjernst · 2018-08-10T17:30:31Z

buildSrc/build.gradle

+    systemProperty (
+            'test.build-tools.plugin.examples',
+            files(
+                    project(':example-plugins').subprojects.collect { it.projectDir }


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?

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.

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).

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.

rjernst · 2018-08-10T17:31:51Z

buildSrc/src/test/java/org/elasticsearch/gradle/BuildExamplePluginsIT.java

+                "  maven {\n" +
+                "    url \"" + getLocalTestRepoPath()  + "\"\n" +
+                "  }\n" +
+                "  maven {\n" +


Can we conditionally add this only if the build is using a lucene snapshot?

rjernst · 2018-08-10T17:32:52Z

buildSrc/src/test/java/org/elasticsearch/gradle/BuildExamplePluginsIT.java

+    }
+
+    private String getLuceneSnapshotRevision() {
+        return System.getProperty("test.luceene-snapshot-revision", "not-a-snapshot");


typo: luceene -> lucene

rjernst · 2018-08-10T17:34:52Z

plugins/examples/custom-settings/build.gradle

@@ -17,15 +17,21 @@
 * under the License.
 */

-apply plugin: 'elasticsearch.esplugin'
+plugins {
+  id 'elasticsearch.esplugin'


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.

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.

rjernst · 2018-08-10T17:35:08Z

plugins/examples/custom-settings/build.gradle

+}
+


nit: extra newlines

alpar-t · 2018-08-15T13:50:37Z

@rjernst now using the old syntax to apply plugin.

rjernst

Thanks @atorok. Your proposal to address my concerns in a followup is fine with me.

Add tests for build-tools to make sure example plugins build stand-alone using it. This will catch issues such as referencing files from the buildSrc directly, breaking external uses of build-tools.

* 6.x: (42 commits) [DOCS] Splits the users API documentation into multiple pages (#32825) [DOCS] Splits the token APIs into separate pages (#32865) [DOCS] Creates redirects for role management APIs page Bypassing failing test PainlessDomainSplitIT#testHRDSplit (#32966) TEST: Mute testRetentionPolicyChangeDuringRecovery [DOCS] Fixes more broken links to role management APIs [Docs] Tweaks and fixes to rollup docs [DOCS] Fixes links to role management APIs [ML][TEST] Fix BasicRenormalizationIT after adding multibucket feature [DOCS] Splits the roles API documentation into multiple pages (#32794) [TEST] Run pre 6.4 nodes in non-FIPS JVMs (#32901) Remove assertion in testDocStats on deletedDocs counter (#32914) [ML] fix updating opened jobs scheduled events (#31651) (#32881) Enable FIPS140LicenseBootstrapCheck (#32903) HLRC: Move ML request converters into their own class (#32906) [DOCS] Update getting-started.asciidoc (#29518) Fix allowed value for HighlighterBuilder encoder in javadocs (#32780) [DOCS] Add "remove a tag" script logic as an example (#32556) RFC: Test that example plugins build stand-alone (#32235) Security: remove put privilege API (#32879) ...

alpar-t added >test Issues or PRs that are addressing/adding tests :Delivery/Build Build or test infrastructure labels Jul 20, 2018

nik9000 reviewed Jul 20, 2018

View reviewed changes

rjernst reviewed Jul 20, 2018

View reviewed changes

Fix task up-to-date annotations

fa25aee

Add lang-painless-spi, luceene snapshot repo

b0213f4

alpar-t added 3 commits August 7, 2018 10:23

Merge remote-tracking branch 'origin/master' into test-example-plugin…

bca30ef

…-build

Create a publishNebulaPublicationToLocalTestRepository task

1356199

Will publish everything that has publishing configured to a local repo

Connect tests to the local repo

db988ef

alpar-t commented Aug 8, 2018

View reviewed changes

pr review

34d73bc

rjernst reviewed Aug 10, 2018

View reviewed changes

useold syntax to apply plugin

cdb057b

PR review

4ef6a43

rjernst approved these changes Aug 16, 2018

View reviewed changes

alpar-t added v7.0.0 v6.5.0 labels Aug 17, 2018

alpar-t merged commit d16562e into elastic:master Aug 17, 2018

alpar-t deleted the test-example-plugin-build branch August 17, 2018 06:41

alpar-t mentioned this pull request Nov 26, 2018

Plugins: External 6.3.0 plugin fail if idea plugin is not loaded in build.gradle #31329

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

alpar-t commented Jul 20, 2018

elasticmachine commented Jul 20, 2018

nik9000 left a comment

rjernst left a comment •

edited

Loading

rjernst Jul 20, 2018

alpar-t Jul 21, 2018

rjernst Jul 20, 2018

alpar-t Jul 21, 2018

alpar-t commented Jul 21, 2018 •

edited

Loading

alpar-t commented Jul 21, 2018

rjernst commented Jul 24, 2018

alpar-t commented Aug 7, 2018

alpar-t commented Aug 7, 2018

alpar-t Aug 8, 2018

rjernst commented Aug 8, 2018

alpar-t commented Aug 10, 2018

alpar-t commented Aug 10, 2018

rjernst left a comment

rjernst Aug 10, 2018

rjernst Aug 15, 2018

rjernst Aug 10, 2018

alpar-t Aug 10, 2018 •

edited

Loading

rjernst Aug 15, 2018

rjernst Aug 10, 2018

alpar-t Aug 10, 2018 •

edited

Loading

rjernst Aug 15, 2018

alpar-t Aug 16, 2018

rjernst Aug 10, 2018

rjernst Aug 10, 2018

rjernst Aug 10, 2018

alpar-t Aug 10, 2018

rjernst Aug 10, 2018

alpar-t commented Aug 15, 2018

rjernst left a comment

		@@ -56,17 +56,38 @@ class PluginPropertiesExtension {

		/** A license file that should be included in the built plugin zip. */
		@Input

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

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

Conversation

alpar-t commented Jul 20, 2018

elasticmachine commented Jul 20, 2018

nik9000 left a comment

Choose a reason for hiding this comment

rjernst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented Jul 21, 2018 • edited Loading

alpar-t commented Jul 21, 2018

rjernst commented Jul 24, 2018

alpar-t commented Aug 7, 2018

alpar-t commented Aug 7, 2018

Choose a reason for hiding this comment

rjernst commented Aug 8, 2018

alpar-t commented Aug 10, 2018

alpar-t commented Aug 10, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alpar-t commented Aug 15, 2018

rjernst left a comment

Choose a reason for hiding this comment

rjernst left a comment •

edited

Loading

alpar-t commented Jul 21, 2018 •

edited

Loading

alpar-t Aug 10, 2018 •

edited

Loading

alpar-t Aug 10, 2018 •

edited

Loading