[Rest Api Compatibility] Add transformation for simple text replaces#71118
[Rest Api Compatibility] Add transformation for simple text replaces#71118pgomulka merged 7 commits intoelastic:masterfrom
Conversation
This commits adds a simple textual replace transformation for Rest Api Compatibility testing
|
Pinging @elastic/es-delivery (Team:Delivery) |
jakelandis
left a comment
jakelandis
left a comment
There was a problem hiding this comment.
Looking good just a few minor requests.
| return null; | ||
| } | ||
|
|
||
| default boolean matches(JsonNode child) { |
| /** | ||
| * A transformation to replace the flat textual fields. | ||
| */ | ||
| public class ReplaceTextual implements RestTestTransformByParentObject { |
There was a problem hiding this comment.
we can certainly make it abstract, but why?
All the difference will be a constructor parameter, the implementation stays the same.
There was a problem hiding this comment.
It stems from a minor preference to introduce ReplaceIsTrue and ReplaceIsFalse (I thought I saw before, but must be mis-remebering) , where they are very small subclasses that define a hardcoded keyname. I think this helps abit with readability especially since the number of valid key names is very small. It also prevents someone using an invalid key name (invalid in the sense that is not supported by the testing framework).
There was a problem hiding this comment.
I am not too sure this will help much. I am actually worried that this will lead to class explosion - new class for each keyword.
For the time being, I will change the scope of ReplaceTextual to package (for testing) and create ReplaceIsTrue/False. I think we should to revisit and refactor this back once we have more keywords to support.
There was a problem hiding this comment.
worried that this will lead to class explosion
I am not too worried about it. There are only a small handful of keys, here are the top level: https://github.com/elastic/elasticsearch/blob/master/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/ExecutableSection.java#L26 and then a few more within those top level ones.
I think we should to revisit and refactor this back once we have more keywords to support.
Agreed, assuming we can avoid contract by String... something like an enum of known keys or the like as part of a contract.
| transformations.add(new ReplaceMatch(subKey, MAPPER.convertValue(value, JsonNode.class))); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Can you add a Gradle test for these 2 to YamlRestCompatTestPluginFuncTest / "transform task executes and works as configured" ?
When updating these just add the task configuration to the build file, the thing you want to change to the test.yml. The test should fail and output the result. In the past I just copy/paste that output and ensure that the diff looks correct (e.g. the only thing that changed is what is expected).
| @@ -0,0 +1,108 @@ | |||
| --- | |||
There was a problem hiding this comment.
Rather then using the match folder, can you create a new folder "text_replace" (or the like).
Also, it might be good to remove some the the "match" for readbility and add test for something like "[key/value]_not_to_replace".
There was a problem hiding this comment.
will do. I will also need to tweak the test to be more rigid - I missed a case where a key can be key_to_replace, but value is not to be replace. The impl was correct, just a test was not asserting about this.
I have a feeling that we will have to revisit these tests and simplify/make them more readable.
|
|
||
| @Override | ||
| protected boolean getHumanDebug() { | ||
| return true; |
There was a problem hiding this comment.
can you flip to false before committing.
jakelandis
left a comment
jakelandis
left a comment
There was a problem hiding this comment.
LGTM, thanks for the iterations.
3 participants
This commits adds a simple textual replace transformation for Rest Api Compatibility testing.
It allows to replace simple key-value pairs. For instance used in this PR to replace
is_trueandis_false.is_true: old_valuewithis_true: new_value