Skip to content

Make some agg tests easier to read #54954

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

Merged
merged 1 commit into from
Apr 10, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 8, 2020

We added a fancy method to provide random realistic test data to the
reduction tests in #54910. This uses that to remove some of the more
esoteric machinations in the agg tests. This will marginally increase
the coverage of the serialiation tests and, more importantly, remove
some mysterious value generation code that only really made sense for
random reduction tests but was used all over the place. It doesn't, on
the other hand, make the tests shorter. Just hopefully more clear.

I only cleaned up a few tests this way. If we like this it'd probably be
worth grabbing others.

We added a fancy method to provide random realistic test data to the
reduction tests in elastic#54910. This uses that to remove some of the more
esoteric machinations in the agg tests. This will marginally increase
the coverage of the serialiation tests and, more importantly, remove
some mysterious value generation code that only really made sense for
random reduction tests but was used all over the place. It doesn't, on
the other hand, make the tests shorter. Just *hopefully* more clear.

I only cleaned up a few tests this way. If we like this it'd probably be
worth grabbing others.
@nik9000 nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/Aggregations Aggregations v8.0.0 v7.8.0 labels Apr 8, 2020
@nik9000 nik9000 requested a review from imotov April 8, 2020 13:07
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

Oh, that's a nice refactoring. I was a bit concerned that we were overdoing it, but I now can clearly see that his abstraction was indeed really needed there. Thanks!

@nik9000
Copy link
Member Author

nik9000 commented Apr 10, 2020

Oh, that's a nice refactoring. I was a bit concerned that we were overdoing it, but I now can clearly see that his abstraction was indeed really needed there. Thanks!

I opened 3 random tests. All three needed it.

@nik9000 nik9000 merged commit da5cf73 into elastic:master Apr 10, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Apr 10, 2020
We added a fancy method to provide random realistic test data to the
reduction tests in elastic#54910. This uses that to remove some of the more
esoteric machinations in the agg tests. This will marginally increase
the coverage of the serialiation tests and, more importantly, remove
some mysterious value generation code that only really made sense for
random reduction tests but was used all over the place. It doesn't, on
the other hand, make the tests shorter. Just *hopefully* more clear.

I only cleaned up a few tests this way. If we like this it'd probably be
worth grabbing others.
nik9000 added a commit that referenced this pull request Apr 10, 2020
We added a fancy method to provide random realistic test data to the
reduction tests in #54910. This uses that to remove some of the more
esoteric machinations in the agg tests. This will marginally increase
the coverage of the serialiation tests and, more importantly, remove
some mysterious value generation code that only really made sense for
random reduction tests but was used all over the place. It doesn't, on
the other hand, make the tests shorter. Just *hopefully* more clear.

I only cleaned up a few tests this way. If we like this it'd probably be
worth grabbing others.
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 v7.8.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants