search

elastic / elasticsearch

Free and Open Source, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
69.58k stars 24.63k forks source link

Agg Test refactoring meta #51324

Open polyfractal opened 4 years ago

polyfractal commented 4 years ago

Spinoff from https://github.com/elastic/elasticsearch/issues/41713

Agg testing

Convert IT to AggregatorTestCase

We have a lot of older integration tests that could easily be converted to the newer AggregatorTestCase. Those would run faster and be easier to debug

https://github.com/elastic/elasticsearch/issues/42893

Tests for pipeline aggs

Many pipeline aggregations are IT-only and lack any kind of AggregatorTestCase tests. This was because AggTestCase couldn't handle pipeline aggs, but that limitation has since been removed.

https://github.com/elastic/elasticsearch/issues/36015

Remove BaseAggregationTestCase?

Many agg tests extend BaseAggregationTestCase, which has it's own to/from XContent, serialization, etc tests. Perhaps we should remove in favor of the newer AbstractSerializating etc base classes so that aggs don't have their own special testing?

https://github.com/elastic/elasticsearch/issues/42894

Standardize test style on testCase

Currently, the aggs tests are split between tests that define a testCase function, which typically takes a query, a lambda to add docs to the index, and a lambda to verify the aggregation. Other tests repeat the aggregator & index setup per test. The testCase form is preferred, and we should clean up the other style(s) to use that. Also, the testCase methods themselves can probably be pulled into AggregationTestCase.

Create a checklist of recommended Agg tests

Most aggs share a similar set of tests (match-all, match-none, filter, formatting, etc) but actual consistency between aggs is highly variable depending on lineage. We should create a standardized checklist of agg tests that we want all aggs to test. This would help prevent situations like missing and scripting being almost entirely untested across large swaths of the agg tests

Document current and deprecated test classes

Because of all the different test types, we should document their purpose and which should be avoided in a high-level package javadoc

Related, we should @deprecate classes we no longer want people to use.

elasticmachine commented 4 years ago

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

pedrochamberlain commented 3 years ago

Hello, is this issue still open? I'd like to give it a shot. Is this a good first issue?

nik9000 commented 3 years ago

Hi @pedrochamberlain! Check out the list in #26220 - maybe work on the missing aggregation for a nice first contribution. You can look at the more modern PRs linked from the list for inspiration in how.

We've had a lot of folks show up suddenly asking about contributions which is both great and a little overwhelming. Sometimes this happens when a professor tells their students to contribute to open source projects as part of a class. We discourage these sorts of contributions because folks don't tend to go all the way through the code review phase. The class tends to end and we end up with a bunch of half finished PRs that we've spent a while reviewing. Sorry for the paragraph, it's become a thing.

Anyway, if you are sure your willing to go through with the review process, please have a look at those tests!