Skip to content
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

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

Closed
honzakral opened this issue Jul 15, 2017 · 5 comments
Closed

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

honzakral opened this issue Jul 15, 2017 · 5 comments
Labels
>enhancement help wanted adoptme >test Issues or PRs that are addressing/adding tests

Comments

@honzakral
Copy link
Contributor

In (0) you can see a test that uses lang instead of id to get around the bug fixed by #25736

This test, however, should be failing since it doesn't specify both lang and id which are marked as required in the API spec. This suggests that either the test is not being run as it should or that the test runner doesn't check for all required attributes.

0 - https://github.com/elastic/elasticsearch/blob/5.x/modules/lang-painless/src/test/resources/rest-api-spec/test/painless/16_update2.yaml#L7-L9

@honzakral honzakral added the >test Issues or PRs that are addressing/adding tests label Jul 15, 2017
@javanna
Copy link
Member

javanna commented Sep 25, 2017

The test is run, but the java yaml test runner doesn't check whether required parameters are specified or not. That unfortunately allows for writing tests that may not work when run from other runners. Marking adoptme.

@olcbean
Copy link
Contributor

olcbean commented Oct 7, 2017

Hey @javanna, can I give this one a try if you are not working on it? my thinking is to enforce not only the params but also the required parts.

@javanna
Copy link
Member

javanna commented Oct 9, 2017

go for it @olcbean , thanks!

@olcbean
Copy link
Contributor

olcbean commented Oct 27, 2017

@honzakral the test is correct but unfortunately the rest-api-spec for put_script in 5.6 contains a minor mistake : id should have been "required" : false (see #26923 for more details; the same applies to delete_script and get_script).

#27035 makes sure that if a url parameter is specified as required but it is not provided, then the test fails.

@honzakral @javanna is there anything else that needs to be done for this issue?
(The mentioned issues are fixed for 6.0+)

@javanna
Copy link
Member

javanna commented Oct 31, 2017

I think we are good @olcbean , closing! Thanks a lot for your contributions!

@javanna javanna closed this as completed Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement help wanted adoptme >test Issues or PRs that are addressing/adding tests
Projects
None yet
Development

No branches or pull requests

3 participants