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

Add specific test for serializing all mapping parameter values #61844

Merged
merged 2 commits into from
Sep 2, 2020

Conversation

romseygeek
Copy link
Contributor

This commit adds a test to MapperTestCase that explicitly checks that a mapper can
serialize all its default values, and that this serialization can then be re-parsed. Note that
the test is disabled for non-parametrized mappers as their serialization may in some cases
output parameters that are not accepted. Gradually moving all mappers to parametrized
form will address this.

The commit also contains a fix to keyword mappers, which were not correctly serializing
the similarity parameter; this partially addresses #61563

@romseygeek romseygeek added >non-issue >test Issues or PRs that are addressing/adding tests :Search Foundations/Mapping Index mappings, including merging and defining field types v8.0.0 v7.10.0 labels Sep 2, 2020
@romseygeek romseygeek requested a review from nik9000 September 2, 2020 11:22
@romseygeek romseygeek self-assigned this Sep 2, 2020
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Sep 2, 2020
@romseygeek
Copy link
Contributor Author

Marked as non-issue for the keyword mapper bugfix, which has not been released.


@Override
public void testMinimalToMaximal() {
// `include_defaults` includes unsupported properties in non-parametrized mappers
Copy link
Member

Choose a reason for hiding this comment

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

Could you assumeFalse here so it logs as skipped? I don't think that makes a big difference in the long run, but it makes me feel a little better about doing nothing in a test.

@romseygeek romseygeek merged commit 3269d1b into elastic:master Sep 2, 2020
@romseygeek romseygeek deleted the mapper/include_defaults branch September 2, 2020 19:03
romseygeek added a commit to romseygeek/elasticsearch that referenced this pull request Sep 2, 2020
…ic#61844)

This commit adds a test to MapperTestCase that explicitly checks that a mapper can
serialize all its default values, and that this serialization can then be re-parsed. Note that
the test is disabled for non-parametrized mappers as their serialization may in some cases
output parameters that are not accepted. Gradually moving all mappers to parametrized
form will address this.

The commit also contains a fix to keyword mappers, which were not correctly serializing
the similarity parameter; this partially addresses elastic#61563. It also enables `null` as a
value for `null_value` on `scaled_float`, as a follow-up to elastic#61798
romseygeek added a commit that referenced this pull request Sep 3, 2020
… (#61877)

This commit adds a test to MapperTestCase that explicitly checks that a mapper can
serialize all its default values, and that this serialization can then be re-parsed. Note that
the test is disabled for non-parametrized mappers as their serialization may in some cases
output parameters that are not accepted. Gradually moving all mappers to parametrized
form will address this.

The commit also contains a fix to keyword mappers, which were not correctly serializing
the similarity parameter; this partially addresses #61563. It also enables `null` as a
value for `null_value` on `scaled_float`, as a follow-up to #61798
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :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.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants