Replace heavy weight build-tools integ tests with a simple unit test. by alpar-t · Pull Request #44056 · elastic/elasticsearch

alpar-t · 2019-07-08T08:44:42Z

Remove heavy build-tool integ test.
Add a unit test for the plugin builder plugin.

- Moves the example project builds into their own project - switch to doing an assemble only rather than a full check - Remove most of testclusters IT - this is a central piece of test infrastructure now widely use we would for sure catch any fallout in all the other tests. Closes elastic#41256 elastic#41256

elasticmachine · 2019-07-08T08:44:44Z

Pinging @elastic/es-core-infra

alpar-t · 2019-07-08T08:47:46Z

An alternative I taught about is to separate the examples completely from the build,
after all there's no advantage to treating them as projects, and have a single or multiple shadow projects that would run ./gradlew ceheck on them as part of the main project check, this way we are not duplicating work and testing in a more meaningful manner.
This occurred to me while I was finishing this PR so I would rather have this merged first to re-enable these tests and do this at a lather time. We would need to use the worker API to be able to check them in parallel.

mark-vieira · 2019-07-08T18:28:59Z

buildSrc/build.gradle

  }

+  task integTest(type: Test) {
+    inputs.dir(file("src/testKit"))


We should add a property name (so this doesn't show in scan comparison as $1 or similar) and explicit path sensitivity so we get good cache reuse. We can also directly pass a path to dir() no need to convert to a file.

inputs.dir("src/testKit").withPropertyName("testKitDir").withPathSensitivity(PathSensitivity.RELATIVE)

mark-vieira · 2019-07-08T18:32:57Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/test/RestIntegTestTask.groovy

        }
        project.dependencies {
-            restSpec project.project(':rest-api-spec')
+            if (ClasspathUtils.isElasticsearchProject()) {


I don't think this will work when running tests via the :build-tools project as this will be considered "external". This will only be true in build logic, or when running the task in :buildSrc. We'll need to either explicitly add the buildSrc maker file to the test classpath here, or include some other flag to tell this utility it's running within the Elasticsearch build.

mark-vieira · 2019-07-08T18:34:40Z

buildSrc/src/test/java/org/elasticsearch/gradle/testclusters/TestClustersPluginIT.java

 import java.util.Arrays;

-@Ignore("https://github.com/elastic/elasticsearch/issues/42453")
 public class TestClustersPluginIT extends GradleIntegrationTestCase {


The usefulness of this test is pretty questionable at this point. I'd be more inclined to completely remove it.

mark-vieira · 2019-07-08T18:37:24Z

qa/build-standalone-example-plugins/build.gradle

+    FileCollection examplesToTest = files(
+            project(':example-plugins').subprojects.collect { it.projectDir }
+    )
+    inputs.files(examplesToTest.sort())


File order doesn't matter here (only for classpaths). We should declare relative path sensitivity though.

mark-vieira · 2019-07-08T18:41:35Z

qa/build-standalone-example-plugins/build.gradle

+    }
+}
+
+task integTest(type: Test) {


The main issue with moving these tests to their own project outside of buildSrc is now we are not tracking the build logic as an input in any way. So we'll continue to use cached results pretty much indefinitely until the sample projects themselves change. Alos, these tests are going to resolve the build-tools JAR "externally" now. So a breaking change in buildSrc will only manifest when the new JAR is pulled down, not on the build that included the breaking change.

Due to the issues above and the complexity involved in fixing them I think I'm inclined to leave this stuff part of buildSrc for simplicity. Essentially, these tests are to ensure changes in buildSrc have not caused a regression. For that reason, I think that is where the tests should live, not in a separate project.

That's only part of the picture. Changes in build-tools is one thing, but more often some dependency is added, e.x. a new library but publishing for it is not configured for it.
This was meant for a way to also test that we are publishing everything we need to, and eventually change the release manager so that we can generate it's configuration from Gradle instead of having a hard-coded list so that we could generate a configuration to publish everything that has publishing configured.

One way to solve this is to add the local repo as input, but that would void the cache most of the time, but It might be the only correct way, as some project might change and have it's publication removed, so the artifact will no longer be there.

but more often some dependency is added, e.x. a new library but publishing for it is not configured for it.

Under what scenario would this happen? The buildSrc project cannot depend on any other project, since it's needed at build time. So it's only dependencies would be other third-party libraries that will be available in public repos. If we want better isolation here we should do as you recommend and remove the example projects from the root project completely so that they have to get everything as a binary dependency.

One way to solve this is to add the local repo as input, but that would void the cache most of the time, but It might be the only correct way

We can just as easily just add :build-tools to the test classpath here. This is just as correct.

That said, I still don't quite see the benefits of this being a separate project. It doesn't buy us any additional coverage, but creates additional complexity. What we are testing is build-tools so I think these tests should really live there. None of the issues we had with example project testing was caused by the tests being part of the build-tools project.

IMO we should defer any significant refactoring here until we tackle the larger issues of third-party plugin development being basically undocumented, our buildSrc being littered with loads of internal stuff never intended to be used outside of the elasticsearch project, and our funky quasi-compatibility with Java 8.

By "significant refactoring" I mean just the addition of the separate project for example build testing.

We agreed with @rjernst a long time ago that we would move these to their own project, the main motivation is that it created a lot of confusion when these broke due to unpublished dependencies. A failure in build-tools didn't make it obvious at all what was going on.

Note that the integ tests are being ran as part of build-tools not buildSrc.

That being said I agree that we need to do all those things, but we should not hold up running these tests until all of that happens.

We agreed with @rjernst a long time ago that we would move these to their own project, the main motivation is that it created a lot of confusion when these broke due to unpublished dependencies.

By "own project" I just meant a separate CI build. The concern at the time was performance.

In any case, I still contend that there's no need to move these to a separate project to test dependency isolation. Simply using a GradleRunner and not passing withPluginClasspath() would have the same effect w/o the need for another project.

If we want to keep the separate QA project that's fine, we just need to track build-tools as an input. Let's not try to factor in all the other dependencies the build might use. We weren't testing that before so that hasn't changed. We are only doing assembly here so we don't actually care about the runtime behavior of those JARs, only that they exist.

jpountz · 2019-07-10T07:47:55Z

@atorok FYI I'm moving it to 7.4 since 7.3 is now feature frozen.

alpar-t · 2019-07-11T16:48:18Z

This was discussed and we decided that :

We want to check the POMs during the release process
- check that they don't reference unpublished artifacts
- the pom is vlaid XML
we don't want to build the example plugins as external projects but instead have a test for the plugin build plugin

…s-integ-test

alpar-t · 2019-07-12T08:41:07Z

@elasticmachine run elasticsearch-ci/1

…s-integ-test

alpar-t · 2019-09-16T15:15:50Z

@mark-vieira this is ready for review now. I added a simple unit test as discussed.
Sorry for the delay.

The test is meant to catch configuration that are not applicable when applying on a stand alone project.

mark-vieira · 2019-09-17T19:10:29Z

buildSrc/src/test/java/org/elasticsearch/gradle/plugin/PluginBuildPluginTests.java

+        extension.setDescription("just a test");
+        extension.setClassname(getClass().getName());
+
+        PluginBuildPlugin.configureAfterEvaluate(project);


I think the better thing to do here is simply call Project.evaluate(). It more accurately represents what will happen and will also trigger other afterEvaluate code that we haven't explicitly refactored in this way.

There is no (longer) such method. I tried calling it with reflection too.

Method evaluate = project.getClass().getDeclaredMethod("evaluate"); evaluate.invoke(project);

You'll have to cast it to ProjectInternal.

https://github.com/gradle/gradle/blob/master/subprojects/core/src/main/java/org/gradle/api/internal/project/ProjectInternal.java#L65

The cast works but then leads to a very strange cast exception down the road: https://gradle-enterprise.elastic.co/s/jjbp3xhyh3pvc/tests/td2ppmui4dyvs-7dxeqxtgf7z3g?openStackTraces=WzJd

I suspect what is happening here is that actually evaluating the project is exposing some weird behavior. Probably because we have some dependencies between plugins that aren't being well defined. Sounds like the nebula InfoJavaPlugin is doing something weird.

…s-integ-test

alpar-t · 2019-09-25T16:27:35Z

I bumped the version of the plugin and it works now.
Unfortunately I had to mute the test because it already picked up an issue, tracked in #47123

…s-integ-test

alpar-t · 2019-09-26T11:34:07Z

@mark-vieira ready for another review

mark-vieira

I think we could probably improve some of the assertions here but this is still better coverage than we had before. At least once we're able to reenable these tests.

buildSrc/src/test/java/org/elasticsearch/gradle/plugin/PluginBuildPluginTests.java

…#44056) Remove heavy build-tool integ test. Add a unit test for the plugin builder plugin. Closes #41256 #41256

alpar-t added >non-issue :Delivery/Build Build or test infrastructure v8.0.0 v7.3.0 labels Jul 8, 2019

alpar-t requested a review from mark-vieira July 8, 2019 08:44

mark-vieira requested changes Jul 8, 2019

View reviewed changes

jpountz added v7.4.0 and removed v7.3.0 labels Jul 10, 2019

alpar-t added 2 commits July 11, 2019 19:54

Merge remote-tracking branch 'origin/master' into break-up-build-tool…

2f974e5

…s-integ-test

Clean up as agreed

74abb93

colings86 added v7.5.0 and removed v7.4.0 labels Aug 30, 2019

Merge remote-tracking branch 'origin/master' into break-up-build-tool…

7640f2d

…s-integ-test

alpar-t changed the title ~~Split up build-tool integ test~~ Replace heavy weight build-tools integ tests with a simple unit test. Sep 16, 2019

alpar-t requested a review from mark-vieira September 16, 2019 15:15

Add a light weight test for applying the plugin

454a94c

The test is meant to catch configuration that are not applicable when applying on a stand alone project.

mark-vieira requested changes Sep 17, 2019

View reviewed changes

alpar-t added 3 commits September 24, 2019 11:03

Merge remote-tracking branch 'origin/master' into break-up-build-tool…

5581884

…s-integ-test

Merge remote-tracking branch 'origin/master' into break-up-build-tool…

a8dd30f

…s-integ-test

use ProjectInternal.evaluate

57c38e1

Merge remote-tracking branch 'origin/master' into break-up-build-tool…

12901c5

…s-integ-test

mark-vieira approved these changes Sep 26, 2019

View reviewed changes

buildSrc/src/test/java/org/elasticsearch/gradle/plugin/PluginBuildPluginTests.java Show resolved Hide resolved

alpar-t merged commit da6e0c6 into elastic:master Sep 30, 2019

alpar-t deleted the break-up-build-tools-integ-test branch September 30, 2019 07:23

alpar-t added a commit that referenced this pull request Sep 30, 2019

Replace heavy weight build-tools integ tests with a simple unit test. (…

3648481

…#44056) Remove heavy build-tool integ test. Add a unit test for the plugin builder plugin. Closes #41256 #41256

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

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Conversation

alpar-t commented Jul 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 8, 2019

Uh oh!

alpar-t commented Jul 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark-vieira Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz commented Jul 10, 2019

Uh oh!

alpar-t commented Jul 11, 2019

Uh oh!

alpar-t commented Jul 12, 2019

Uh oh!

alpar-t commented Sep 16, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpar-t commented Sep 25, 2019

Uh oh!

alpar-t commented Sep 26, 2019

Uh oh!

mark-vieira left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

alpar-t commented Jul 8, 2019 •

edited

Loading

mark-vieira Jul 9, 2019 •

edited

Loading