Skip to content

Migrate some more mapper test cases #61507

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

Merged
merged 2 commits into from
Aug 25, 2020

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Aug 24, 2020

Migrate some more mapper test cases from ESSingleNodeTestCase to
MapperTestCase.

Migrate some more mapper test cases from `ESSingleNodeTestCase` to
`MapperTestCase`.
@nik9000 nik9000 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.10.0 labels Aug 24, 2020
@nik9000 nik9000 requested a review from romseygeek August 24, 2020 22:05
@elasticmachine
Copy link
Collaborator

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

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Aug 24, 2020
Copy link
Contributor

@romseygeek romseygeek 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 @nik9000!

protected Collection<Class<? extends Plugin>> getPlugins() {
return pluginList(MapperExtrasPlugin.class);
protected boolean supportsMeta() {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A useful byproduct of this exercise is finding all the mappers that we've forgotten to implement meta on!


@Override
protected void metaMapping(XContentBuilder b) throws IOException {
// We serialize extra fields on top of the
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment looks truncated?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍


for (String field : new String[] { "a_field", "a_field._index_prefix", "a_field._2gram", "a_field._3gram"}) {
DocumentMapper mapper = createDocumentMapper(fieldMapping(this::minimalMapping));
ParsedDocument doc = mapper.parse(source(b -> b.field("field", "new york city")));
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ so much nicer

@@ -17,7 +17,6 @@
* under the License.
*/
apply plugin: 'elasticsearch.yaml-rest-test'
apply plugin: 'elasticsearch.internal-cluster-test'
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@@ -205,35 +222,39 @@ protected boolean supportsMeta() {
return true;
}

protected void metaMapping(XContentBuilder b) throws IOException {
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 a pain, and the really annoying thing is that it's hard to fix these inconsistencies because of BWC. I need to think some more about it.

@nik9000
Copy link
Member Author

nik9000 commented Aug 25, 2020

run elasticsearch-ci/packaging-sample-unix

@nik9000 nik9000 merged commit 4d37d14 into elastic:master Aug 25, 2020
nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Aug 25, 2020
Migrate some more mapper test cases from `ESSingleNodeTestCase` to
`MapperTestCase`.
nik9000 added a commit that referenced this pull request Aug 25, 2020
Migrate some more mapper test cases from `ESSingleNodeTestCase` to
`MapperTestCase`.
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.10.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants