Run aggs tests in aggs module by nik9000 · Pull Request #90851 · elastic/elasticsearch

nik9000 · 2022-10-12T18:30:56Z

Run the aggregations tests v7 compat tests against the aggregations module and not the rest-api-spec module. This allows us to drop rest-api-spec's dependency on the aggregations module and keep it "just the server" which is nice.

There are a few side effects here that are ok:

We run all aggregations REST tests in the aggregations module. Even the ones in rest-api-spec. This means we run them twice. We plan to move all of the aggregations REST tests into the aggregations module anyway.
We now bundle the REST tests in the aggregations module into the tests that the clients run for their verification step. This should keep our clients from losing coverage.

Relates to #90283

Run the aggregations tests v7 compat tests against the aggregations module and *not* the `rest-api-spec` module. This allows us to drop `rest-api-spec`'s dependency on the aggregations module and keep it "just the server" which is nice. There are a few side effects here that are ok: 1. We run all aggregations REST tests in the aggregations module. Even the ones in `rest-api-spec`. This means we run them twice. We plan to move all of the aggregations REST tests into the aggregations module anyway. 2. We now bundle the REST tests in the aggregations module into the tests that the clients run for their verification step. This should keep our clients from losing coverage.

elasticsearchmachine · 2022-10-12T18:31:19Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

nik9000 · 2022-10-12T18:32:10Z

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

+                            .toPath()
+                            .resolve(RELATIVE_REST_CORE)
+                            .resolve(RELATIVE_REST_PROJECT_RESOURCES)
+                            .resolve(RELATIVE_TEST_PATH)


The old path didn't exist. I could have merged the paths some more but I don't feel particularly confident working on this code. I am a little worried what'll happen when I "fix" this. Will we start running tests we hadn't been running before? CI will tell me if any of them fail at least....

Ah, so this was just wrong before I assume?

I think so! The path didn't exist and nothing was ever copied.

Indeed this is a bug. Fortunately it only impacts copying tests from core (includeCore) for modules that have the yaml-rest-compat-test plugin. It seems that this only impacts this aggs module.

However, while reviewing i see that we have 2 projects (enrich, watcher with security) attempting to copy some tests from xpack (includeXpack) that are not running the compat tests as they should be since the path to the x-pack tests is also wrong. I will followup on that in a different PR.

#90925 to fix the x-pack path, and #90929 to be a bit smarter here.

nik9000 · 2022-10-12T18:32:29Z

modules/aggregations/build.gradle

+  task.skipTest("search.aggregation/210_top_hits_nested_metric/top_hits aggregation with sequence numbers", "#42809 the use nested path and filter sort throws an exception")
+  task.skipTest("search.aggregation/370_doc_count_field/Test filters agg with doc_count", "Uses profiler for assertions which is not backwards compatible")
+
+  task.addAllowedWarningRegex("\\[types removal\\].*")


These were all from the rest-api-spec module

nik9000 · 2022-10-12T18:33:35Z

x-pack/rest-resources-zip/build.gradle

 dependencies {
  apis project(path: ':rest-api-spec', configuration: 'restSpecs')
  freeTests project(path: ':rest-api-spec', configuration: 'restTests')
+  freeTests project(path: ':modules:aggregations', configuration: 'restTests')


The clients were going to lose tests every time we port things from rest-api-spec into the aggregations module without this.

martijnvg

I can't judge the change to YamlRestCompatTestPlugin, otherwise LGTM.

We run all aggregations REST tests in the aggregations module. Even the ones in rest-api-spec. This means we run them twice. We plan to move all of the aggregations REST tests into the aggregations module anyway.

This is a lesser concern to me, because it is temporary. Maybe we can move all core aggs yaml rest tests first in a followup change and then later move the actual aggs to the new module? I suspect most yaml tests will keep working without other changes, should just be a simple git move?

martijnvg · 2022-10-13T09:29:54Z

rest-api-spec/build.gradle

-  // This dependency can be removed when aggregations and
-  // respective yaml test have been moved to aggregations module.
-  // See for the progress: https://github.com/elastic/elasticsearch/issues/90283
-  module ':modules:aggregations'


mark-vieira

LGTM

mark-vieira · 2022-10-13T20:37:46Z

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

+                            .toPath()
+                            .resolve(RELATIVE_REST_CORE)
+                            .resolve(RELATIVE_REST_PROJECT_RESOURCES)
+                            .resolve(RELATIVE_TEST_PATH)


Ah, so this was just wrong before I assume?

jakelandis

LGTM , thanks for fixing this.

Compatible REST tests (like normal REST tests) allow copying the tests from either `rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test` (includeCore) or `x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test` (includeXpack). Both are currently broken. #90851 fixes the former , and this commit fixes the later. The fix here is to ensure the correct path to the x-pack rest tests when pulling from the elder branch for compatible testing. This only impacts modules that have the compatibility plugin enabled and are configured to pull from the main x-pack set of tests. Currently, this should only impact `:x-pack:plugin:enrich:qa:rest` but will fix it for future projects with that configuration.

nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v8.6.0 labels Oct 12, 2022

nik9000 requested review from jakelandis, mark-vieira and martijnvg and removed request for mark-vieira October 12, 2022 18:31

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Oct 12, 2022

nik9000 commented Oct 12, 2022

View reviewed changes

nik9000 mentioned this pull request Oct 12, 2022

Move mov_fn agg to module #90836

Merged

nik9000 added 2 commits October 12, 2022 14:46

Fix doc

e59d77b

Better grouping

51dabd4

martijnvg approved these changes Oct 13, 2022

View reviewed changes

mark-vieira approved these changes Oct 13, 2022

View reviewed changes

jakelandis approved these changes Oct 14, 2022

View reviewed changes

jakelandis mentioned this pull request Oct 14, 2022

Fix path to compatible x-pack tests #90925

Merged

nik9000 merged commit cd4116c into elastic:main Oct 17, 2022

Conversation

nik9000 commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Oct 12, 2022

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!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mark-vieira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nik9000 commented Oct 12, 2022 •

edited

Loading