Skip to content

Conversation

@cbuescher
Copy link
Member

The parsing of the ranking evaluation request and its subcomponents should
detect unknown fields and throw parsing errors. This currently isn't testing so
this change adds those tests and changes the parser behaviour in cases where it
is needed.

@cbuescher
Copy link
Member Author

@javanna this should adress questions you had while reviewing #28357, maybe you can have a look at this tests?

The parsing of the ranking evaluation request and its subcomponents should
detect unknown fields and throw parsing errors. This currently isn't testing so
this change adds those tests and changes the parser behaviour in cases where it
is needed.
@cbuescher cbuescher force-pushed the test-rankeval-parsing-not-lenient branch from 7446959 to 93c56c5 Compare February 5, 2018 08:17
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

left a question, LGTM otherwise

}
}

public void testXContentParsingNotLenient() throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

is this testing that parsing is or is not lenient? I can't get that from the name ;) Is this part of a request or response?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is testing that adding random fields somewhere in the xContent throws an error. So its testing that parsing is not lenient. All these are part of the request, so I think thats the behaviour we want there, whereas we want to be lenient on the response side. I can rename all these test methods to e.g. testRequestParsingIsNotLenient, maybe add a short comment as well. Would that be better?

Copy link
Member

Choose a reason for hiding this comment

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

Adding the is verb is good enough on my end, thanks.

@cbuescher
Copy link
Member Author

@javanna thanks for the review, I pushed an update and will merge once CI is green

@cbuescher cbuescher merged commit 0179127 into elastic:master Feb 8, 2018
cbuescher pushed a commit that referenced this pull request Feb 8, 2018
Parsing of a ranking evaluation request and its subcomponents should throw parsing
errors on unknown fields. This change adds tests for this and changes the parser 
behaviour in cases where it is needed.
@jpountz jpountz added the >test Issues or PRs that are addressing/adding tests label Jun 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>test Issues or PRs that are addressing/adding tests v6.3.0 v7.0.0-beta1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants