[Test] Use a common testing class for all XContent filtering tests #25491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We have two ways to filter XContent: - The first method is to parse the XContent as a map and use XContentMapValues.filter(). This method filters the content of the map using an automaton. It is used for source filtering, both at search and indexing time. It performs well but can generate a lot of objects and garbage collections when large XContent are filtered. It also returns empty objects (see f2710c1) when all the sub fields have been filtered out and handle dots in field names as if they were sub fields. - The second method is to parse the XContent and copy the XContentParser structure to a XContentBuilder initialized with includes/excludes filters. This method uses the Jackson streaming filter feature. It is used by the Response Filtering ('filter_path') feature. It does not generate a lot of objects, and does not return empty objects and also does not handle dots in field names explicitely. Both methods have similar goals but different tests. This commit changes the current XContentBuilder test class so that it becomes a more generic testing class and we can now ensure that filtering methods generate the same results. It also removes some tests from the XContentMapValuesTests class that should be in XContentParserTests.
tlrx
added
:Core/Infra/Core
nik9000
approved these changes
Jun 30, 2017
There was a problem hiding this comment.
This makes sense to me. I like the idea of unifying the tests for the things that function the same way and calling out the unique tests for the unique behavior differences. I only scanned the common tests to make sure that they made sense. If you want a more deep look at them let me know, but if you are confident in them then I'm be happy for you to merge.
| return XContentBuilder.builder(getXContentType().xContent()); | ||
| } | ||
| /** | ||
| * Tests for {@link org.elasticsearch.common.xcontent.XContent} filtering. |
There was a problem hiding this comment.
I think it is fine to import XContent just for Javadoc if you like.
tlrx
added a commit
that referenced
this pull request
Jul 3, 2017
…25491) We have two ways to filter XContent: - The first method is to parse the XContent as a map and use XContentMapValues.filter(). This method filters the content of the map using an automaton. It is used for source filtering, both at search and indexing time. It performs well but can generate a lot of objects and garbage collections when large XContent are filtered. It also returns empty objects (see f2710c1) when all the sub fields have been filtered out and handle dots in field names as if they were sub fields. - The second method is to parse the XContent and copy the XContentParser structure to a XContentBuilder initialized with includes/excludes filters. This method uses the Jackson streaming filter feature. It is used by the Response Filtering ('filter_path') feature. It does not generate a lot of objects, and does not return empty objects and also does not handle dots in field names explicitely. Both methods have similar goals but different tests. This commit changes the current XContentBuilder test class so that it becomes a more generic testing class and we can now ensure that filtering methods generate the same results. It also removes some tests from the XContentMapValuesTests class that should be in XContentParserTests.
|
Thanks @nik9000 |
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Jul 4, 2017
* master: (52 commits) Include shared/attributes.asciidoc from docs master Fixed page breaks for ICU Collation Keyword Fields Remove QueryParseContext (elastic#25486) [Test] Use a common testing class for all XContent filtering tests (elastic#25491) Tests fix - Significant terms/text aggs (elastic#25499) [DOCS] add docs for REST high level client index method (elastic#25501) Tests: Add Debian 9 (Stretch) to the packaging tests test: Run flush before upgrade and refresh after upgrade. Fix third party audit for repository-hdfs [TEST] Expect nodes getting disconnected quickly testPrimaryFailureIncreasesTerm should use assertBusy to wait for yellow Cleanup network / transport related settings (elastic#25489) Fix repository-hdfs plugin packaging test Remove allocation id from replica replication response (elastic#25488) Adjust BWC version on bad allocation request test Upgrading HDFS Repository Plugin to use HDFS 2.8.1 Client (elastic#25497) Adjust status on bad allocation explain requests Preliminary support for ARM Add doc note regarding explicit publish host Fix typo in name of test ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have two ways to filter XContent:
The first method is to parse the XContent as a map and use
XContentMapValues.filter(). This method filters the content of the mapusing an automaton. It is used for source filtering, both at search and
indexing time. It performs well but can generate a lot of objects and
garbage collections when large XContent are filtered. It also returns
empty objects (see f2710c1) when all
the sub fields have been filtered out and handle dots in field names as
if they were sub fields.
The second method is to parse the XContent and copy the XContentParser
structure to a XContentBuilder initialized with includes/excludes
filters. This method uses the Jackson streaming filter feature. It is
used by the Response Filtering ('filter_path') feature. It does not
generate a lot of objects, and does not return empty objects and also
does not handle dots in field names explicitly.
Both methods have similar goals but different tests. This commit changes
the current
AbstractFilteringJsonGeneratorTestCaseclass so that it teststhe expected behavior when filtering XContent to ensure that filtering methods
generate the same results. Custom behaviors are still tested in specific classes.
It also removes some tests from the
XContentMapValuesTestsclass thatshould be in
XContentParserTests.