[TEST] Add test for Aggregations#fromXContent #24524
Conversation
AggregationsTests#testFromXContent verifies that parsing of aggregations works by combining multiple aggs at the same level, and also adding sub-aggregations to multi bucket and single bucket aggs, up to a maximum depth of 5.
javanna
added
:Java High Level REST Client
>test
|
|
||
| import static java.util.Collections.singletonMap; | ||
|
|
||
| public class AggregationsTests extends ESTestCase { |
There was a problem hiding this comment.
Can you add some doc about the purpose of this test?
| aggsTests.add(new InternalGeoCentroidTests()); | ||
| aggsTests.add(new InternalHistogramTests()); | ||
| aggsTests.add(new InternalDateHistogramTests()); | ||
| return aggsTests; |
There was a problem hiding this comment.
nit: can it be an unmodifiable list?
| return createTestInstance(0, 5); | ||
| } | ||
|
|
||
| private static InternalAggregations createTestInstance(int currentDepth, int maxDepth) { |
| subAggregationsSupplier = () -> { | ||
| final int numAggregations = randomIntBetween(1, 3); | ||
| List<InternalAggregation> aggs = new ArrayList<>(); | ||
| for (int i = 0; i <numAggregations; i++) { |
|
|
||
| @Before | ||
| public void setup() { | ||
| @After //we force @After to have it run before ESTestCase#after otherwise it fails |
There was a problem hiding this comment.
it does not make any sense. let me dig why tests run though... I thought after needed to be forced on tearDown, let me verify that!
cbuescher
left a comment
cbuescher
left a comment
There was a problem hiding this comment.
I left two comment regarding setUp/tearDown. I generally find that part a bit hard to follow, but I see why it is done this way.
| public void setup() { | ||
| @After //we force @After to have it run before ESTestCase#after otherwise it fails | ||
| @Override | ||
| public void setUp() { |
There was a problem hiding this comment.
Does this need to call super.setUp() as well as it overrides LuceneTestCase#setUp()?
There was a problem hiding this comment.
yes it does. I have messed these two up for some reason. Probably ran only AggregationsTests and not the cardinality tests...fixing
| } | ||
|
|
||
| @Override | ||
| public void tearDown() { |
There was a problem hiding this comment.
Does this need to call super.tearDown() as well as it overrides LuceneTestCase#tearDown()?
I agree, and that is because there are multiple ways to do the same thing, but each one works slightly differently :) I wanted to have one single way to set things up in the base test class. THen I hooked into the lucene setup and teardown methods. That also makes sure that we don't forget to call super which is nice. Yet given that the annotations are in the base class LuceneTestCase, the after in ESTestCase runs first, and that is why in one case we have to force the After annotation in a subclass, so that tearDown is called before we check that we did clean things up. |
|
@tlrx @cbuescher I have addressed your comments, can you have another look please? |
AggregationsTests#testFromXContent verifies that parsing of aggregations works by combining multiple aggs at the same level, and also adding sub-aggregations to multi bucket and single bucket aggs, up to a maximum depth of 5.
3 participants
AggregationsTests#testFromXContent verifies that parsing of aggregations works by combining multiple aggs at the same level, and also adding sub-aggregations to multi bucket and single bucket aggs, up to a maximum depth of 5.