Skip to content
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

Explicitly say if stored fields aren't supported in MapperTestCase #72474

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

romseygeek
Copy link
Contributor

MapperTestCase has a check that if a field mapper supports stored fields,
those stored fields are available to index time scripts. Many of our mappers
do not support stored fields, and we try and catch this with an assumeFalse
so that those mappers do not run this test. However, this test is fragile - it
does not work for mappers created with an index version below 8.0, and it
misses mappers that always store their values, e.g. match_only_text.

This commit adds a new supportsStoredField method to MapperTestCase,
and overrides it for those mappers that do not support storing values. It
also adds a minimalStoredMapping method that defaults to the minimal
mapping plus a store parameter, which is overridden by match_only_text
because storing is not configurable and always available on this mapper.

@romseygeek romseygeek added >test Issues or PRs that are addressing/adding tests :Search Foundations/Mapping Index mappings, including merging and defining field types v8.0.0 v7.14.0 labels Apr 29, 2021
@romseygeek romseygeek requested a review from iverase April 29, 2021 13:27
@romseygeek romseygeek self-assigned this Apr 29, 2021
@elasticmachine elasticmachine added the Team:Search Meta label for search team label Apr 29, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

}
assumeTrue("Field type does not support stored fields", supportsStoredFields());
MapperService mapperService = createMapperService(fieldMapping(this::minimalStoreMapping));
assertParseMinimalWarnings();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is much nicer :)

Copy link
Contributor

@iverase iverase left a comment

Choose a reason for hiding this comment

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

lgtm

@romseygeek romseygeek merged commit 009f23e into elastic:master Apr 30, 2021
@romseygeek romseygeek deleted the mapper/stored-field-tests branch April 30, 2021 08:00
romseygeek added a commit that referenced this pull request Apr 30, 2021
…72474)

MapperTestCase has a check that if a field mapper supports stored fields,
those stored fields are available to index time scripts. Many of our mappers
do not support stored fields, and we try and catch this with an assumeFalse
so that those mappers do not run this test. However, this test is fragile - it
does not work for mappers created with an index version below 8.0, and it
misses mappers that always store their values, e.g. match_only_text.

This commit adds a new supportsStoredField method to MapperTestCase,
and overrides it for those mappers that do not support storing values. It
also adds a minimalStoredMapping method that defaults to the minimal
mapping plus a store parameter, which is overridden by match_only_text
because storing is not configurable and always available on this mapper.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team >test Issues or PRs that are addressing/adding tests v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants