Clean up semantic text field mapper tests#127853
Conversation
Mikep86
added
>non-issue
auto-backport
elasticsearchmachine
added
Team:SearchOrg
|
Pinging @elastic/search-eng (Team:SearchOrg) |
|
Pinging @elastic/search-relevance (Team:Search - Relevance) |
Mikep86
added
>test
kderusso
left a comment
kderusso
left a comment
There was a problem hiding this comment.
LGTM, thanks for cleaning this up. Left one nitpick, non-blocking.
| .build(); | ||
| assertTrue(InferenceMetadataFieldsMapper.isEnabled(settings)); | ||
|
|
||
| // Test that index.mapping.semantic_text.use_legacy_format == false is ignored when the index version is too old to support the new |
There was a problem hiding this comment.
Out of the scope of this PR: Is it technically correct that we don't error in this case, since we're requesting a format that can't be applied?
There was a problem hiding this comment.
Perhaps not, but it's a moot point now as erroring out on old index versions could be a breaking change. Add into that that this is an index setting that should not be used externally and it just doesn't seem worth following up on.
| IndexVersionUtils.randomVersionBetween( | ||
| random(), | ||
| IndexVersions.SEMANTIC_TEXT_FIELD_TYPE, | ||
| IndexVersionUtils.getPreviousVersion(IndexVersions.INFERENCE_METADATA_FIELDS_BACKPORT) |
There was a problem hiding this comment.
Nitpick: We're starting to get a little confusing with index versions as the names aren't always clear what the change was. Would it make sense to do a favor to our future selves and future maintainers, since we're cleaning up anyway, to create private members to use for testing? e.g.
private static final IndexVersion INDEX_VERSION_THAT_WE_INTRODUCED_NEW_FORMAT = ...
...
Naming for illustrative purposes only, I'm sure there are better ones, but you get the drift 😁
There was a problem hiding this comment.
I made the comments more descriptive here, I think that has the same effect. WDYT?
There was a problem hiding this comment.
I still think readability would be enhanced by better names, but the comments are better, thanks for adding them
| } else { | ||
| return createMapperService(settings, mappings); | ||
| } | ||
| return createMapperService(indexVersion, settings, mappings); |
|
@elasticmachine update branch |
💔 Backport failed
You can use sqren/backport to manually backport by running |
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
(cherry picked from commit 9e542c0) # Conflicts: # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticInferenceMetadataFieldsMapperTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java
* Clean up semantic text field mapper tests (#127853) (cherry picked from commit 9e542c0) # Conflicts: # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticInferenceMetadataFieldsMapperTests.java # x-pack/plugin/inference/src/test/java/org/elasticsearch/xpack/inference/mapper/SemanticTextFieldMapperTests.java * Fix getRandomCompatibleIndexVersion
5 participants
Fix issues identified in #126629:
testIsEnabledand remove code that was necessary for those tests to pass