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.

Sign up for GitHub

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

ES|QL fix telemetry tests (usage stats) after promoting CATEGORIZE #117878

Merged
merged 5 commits into from
Dec 3, 2024
Merged

ES|QL fix telemetry tests (usage stats) after promoting CATEGORIZE #117878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5c14a80
ES|QL fix telemetry tests (usage stats) after promoting CATEGORIZE to…
luigidellaquila Dec 3, 2024
31f10f3
Make CATEGORIZE_V4 capability non-snapshot
luigidellaquila Dec 3, 2024
1bc234e
Merge branch 'main' into esql/fix_categorize_metrics_test
luigidellaquila Dec 3, 2024
0f8b6d3
Merge branch 'main' into esql/fix_categorize_metrics_test
luigidellaquila Dec 3, 2024
bf9e076
Merge branch 'main' into esql/fix_categorize_metrics_test
luigidellaquila Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions muted-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,9 +236,6 @@ tests:
issue: https://github.com/elastic/elasticsearch/issues/117815
- class: org.elasticsearch.xpack.ml.integration.DatafeedJobsRestIT
issue: https://github.com/elastic/elasticsearch/issues/111319
- class: org.elasticsearch.xpack.test.rest.XPackRestIT
method: test {p0=esql/60_usage/Basic ESQL usage output (telemetry) non-snapshot version}
issue: https://github.com/elastic/elasticsearch/issues/117862

# Examples:
#
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/60_usage.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,4 +163,4 @@ setup:
- match: {esql.functions.cos: $functions_cos}
- gt: {esql.functions.to_long: $functions_to_long}
- match: {esql.functions.coalesce: $functions_coalesce}
- length: {esql.functions: 118} # check the "sister" test above for a likely update to the same esql.functions length check
- length: {esql.functions: 119} # check the "sister" test above for a likely update to the same esql.functions length check
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the other test be changed as well?

Also, I am not clear about the "promotion" of categorize. Looking at EsqlCapabilities I see it's still a snapshot only feature test-wise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if this is about snapshot/non-snapshot thingies, shouldn't the PR run with test-release label?
Even the issue it's trying to fix (#117862) comes from test-release CI runs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it's still a snapshot only feature test-wise.
It's going to be un-snapshotted soon!

Thanks for pointing out the test-release label, I was unaware of that one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other test is good because the function existed already (but only in snapshot).

The promotion happened in EsqlFunctionRegistry, I think the capabilities should be changed as well. Since we are on it, let me do it now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, the un-snapshotting of the capability in #117881 should make sure that the CATEGORIZE test also run on release tests. Thanks for looking into this Luigi!