Introduce yamlRestCompatTest task and plugin. #62985

jakelandis · 2020-09-28T22:02:14Z

This commit introduces a plugin to run REST compatibility tests.
The plugin is not currently applied to any projects with this commit,
but it establishes the foundation for testing REST compatibility.

This plugin will be applied to all core, plugin, and module projects
that currently have YAML based REST tests. This plugin, when applied
will do the following:

Create a new sourceset that extends from the yamlRestTest sourceset
Ensure the default distribution is used to execute the tests
Checkout bwc:minor version of the source code
Copy the rest api and rest tests from bwc:minor to the new sourceset
Create the test task, but manipulate the classpath so it uses the
"normal" YAML test runner, but the "compat" set of tests
wire up dependencies, ide, and check

The implementation here depends on :distribution:bwc:minor:checkoutBwcBranch
which is a convenient way to git checkout the most recent prior branch.
This approach works but is a bit fragile and long term would like a more
robust way to checkout arbitrary prior branches. This would be a fairly large
effort and is not included in this commit.

Also not included in this commit is the code necessary to inject the
compatible header, any customization to running the compatibility tests,
or any of the logic to override specific tests. Future commits will address
those concerns.

elasticmachine · 2020-09-29T13:42:30Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

jakelandis · 2020-09-29T13:49:38Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiTask.java

    final ListProperty<String> includeCore = getProject().getObjects().listProperty(String.class);
    final ListProperty<String> includeXpack = getProject().getObjects().listProperty(String.class);
    String sourceSetName;
+    boolean skipHasRestTestCheck;


This is needed so that you can run compat tests for a project that may no longer have YAML REST tests in master. Also, there may be cases where a module is completely deleted in master, but we will still want to use this plugin to only test compatibility.

This basically ensures that the execution of "compat" YAML tests is not directly coupled to the existence of "normal" YAML rest tests.

jakelandis · 2020-09-29T13:51:21Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiTask.java

+    boolean skipHasRestTestCheck;
    Configuration coreConfig;
    Configuration xpackConfig;
+    Configuration additionalConfig;


This is needed as a way to copy the 7.x module/plugin tests to the source set. For "normal" YAML tests they are already in the source set (since they are part of the current source code).

rjernst

We seem to be only testing against the previous minor, but shouldn't we create bwc test tasks for comparing against all compatible versions?

rjernst · 2020-09-29T18:45:45Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/YamlRestCompatTestPlugin.java

+            .getBuildDir()
+            .toPath()
+            .resolve("bwc")
+            .resolve("checkout-" + priorMajorVersion + ".x");


Why are we peeking at the source? Instead can we produce the files as an artifact? This would allow us to test against already released versions in the same way, just grab the zip of the spec/test files of that released version.

The artifacts we produce only contain the OSS core api and tests. I need the x-pack core api/tests as well as all of the OSS/X-pack module/plugin API/tests. Back filling those I think is a reasonable approach as a long term solution, but I think sourcing like this is decent temporary approach.

I tend to agree with @rjernst that peeking into other source folders is kind of an anti pattern. Instead sharing it declaratively as described here: https://docs.gradle.org/current/userguide/cross_project_publications.html is what we should aim for.

Ideally I think the bwc project would just share the checkoutDir as an artifact. Can you put a TODO in there to look into this after #62473 I will then pick that up at one. point. This PR does quite some rework on the bwc setup

Thanks, exposing the checkoutDir as an artifact, I think will clean this up. I added a TODO in the code to come back to this.

jakelandis · 2020-09-29T22:07:54Z

We seem to be only testing against the previous minor, but shouldn't we create bwc test tasks for comparing against all compatible versions?

Yes, testing against all prior minor versions is the long term goal. This PR is satisfies the short term goal that will allow the bare min. needed to start testing compatibility. I am presuming that testing compatibility against the latest minor should be part of the project's check task, and testing against all branches would be a new lifecycle task (similar to bwc).

To satisfy the long term goal (and introduce a new lifecycle task to test all prior minors) we will need a way to checkout an arbitrary branch/tag or publish artifacts for ALL of the rest tests for each minor version (the existing artifacts only contain the OSS api/tests). This will need further discussion, but hoping that this approach is sufficient as a starting point.

breskeby

I left some remarks regarding resolving and sharing artefacts among projects similar to what @rjernst commented and added a remark about avoiding project.copy{} at execution time. We probably want to tackle this sharing resources as artifacts in a separate story and not as part of this PR. If not we should at least create a ticket for this problem

breskeby · 2020-09-30T15:17:24Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/YamlRestCompatTestPlugin.java

+            .getBuildDir()
+            .toPath()
+            .resolve("bwc")
+            .resolve("checkout-" + priorMajorVersion + ".x");


I tend to agree with @rjernst that peeking into other source folders is kind of an anti pattern. Instead sharing it declaratively as described here: https://docs.gradle.org/current/userguide/cross_project_publications.html is what we should aim for.

breskeby · 2020-09-30T15:25:41Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiTask.java

        // always copy the core specs if the task executes
        if (BuildParams.isInternal()) {
            getLogger().debug("Rest specs for project [{}] will be copied to the test resources.", project.getPath());
            project.copy(c -> {


We'd like to get away from using the project property within task actions. I'm currently working on #62968 Instead using project.copy{} we should inject a FileSystemOperations service and use that one for these kind of operations. See for example the LoggedExec task how this service is injected: https://github.com/elastic/elasticsearch/pull/62968/files#diff-babd4b74d83a9cea8bf0823cf6b71b66

I added some TODO's for the new project.copy's to ensure it uses the newer style once that PR lands. I didn't make the change now to avoid avoid copy/paste code from your PR (which could merge clash). If this PR beats yours then feel free to add it to your, else I will circle back to this.

breskeby · 2020-09-30T15:25:54Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/CopyRestApiTask.java

        }
+        // copy any additional config
+        if (additionalConfig != null) {
+            project.copy(c -> {


same as above

breskeby · 2020-09-30T15:28:13Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/YamlRestCompatTestPlugin.java

+            .getBuildDir()
+            .toPath()
+            .resolve("bwc")
+            .resolve("checkout-" + priorMajorVersion + ".x");


Ideally I think the bwc project would just share the checkoutDir as an artifact. Can you put a TODO in there to look into this after #62473 I will then pick that up at one. point. This PR does quite some rework on the bwc setup

breskeby · 2020-09-30T15:34:19Z

buildSrc/src/main/java/org/elasticsearch/gradle/test/rest/YamlRestCompatTestPlugin.java

+                project.getDependencies()
+                    .add(
+                        task.coreConfig.getName(),
+                        project.files(checkoutDir.resolve("rest-api-spec/src/main/resources").resolve(RELATIVE_API_PATH))


same here. We should not rely on file dependencies and provide these dependencies as artifacts of the bwc project.

jakelandis · 2020-09-30T21:01:06Z

Add some more TODO's to revisit after Rene's PRs land as well as to test multiple versions using published artifacts as discussed.

This commit introduces a plugin to run REST compatibility tests. The plugin is not currently applied to any projects with this commit, but it establishes the foundation for testing REST compatibility. This plugin will be applied to all core, plugin, and module projects that currently have YAML based REST tests. This plugin, when applied will do the following: * Create a new sourceset that extends from the yamlRestTest sourceset * Ensure the default distribution is used to execute the tests * Checkout bwc:minor version of the source code * Copy the rest api and rest tests from bwc:minor to the new sourceset * Create the test task, but manipulate the classpath so it uses the "normal" YAML test runner, but the "compat" set of tests * wire up dependencies, ide, and check The implementation here depends on :distribution:bwc:minor:checkoutBwcBranch which is a convenient way to git checkout the most recent prior branch. This approach works but is a bit fragile and long term would like a more robust way to checkout arbitrary prior branches. This would be a fairly large effort and is not included in this commit. Also not included in this commit is the code necessary to inject the compatible header, any customization to running the compatibility tests, or any of the logic to override specific tests. Future commits will address those concerns.

Backport for #62985 that includes the related changes, but not the actual plugin for yamlRestCompatTest. The plugin is not necessary in 7.x, and back porting relevant changes to help keep 7.x code inline with master.

jakelandis added 10 commits September 24, 2020 08:59

initial commit

f100cb0

add missing file

42372d5

wip

339695e

remove plugin that layers in

ef6bc3c

use copy resources

96245d7

remove testing and remove sourceSetForOutput

24aa853

remove testing stuff

ba34a16

spotless

85a26b3

minor wording

f3878a1

Merge remote-tracking branch 'upstream/master' into rest_compat_test

b6b05ce

jakelandis requested review from breskeby, mark-vieira and rjernst September 29, 2020 13:41

jakelandis added :Delivery/Build Build or test infrastructure >non-issue v7.10.0 v8.0.0 labels Sep 29, 2020

jakelandis marked this pull request as ready for review September 29, 2020 13:42

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Sep 29, 2020

jakelandis commented Sep 29, 2020

View reviewed changes

rjernst reviewed Sep 29, 2020

View reviewed changes

breskeby approved these changes Sep 30, 2020

View reviewed changes

jakelandis added 2 commits September 30, 2020 15:47

clean up TODO's

428babb

add another TODO

d0ebbcd

spotless

f03894e

jakelandis merged commit f56bf94 into elastic:master Sep 30, 2020

jakelandis deleted the rest_compat_test branch September 30, 2020 22:09

jakelandis added the backport pending label Sep 30, 2020

jakelandis mentioned this pull request Sep 30, 2020

[7.x] Introduce changes related to yamlRestCompatTest (#62985) #63105

Merged

breskeby mentioned this pull request Oct 2, 2020

Checkout dir should be available as artifact from bwc projects #63173

Merged

mark-vieira added Team:Delivery Meta label for Delivery team and removed Team:Core/Infra Meta label for core/infra team labels Nov 11, 2020

jakelandis removed backport pending v7.10.0 labels Mar 2, 2021

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

Introduce yamlRestCompatTest task and plugin. #62985

Introduce yamlRestCompatTest task and plugin. #62985

Uh oh!

Conversation

jakelandis commented Sep 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Sep 29, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst left a comment

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!

jakelandis commented Sep 29, 2020

Uh oh!

breskeby left a comment

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!

jakelandis commented Sep 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jakelandis commented Sep 28, 2020 •

edited

Loading