Use versioned sourceset for REST compat testing #78418

jakelandis · 2021-09-28T21:16:35Z

This commit changes the sourceset name for REST API compatility testing
from yamlRestCompatTest to yamlRestTestV(N)Compat wher (N) is the version
from which the tests are sourced. For example yamlRestTestV7Compat
or yamlRestTestV8Compat.

Two motivating reasons for this change.

consitent naming - all compatible rest testing will now use a common
prefix of yamlRestTestV7Compat where the task name to execute is
yamlRestTestV7CompatTestand the transformation task is yamlRestTestV7CompatTransform
It makes sense that corresponding source set follows this convention.
Better support when the major version changes. Gradle uses the source
set under the covers to execute the compatiblity tests. However, a user
can add custom REST tests to this source set. By using dynamically named
source sets, there should little concern of accidentally running the wrong
version tests. Additionaly it is visualy easy to tell which version the tests are
from.

This commit also removes the unecessary duplicated v7compat directory
and adds copy and classpath duplicate checks to ensure that we don't
accidentally stomp on duplicates with all of the copy and classpath manipulation.

related: #75267

…e_set

elasticmachine · 2021-09-30T16:28:20Z

Pinging @elastic/es-delivery (Team:Delivery)

mark-vieira · 2021-09-30T18:57:03Z

...al/src/main/java/org/elasticsearch/gradle/internal/rest/compat/YamlRestCompatTestPlugin.java

@@ -55,6 +54,7 @@
    @Override
    public void apply(Project project) {
        final int compatibleVersion = Version.fromString(VersionProperties.getVersions().get("elasticsearch")).getMajor() - 1;
+        final String sourceSetName = "yamlRestTestV" + compatibleVersion + "Compat";
        final Path compatRestResourcesDir = Path.of("restResources").resolve("v" + compatibleVersion);


If we are having version-specific source sets, do we still need to capture the version here in the file path as well. As it is now we have files like src/yamlRestTestV7Compat/resources/rest-api-spec/test/v7compat/foo/bar.yml. We probably don't need to track the version twice now. Also begs the question whether we need this change in the first place if we are already managing per-version test resources. How will this affect backporting, etc?

src/yamlRestTestV7Compat/resources/rest-api-spec/test/v7compat/foo/bar.yml uses a 4 part path to keept it from colliding with the actual v7 test/foo/bar.yml if it exists. (Both the real v7 tests and the custom test in this source set are on the same classpath, and there quite a bit of re-use of file names). v7compat in the path is just a convention and included the v7 to help demark these are intended for compatibilty with v7. The fact this is just a convention and without any teeth (i.e. these would execute when master is v9) is a motivating factor for this PR.

I can remove the v7 from the v7compat convention in this PR since I agree it doesn't make sense now. I think I could also remove this extra directory entirely and ensure uniqueness of the files between the two sources of tests (with validation in Gradle). Thoughts ?

Since our REST compatilbity only supports 1 major version, these should never be backported and anything in this sourceset should be removed in the next major version.

I can remove the v7 from the v7compat convention in this PR since I agree it doesn't make sense now. I think I could also remove this extra directory entirely and ensure uniqueness of the files between the two sources of tests (with validation in Gradle). Thoughts ?

Yeah, I think that makes sense. It's just less stuff to try and keep in sync.

x-pack/plugin/build.gradle

…rsion_source_set

jakelandis · 2021-09-30T22:18:04Z

...ools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/CopyRestApiTask.java

@@ -126,13 +127,15 @@ void copy() {

        getLogger().debug("Rest specs for project [{}] will be copied to the test resources.", projectPath);
        fileSystemOperations.copy(c -> {
+            c.setDuplicatesStrategy(DuplicatesStrategy.FAIL);


These duplicate fails are not actually needed for this PR, but I realized while adding the custom duplicate check to the compat testing task, it would be good to add a duplicate check here too.

I don't think this is necessary since FAIL is now the default behavior in Gradle 7.0 and later.

ah.. it is. gradle/gradle#15759 I will remove this.

jakelandis · 2021-09-30T22:32:02Z

...al/src/main/java/org/elasticsearch/gradle/internal/rest/compat/YamlRestCompatTestPlugin.java

+                            + ".x] branch and the [" + sourceSetName + "] tests. ");
+                    }
+                }
+            });


example of failure:

* What went wrong: Execution failed for task ':rest-api-spec:yamlRestTestV7CompatTest'. > Found duplicated test(s) [search/30_limits.yml] please ensure there not any duplicate YAML test files between the [7.x] branch and the [yamlRestTestV7Compat] tests.

I'm wondering if it might be better (and simpler) to implement this in the yaml testing framework code instead. It could be as simple as checking the result of the call to add() here and throwing an exception if it returns false.

jakelandis · 2021-09-30T22:38:11Z

@mark-vieira - should be ready for another review. I implemented a duplicate check to ensure that the same test file is not on the class path more then once. 98% of the tests use 2 parts (foo/bar.yml) under the folder test and the duplicate check checks for those 2 parts. There is a slim chance of a false positive if a test uses 3 parts and the custom compat uses 2 part and there is name collision, but that is extremely unlikely.

I also removed the uncessary v7compat directory now that there is a proper check for duplicates.

jakelandis · 2021-10-01T16:54:14Z

@elasticmachine update branch

...al/src/main/java/org/elasticsearch/gradle/internal/rest/compat/YamlRestCompatTestPlugin.java

mark-vieira · 2021-09-30T22:49:48Z

...al/src/main/java/org/elasticsearch/gradle/internal/rest/compat/YamlRestCompatTestPlugin.java

+                            + ".x] branch and the [" + sourceSetName + "] tests. ");
+                    }
+                }
+            });


I'm wondering if it might be better (and simpler) to implement this in the yaml testing framework code instead. It could be as simple as checking the result of the call to add() here and throwing an exception if it returns false.

elasticmachine · 2021-10-04T21:12:11Z

Pinging @elastic/clients-team (Team:Clients)

…e_set

jakelandis · 2021-10-12T00:52:13Z

I'm wondering if it might be better (and simpler) to implement this in the yaml testing framework code instead. It could be as simple as checking the result of the call to add() here and throwing an exception if it returns false.

Changed on 2a8fc87 (#78418)

It turns out we actually do support running duplicates on the classpath without stomping on each other. The framework will append a #2 to each test name. In light of this, i reduced the error down to a warning since which test (the first or the second with the same name) gets the #2 appended is dependent on the classloading order. We probably could add in some specific ordering of the collections so it wouldn't be classloader ordering but it would be some other non obvious ordering. The new warning is there in part to inform future developers that we actually do support duplicated names.

Example of the new warning message:

org.elasticsearch.test.rest.ClientYamlTestSuiteIT STANDARD_OUT
    [2021-10-11T19:44:38,076][WARN ][o.e.t.r.y.ESClientYamlSuiteTestCase] [[Test worker]] Found duplicate test name [search/10_source_filtering.yml] on the class path. This can result in class loader dependent execution commands and reproduction commands (will add #2 to one of the test names dependent on the classloading order)

I don't have a strong opinion to keep or leave this warning.

jakelandis · 2021-10-12T01:01:03Z

@mark-vieira - finally got back round to this. should be ready for another review

mark-vieira

LGTM

jakelandis · 2021-10-12T20:01:14Z

@elasticmachine update branch

jakelandis added 3 commits September 28, 2021 16:13

Use versioned sourceset for REST compat testing

a29644b

rename

afa5451

fix typo

ef2b65b

elasticsearchmachine added the v8.0.0 label Sep 28, 2021

jakelandis added 4 commits September 29, 2021 09:39

add Compat to sourceset name

4ddbae0

Merge remote-tracking branch 'upstream/master' into use_version_sourc…

251433d

…e_set

minor fixes

5ef319d

update dir names

0cba9f0

jakelandis marked this pull request as ready for review September 30, 2021 16:28

jakelandis added :Delivery/Build Build or test infrastructure >non-issue labels Sep 30, 2021

elasticmachine added the Team:Delivery Meta label for Delivery team label Sep 30, 2021

jakelandis requested a review from mark-vieira September 30, 2021 16:28

jakelandis requested a review from pgomulka September 30, 2021 16:28

mark-vieira reviewed Sep 30, 2021

View reviewed changes

jakelandis commented Sep 30, 2021

View reviewed changes

x-pack/plugin/build.gradle Outdated Show resolved Hide resolved

jakelandis added 4 commits September 30, 2021 15:25

fix todo

0a6e178

implement duplicate check and move stuff around

1f4d065

moved stuff around

69d2450

Merge branch 'master' of github.com:elastic/elasticsearch into use_ve…

342516c

…rsion_source_set

jakelandis commented Sep 30, 2021

View reviewed changes

jakelandis added 2 commits October 1, 2021 09:25

remove unecessary DuplicatesStrategy.FAIL

cd33601

fix import

23b8c62

Merge branch 'master' into use_version_source_set

cf7b457

mark-vieira requested changes Oct 1, 2021

View reviewed changes

sethmlarson added the Team:Clients Meta label for clients team label Oct 4, 2021

jakelandis added 3 commits October 11, 2021 16:50

Merge remote-tracking branch 'upstream/master' into use_version_sourc…

b267512

…e_set

fix merge issue

cdf7dab

remove gradle duplicate check in favor of testing framwork fix

2a8fc87

static and import cleanup

00a15e1

mark-vieira approved these changes Oct 12, 2021

View reviewed changes

Merge branch 'master' into use_version_source_set

e45d9a1

jakelandis merged commit ab221d7 into elastic:master Oct 12, 2021

jakelandis deleted the use_version_source_set branch October 12, 2021 23:25

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use versioned sourceset for REST compat testing #78418

Use versioned sourceset for REST compat testing #78418

jakelandis commented Sep 28, 2021 •

edited

Loading

elasticmachine commented Sep 30, 2021

mark-vieira Sep 30, 2021

jakelandis Sep 30, 2021

mark-vieira Sep 30, 2021

jakelandis Sep 30, 2021

mark-vieira Sep 30, 2021

jakelandis Oct 1, 2021

jakelandis Sep 30, 2021

mark-vieira Sep 30, 2021

jakelandis commented Sep 30, 2021

jakelandis commented Oct 1, 2021

mark-vieira Sep 30, 2021

elasticmachine commented Oct 4, 2021

jakelandis commented Oct 12, 2021 •

edited

Loading

jakelandis commented Oct 12, 2021

mark-vieira left a comment

jakelandis commented Oct 12, 2021

Use versioned sourceset for REST compat testing #78418

Use versioned sourceset for REST compat testing #78418

Conversation

jakelandis commented Sep 28, 2021 • edited Loading

elasticmachine commented Sep 30, 2021

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

jakelandis commented Sep 30, 2021

jakelandis commented Oct 1, 2021

Choose a reason for hiding this comment

elasticmachine commented Oct 4, 2021

jakelandis commented Oct 12, 2021 • edited Loading

jakelandis commented Oct 12, 2021

mark-vieira left a comment

Choose a reason for hiding this comment

jakelandis commented Oct 12, 2021

jakelandis commented Sep 28, 2021 •

edited

Loading

jakelandis commented Oct 12, 2021 •

edited

Loading