Make yaml test runner stricter by enforcing `required` for paths and parameters #27035

olcbean · 2017-10-17T13:21:55Z

Till now the yaml test runner was verifying that the provided path parts and parameters are supported.
With this PR, yaml test runner also checks that all required path parts and parameters are provided.

Related to #25737

elasticmachine · 2017-10-17T13:21:58Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

s1monw · 2017-10-17T17:18:06Z

@elasticmachine ok to test

nik9000

Thanks for making this change!

I left a few minor nick picky things. The biggest one is error handling in the parsing. I know we haven't been great about error handling in the yaml parsing of this stuff before but I'd like us to be better.

nik9000 · 2017-10-17T20:34:01Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java

@@ -80,19 +83,33 @@ public ClientYamlTestResponse callApi(String apiName, Map<String, String> params
        //divide params between ones that go within query string and ones that go within path
        Map<String, String> pathParts = new HashMap<>();
        Map<String, String> queryStringParams = new HashMap<>();
+
+        List<String> apiPathParts = restApi.getPathParts().entrySet().stream().filter(e -> e.getValue() == true).map(Entry::getKey)


Maybe Set<String> instead? The list should be short so it likely doesn't matter but it feels better to me if it is a set.

nik9000 · 2017-10-17T20:34:27Z

...ramework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApi.java

        return pathParts;
    }

-    void addPathPart(String pathPart) {
-        this.pathParts.add(pathPart);
+    void addPathPart(String pathPart, Boolean required) {


I think it should be bool rather than Boolean.

nik9000 · 2017-10-17T20:34:44Z

...ramework/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApi.java

@@ -98,20 +99,20 @@ void addPath(String path) {
        this.paths.add(path);
    }

-    public List<String> getPathParts() {
+    public Map<String, Boolean> getPathParts() {


Probably should have javadoc because it isn't obvious what the boolean reads unless you read it as part of this PR.

nik9000 · 2017-10-17T20:37:13Z

...rk/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java

+                if ("required".equals(parser.currentName())) {
+                    parser.nextToken();
+                    required = parser.booleanValue();
+                }


Can you either replace this with CosntructingObjectParser<Boolean> or manually throw IOExceptions for things other than required and for tokens other than FIELD_NAME?

@nik9000 thanks for the hint! (I did not know of this parser)
I think that CosntructingObjectParser<Boolean> cannot be used here as the required field is an optional one.

I couldn't find an example how to use ConstructingObjectParser for the primitive wrapper classes. Do you have one?

nik9000 · 2017-10-17T20:38:07Z

...c/test/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParserTests.java

-        assertThat(restApi.getPathParts().get(1), equalTo("index"));
-        assertThat(restApi.getPathParts().get(2), equalTo("type"));
+        assertThat(restApi.getPathParts().keySet(), containsInAnyOrder("id","index", "type"));
+        assertThat(restApi.getPathParts().get("index"), equalTo(true));


In cases like this I find that

assertThat(restApi.getPathParts(), hasEntry("index", true));

produces more readable error messages.

olcbean · 2017-10-19T19:40:26Z

@nik9000 thanks for the review! Can you have another look?

nik9000

I agree about the object parser being a bit painful here. But it does get you all that error checking for free which is pretty great. I think you've got it going the right direction though.

I left a minor thing. Otherwise I think this is good to go!

nik9000 · 2017-10-19T20:38:20Z

...rk/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java

+    }
+
+    private boolean getRequired(XContentParser parser) throws IOException {
+        ObjectParser<YamlElement, Void> objParser = new ObjectParser<>("", true, YamlElement::new);


We tend to make these static.

nik9000 · 2017-10-19T20:39:18Z

...rk/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java

@@ -146,4 +147,20 @@ public ClientYamlSuiteRestApi parse(String location, XContentParser parser) thro

        return restApi;
    }
+
+    class YamlElement {


I think it'd be more reasonable to name this Parameter or something.

olcbean · 2017-10-19T21:06:08Z

@nik9000 ready for another round ;)

nik9000

Thanks for being patient with me @olcbean! I left one last thing. I just hadn't communicated clearly enough what I wanted.

nik9000 · 2017-10-20T13:48:19Z

...rk/src/main/java/org/elasticsearch/test/rest/yaml/restspec/ClientYamlSuiteRestApiParser.java

+    }
+
+    private static boolean getRequired(XContentParser parser) throws IOException {
+        ObjectParser<Parameter, Void> objParser = new ObjectParser<>("", true, Parameter::new);


Ah! More like:

private static final ObjectParser<Parameter, Void> PARAMETER_PARSER = new ObjectParser<>("parameter", true, Parameter:true); static { PARAMETER_PARSER.declareBoolean(Parameter::setRequired, new ParseField("required"); }

And they you can call it like PARAMETER_PARSER.parse(parser, null).isRequired() like you have. That way we reuse the parser. It probably isn't a big deal, but it is what we do everywhere else so we may as well do it here.

nik9000 · 2017-10-20T13:53:19Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ClientYamlTestClient.java

            }
        }

+        if (!apiRequiredPathParts.isEmpty()) {


Funny story: we had a contributor for a long time that always wanted false == blah rather than !blah because, he claimed, it is harder to miss the false == part then the ! part. I kind of agree with him but I think we were in the minority. But that is why you have both patterns all over our code base. Either way is fine.

Hope you don't mind that I change all occurrences of ! to false == (consistency on file level)

olcbean · 2017-10-20T15:01:40Z

@nik9000 thanks for your patience!
I just made the changes you suggested above.

nik9000

Looks good to me. I'm going to run it through the tests locally and merge it when it passes.

javanna · 2017-10-23T08:31:29Z

cool stuff, thanks @olcbean LGTM as well.

…parameters (#27035) Till now the yaml test runner was verifying that the provided path parts and parameters are supported. With this PR, yaml test runner also checks that all required path parts and parameters are provided.

nik9000 · 2017-10-25T21:03:32Z

I've (finally) merge this to master and 6.x.

enforce 'required' in yaml test runner for paths and parameters

83560ea

nik9000 reviewed Oct 17, 2017

View reviewed changes

addressing the reviewers comments

30ecc4f

nik9000 requested changes Oct 19, 2017

View reviewed changes

renaming inner class

744b2fa

nik9000 requested changes Oct 20, 2017

View reviewed changes

ObjectParser set to static

94684c2

nik9000 approved these changes Oct 20, 2017

View reviewed changes

javanna added >test Issues or PRs that are addressing/adding tests v7.0.0 v6.1.0 labels Oct 25, 2017

nik9000 merged commit 981b7f4 into elastic:master Oct 25, 2017

olcbean mentioned this pull request Oct 27, 2017

Rest Tests for Painless do not respect the API spec #25737

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make yaml test runner stricter by enforcing `required` for paths and parameters #27035

Make yaml test runner stricter by enforcing `required` for paths and parameters #27035

olcbean commented Oct 17, 2017

elasticmachine commented Oct 17, 2017

s1monw commented Oct 17, 2017

nik9000 left a comment

nik9000 Oct 17, 2017

nik9000 Oct 17, 2017

nik9000 Oct 17, 2017

nik9000 Oct 17, 2017

olcbean Oct 19, 2017

nik9000 Oct 17, 2017

olcbean commented Oct 19, 2017

nik9000 left a comment

nik9000 Oct 19, 2017

nik9000 Oct 19, 2017

olcbean commented Oct 19, 2017

nik9000 left a comment

nik9000 Oct 20, 2017

nik9000 Oct 20, 2017

olcbean Oct 20, 2017

olcbean commented Oct 20, 2017 •

edited

Loading

nik9000 left a comment

javanna commented Oct 23, 2017

nik9000 commented Oct 25, 2017

Make yaml test runner stricter by enforcing required for paths and parameters #27035

Make yaml test runner stricter by enforcing required for paths and parameters #27035

Conversation

olcbean commented Oct 17, 2017

elasticmachine commented Oct 17, 2017

s1monw commented Oct 17, 2017

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Oct 19, 2017

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Oct 19, 2017

nik9000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olcbean commented Oct 20, 2017 • edited Loading

nik9000 left a comment

Choose a reason for hiding this comment

javanna commented Oct 23, 2017

nik9000 commented Oct 25, 2017

Make yaml test runner stricter by enforcing `required` for paths and parameters #27035

Make yaml test runner stricter by enforcing `required` for paths and parameters #27035

olcbean commented Oct 20, 2017 •

edited

Loading