Skip to content

[Rest Api Compatibility] transformations for keys in match and length #72156

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 16 commits into from
May 4, 2021

Conversation

pgomulka
Copy link
Contributor

@pgomulka pgomulka commented Apr 23, 2021

Additional yml tests transformations to modify a key in a match assertion and a key in a
length assertion.
The testing is done with the original and expected transformed files. This should make it a little more easy to write new tests and add new transformations.

Additional transformations to modify a key in a match and a key in a
length assertions
@pgomulka pgomulka added :Delivery/Build Build or test infrastructure v8.0.0 labels Apr 23, 2021
@pgomulka pgomulka self-assigned this Apr 23, 2021
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Apr 23, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@pgomulka pgomulka added >test Issues or PRs that are addressing/adding tests and removed >non-issue labels Apr 27, 2021
@pgomulka pgomulka requested review from jakelandis and joegallo April 27, 2021 10:53
@@ -21,18 +21,21 @@
*/
public class ReplaceMatch implements RestTestTransformByParentObject {
private final String replaceKey;
private final String newKeyName;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be exposed as an @Input. Adding a public getter for this field would also remove the need to override it in ReplaceKeyInMatch.

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

A few suggestions on class organization.

Collections.singletonList(new ReplaceLength("key.in_length_to_replace", "key.in_length_replaced", null))
);

AssertObjectNodes.areEqual(transformedTests, expectedTransformation);
Copy link
Contributor

Choose a reason for hiding this comment

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

This sooooo much more readable ! Thanks for addressing this.

this(replaceKey, replaceKey, replacementNode, testName);
}

public ReplaceMatch(String replaceKey, String newKeyName, JsonNode replacementNode, String testName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change the name of this class (and associated paramenters) to an abstract class ReplaceByKey with getKeyToFind() as an abstract method and then reintroduce ReplaceMatch as a child of this class that defines

  @Override
    @Internal
    public String getKeyToFind() {
        return "match";
    }

This would mean that ReplaceLength (which should be ReplaceKeyInLength) and ReplaceKeyInMatch would extend ReplaceByKey .

So...
RestTestTransformByParentObject -> ReplaceByKey -> [ ReplaceMatch, ReplaceKeyInMatch, ReplaceKeyInLength]

@pgomulka pgomulka requested a review from mark-vieira April 29, 2021 14:59
@pgomulka pgomulka requested a review from jakelandis April 29, 2021 14:59
Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

Changes look good! (one minor, non blocking nitpick request)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants