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

Add REST tests for histogram aggregation #72322

Merged
merged 2 commits into from
Apr 28, 2021

Conversation

csoulios
Copy link
Contributor

Added more tests for histogram aggregation
Related to #26220

@csoulios csoulios added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v8.0.0 v7.14.0 labels Apr 27, 2021
@csoulios csoulios requested a review from nik9000 April 27, 2021 16:35
@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 27, 2021
@elasticmachine
Copy link
Collaborator

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

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Wonderful! I left a few minor things. I think the new lines point I left is important for consistency with the more modern yaml tests, but the others are all your call if you want to apply them. Either way, LGTM. No need for another review pass.


---
"Show empty intervals":
- do:
Copy link
Member

Choose a reason for hiding this comment

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

I tend to use bulk for these just to save a bit of typing. Maybe it faster, but it makes the tests more likely to fit onto one screen. Even refreshing is just one line in the bulk.


- do:
search:
rest_total_hits_as_int: true
Copy link
Member

Choose a reason for hiding this comment

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

I tend not to add these because I think we want to remove them from master in the end. They are useful on backport though. Either way is cool.

- do:
search:
rest_total_hits_as_int: true
body: { "aggs" : { "histo" : { "histogram" : { "field" : "number", "interval" : 20 } } } }
Copy link
Member

Choose a reason for hiding this comment

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

I think it's easier to read when these are on individual lines. I tend to write them in yaml instead of json too just because we're in yaml files. But I think that is less important than the line feeds.

@csoulios
Copy link
Contributor Author

Nik thanks a lot for the feedback. I applied all changes you suggested in 7319bab

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@csoulios csoulios merged commit 99e2cf0 into elastic:master Apr 28, 2021
@csoulios csoulios deleted the test-histogram-agg branch April 28, 2021 14:54
csoulios added a commit that referenced this pull request Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants