Fix constand_keyword test run and properly test recent behavior change #117284
Conversation
lkts
added
>non-issue
auto-backport
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| module org.elasticsearch.xpack.constantkeyword { |
There was a problem hiding this comment.
I am not confident in this files contents.
There was a problem hiding this comment.
@rjernst is modularizing this plugin strictly necessary or is the services info file sufficient?
| @@ -1,7 +1,8 @@ | |||
| import org.elasticsearch.gradle.internal.info.BuildParams | |||
|
|
|||
| apply plugin: 'elasticsearch.internal-es-plugin' | |||
| apply plugin: 'elasticsearch.legacy-yaml-rest-test' | |||
| apply plugin: 'elasticsearch.internal-yaml-rest-test' | |||
| apply plugin: 'elasticsearch.internal-test-artifact' | |||
There was a problem hiding this comment.
The test artifact plugin shouldn't be required here. This is only necessarily if another project needs to consume the tests of this project as an artifact dependency.
| * License v3.0 only", or the "Server Side Public License, v 1". | ||
| */ | ||
|
|
||
| module org.elasticsearch.xpack.constantkeyword { |
There was a problem hiding this comment.
@rjernst is modularizing this plugin strictly necessary or is the services info file sufficient?
| import java.util.Set; | ||
|
|
||
| public class ConstantKeywordFeatures implements FeatureSpecification { | ||
| public static final NodeFeature SYNTHETIC_SOURCE_WRITE_FIX = new NodeFeature("constant_keyword.synthetic_source_write_fix"); |
There was a problem hiding this comment.
Can we add this to MapperFeatures instead? I think that should be okay, although the mapping type is in x-pack.
There was a problem hiding this comment.
We can i just thought this is cleaner.
There was a problem hiding this comment.
Because MapperFeatures can't depend on ConstantKeywordFieldMapper.
There was a problem hiding this comment.
I think we can define a NodeFeature in MapperFeatures class?
There was a problem hiding this comment.
But I am fine if we continue with this approach too.
|
@elasticmachine update branch |
It depends :) it would have been necessary (and sufficient) for tests that run un-modularized. Not 100% about rest tests; they run with a full cluster, so modular I think. |
This is a follow-up to #117182.
That PR didn't change yaml tests since they were not running and didn't introduce a test feature. Tests didn't run because legacy yaml rest tests don't provide tests features and the test was using previously introduced
mapper.source.mode_from_index_setting. As a result this test was always skipped.This PR introduces new test feature, fixes the test and migrates this project to proper yaml test runner.