Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST tests for moving_fn agg #90012

Merged
merged 4 commits into from
Sep 13, 2022

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Sep 12, 2022

This expands on the REST layer tests for the moving_fn agg asserting the results of the various moving functions, some failure cases, and some access edge cases. These tests buy us backwards compatibility tests and, eventually, forwards compatibility testing.

@nik9000 nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v8.5.0 labels Sep 12, 2022
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Sep 12, 2022
@elasticsearchmachine
Copy link
Collaborator

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

@nik9000
Copy link
Member Author

nik9000 commented Sep 12, 2022

These tests were in the lang-painless module which is a little confusing, but it's because we can't really test the fun edge cases of the agg without a scripting language. We have unit tests that do that in the server project but we'd like the forwards and backwards tests that we get from doing it with painless. I don't know of a better place for this, but if anyone has any ideas I'm happy to move it.

This expands on the REST layer tests for the `moving_fn` agg asserting
the results of the various moving functions, some failure cases, and
some access edge cases.
Copy link
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

These tests were in the lang-painless module which is a little confusing,

This shouldn't block this PR, but could we move these tests to rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation ? I think when running these yaml tests we run we the default distribution (which has lang-painless)? And other aggregation yaml tests are here as well.

Another idea that is way outside of the scope of this PR, we could have an analytics module. This module could contain almost all aggregations and then server only contains aggregation infrastructure. This way the module can have its own yaml test task and we could add the lang-painless module as an integration test dependency to this new module. This will have other benefits, such as make server module smaller and make it easier to run aggregation tests. Just an idea, that I wanted to share here...

Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Do you think we should leave one or two tests in server with a comment about where to find the rest of them? Just so someone looking at the "usual place" doesn't think this is untested?

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2022

Do you think we should leave one or two tests in server with a comment about where to find the rest of them? Just so someone looking at the "usual place" doesn't think this is untested?

Yeah, probably.

This shouldn't block this PR, but could we move these tests to rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation ? I think when running these yaml tests we run we the default distribution (which has lang-painless)? And other aggregation yaml tests are here as well.

Another idea that is way outside of the scope of this PR, we could have an analytics module. This module could contain almost all aggregations and then server only contains aggregation infrastructure

The last time I checked they didn't have painless. I'll have another look though. The submodule is a neat idea though.

@nik9000
Copy link
Member Author

nik9000 commented Sep 13, 2022

The last time I checked they didn't have painless. I'll have another look though. The submodule is a neat idea though.

            stack_trace: "java.lang.IllegalArgumentException: script_lang not supported [painless]\n\

@nik9000 nik9000 added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Sep 13, 2022
@elasticsearchmachine elasticsearchmachine merged commit 953a0dd into elastic:main Sep 13, 2022
@nik9000 nik9000 deleted the rest_moving_function branch September 13, 2022 20:09
@martijnvg
Copy link
Member

stack_trace: "java.lang.IllegalArgumentException: script_lang not supported [painless]\n\

:(
Thinking more about this, another complication here is that rest api spect yaml tests are reused several times running ES in different kind of setups.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants