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 YAML REST tests for percentile_ranks metric aggregation #27127

Closed

Conversation

polyfractal
Copy link
Contributor

More YAML REST tests for aggregations. Pretty straightforward :)

Related to #26220

@polyfractal polyfractal added :Analytics/Aggregations Aggregations review >test Issues or PRs that are addressing/adding tests labels Oct 26, 2017
@polyfractal
Copy link
Contributor Author

retest this please

@polyfractal polyfractal removed the review label Nov 7, 2017
@cbuescher
Copy link
Member

@elasticmachine test this please

@colings86
Copy link
Contributor

@polyfractal is this still something you want to get merged? I'm a bit dubious about having YAML tests that check the exact values coming from algorithms like t-digest since it means that any update to improve the algorithm breaks the test, IMO we should have unit tests that check that the algorithm is working as expected and use the YAML tests to just check the API does what it's supposed to.

/cc @elastic/es-search-aggs

@polyfractal
Copy link
Contributor Author

Yes, I agree... these exact-value tests are super flaky and easy to break.

I'll see if I can pare these tests down to something that uses the agg's DSL without checking the actual values. Although I suspect there won't be a lot to actually test since the parameters directly affect the values and nothing else.

@colings86
Copy link
Contributor

I wonder if instead of check for an exact value we could instead check it is within an expected error of the accurate value given the data set is small? e.g. we could check in these tests that it is within 2% of the actual value

@polyfractal
Copy link
Contributor Author

Yep, that'd work. I thought about it at the time, but the yaml tests don't have an easy way to do it... you have to do use two checks (gte + lte) for each value, and I was feeling lazy :P

Perhaps just spot check a few values with the gte/lte pairs?

@colings86
Copy link
Contributor

Perhaps just spot check a few values with the gte/lte pairs?

👍

@polyfractal
Copy link
Contributor Author

Closing for now, almost certainly needs updating and don't have time at the moment.

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Sep 15, 2021
This revives elastic#27127, modernizing it to include stuff like `close_to`.
elasticsearchmachine pushed a commit that referenced this pull request Sep 15, 2021
This revives #27127, modernizing it to include stuff like `close_to`.
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Sep 15, 2021
This revives elastic#27127, modernizing it to include stuff like `close_to`.
elasticsearchmachine pushed a commit that referenced this pull request Sep 15, 2021
* Add integration test for percentile_ranks (#77815)

This revives #27127, modernizing it to include stuff like `close_to`.

* Backport update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants