Skip to content

Conversation

@cbuescher
Copy link
Member

We already have these tests in InternalAggregationTestCase to check random insertions into the response xContent so that we don't fail on future changes in the response format. This change adds the same to AggregationsTests and runs on a whole aggregations tree. Unfortunately we need to exclude many places in the xContent from random insertion, but I added a long comment trying to explaine those.

@cbuescher cbuescher added :Java High Level REST Client >test Issues or PRs that are addressing/adding tests v5.6.0 v6.0.0 labels Jun 26, 2017
@cbuescher cbuescher requested review from javanna and tlrx June 26, 2017 11:40
Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM, I left a very minor comment. Thanks @cbuescher

Copy link
Member

Choose a reason for hiding this comment

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

nit: can we add a small comment about what this method is doing? No need to go in details but just the overall logic and why we do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I added a comment.

Copy link
Member

@javanna javanna 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 @cbuescher

@cbuescher cbuescher force-pushed the addLenientParsing-AggregationsTests branch from d681199 to adef44a Compare June 27, 2017 11:19
@cbuescher cbuescher merged commit c55dc23 into elastic:master Jun 27, 2017
cbuescher added a commit that referenced this pull request Jun 27, 2017
We already have these tests in InternalAggregationTestCase to check random insertions into the response xContent so that we don't fail on future changes in the response format. This change adds the same to AggregationsTests and runs on a whole aggregations tree. Unfortunately we need to exclude many places in the xContent from random insertion, but I added a long comment trying to explaine those.
jasontedor added a commit to ywelsch/elasticsearch that referenced this pull request Jun 28, 2017
* master:
  Do not swallow exception when relocating
  Docs: Fix typo for request cache (elastic#25444)
  Remove implicit 32-bit support
  [DOCS] reworded to prevent code span rendering glitch (elastic#25442)
  Disallow multiple concurrent recovery attempts for same target shard (elastic#25428)
  Update global checkpoint when increasing primary term on replica (elastic#25422)
  Add backwards compatibility indices for 5.4.3
  Add version 5.4.3 after release
  Update MSI installer images (elastic#25414)
  Add missing newline at end of SetsTests.java
  Rename handoff primary context transport handler
  correct expected thrown exception in mappingMetaData to ElasticsearchParseException (elastic#25410)
  test: Make many percolator integration tests real integration tests
  [DOCS] Update docs to use shared attribute file (elastic#25403)
  Add Javadocs and tests for set difference methods
  Tests: Add parsing test for AggregationsTests (elastic#25396)
  test: get upgrade status for all indices
  Mute SignificantTermsAggregatorTests#testSignificance()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>test Issues or PRs that are addressing/adding tests v5.6.0 v6.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants