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 test fixes #27354

Merged
merged 15 commits into from Nov 14, 2017

Conversation

Projects
None yet
7 participants
@clintongormley
Copy link
Member

commented Nov 12, 2017

These are various fixes I had to make to get Perl running with the rest tests.

  • Adjust percentile tests to work with Perl number handling
  • Fix bad YAML in search/110_field_collapsing.yml
  • Force dummy scripts to be strings, not numbers
  • Fixed bad YAML comments
  • Removed unneeded "headers" from bulk/10_basic.yml
  • Rename remote.info to cluster.remote_info
  • Rename render_search_template to indices.render_search_template
  • Rename ingest.processor.grok to ingest.processor_grok
@karmi

This comment has been minimized.

Copy link
Member

commented Nov 13, 2017

Thanks for looking into that, @clintongormley! I've run the Ruby test suite against the patch and I'm getting more failures than on current master :), I'll investigate more in the next couple of days.

"95.0": 143.49999999999997
"99.0": 149.5

# - match:

This comment has been minimized.

Copy link
@karmi

karmi Nov 13, 2017

Member

The fixes made by @fxdgear in #26905, and my subsequent fixes in #27355 worked nicely for me in Ruby. I must admit I like more the original notation. Also, some tests in the proposed notation (aggregations.percentiles_int.values.5\.0) fail for me, not sure how the Ruby runner handles it, must debug...

This comment has been minimized.

Copy link
@clintongormley

clintongormley Nov 13, 2017

Author Member

The test runners are supposed to support dots escaped with \.

The original notation is a non-starter for me, for instance Perl gets back 143.5 and keeps it as that value, rather than converting to 143.49999999999997. But then it sees 1.0 in the response and reads that as 1(then the test fails when it tries to compare it to 1.0).

I can do numeric comparisons when there is a single numeric-like value on the right, but not with an object.

This comment has been minimized.

Copy link
@clintongormley

clintongormley Nov 13, 2017

Author Member

What I could do (keeping the key-to-numeric-value format) is allow for approximate numeric matching (ie within a certain tolerance) so that I can remove the lte/gte

This comment has been minimized.

Copy link
@polyfractal

polyfractal Nov 13, 2017

Member

I'm thinking we may just want to remove the values like 143.49999999999997. I didn't think about it at the time when I added the test, but these are likely subject to how each language interprets json floats and internal float vs double, etc.

Would simplify everyone's life if we stuck to nice "discrete" floats (149.5, etc). The point of these REST tests are to just do rough validation of the body, not really the values, so I think it's fine if we omit the problematic percentiles.

@Mpdreamz
Copy link
Member

left a comment

Other than a small nitpick LGTM

@@ -1,5 +1,5 @@
{
"render_search_template": {
"indices.render_search_template": {

This comment has been minimized.

Copy link
@Mpdreamz

Mpdreamz Nov 13, 2017

Member

But nit picky perhaps but I think this actually makes more sense unnamespaced like other search related API's. Do not feel strongly about this though.

This comment has been minimized.

Copy link
@clintongormley

clintongormley Nov 13, 2017

Author Member

Thanks, you're right - don't know what i was thinking


- do:
catch: /Malformed action\/metadata line \[3\], expected FIELD_NAME but found \[END_OBJECT\]/
headers:
Content-Type: application/json

This comment has been minimized.

Copy link
@javanna

javanna Nov 13, 2017

Member

why did you remove these? I'm afraid they were there for a reason. This may work for clients without the header as they always use json content-type anyway, but providing the header disables the content-type randomization that the java yaml test runner usually does.

This comment has been minimized.

Copy link
@clintongormley

clintongormley Nov 13, 2017

Author Member

OK - I can revert this and add a features skip clause instead. The problem was that it had headers, but hadn't declared the need for the headers feature.

This comment has been minimized.

Copy link
@clintongormley

clintongormley Nov 13, 2017

Author Member

Ah, I see that the headers feature has already been added, but it's impossible to ignore without also adding parsing to skip it... which i shall duly do (and revert this change)

This comment has been minimized.

Copy link
@javanna

javanna Nov 13, 2017

Member

you mean that you would expect two skip clauses here? or the reason also highlighting why the headers skip?

This comment has been minimized.

Copy link
@javanna

javanna Nov 13, 2017

Member

ok sorry we were kind of typing at the same time, ignore my last message ;)

This comment has been minimized.

Copy link
@karmi

karmi Nov 13, 2017

Member

I think I've fixed this in #26896, but I might have forgot to cherry-pick that commit into the 6.0 branch, which I did yesterday.

@@ -158,6 +158,6 @@ teardown:
---
"Test Grok Patterns Retrieval":
- do:
ingest.processor.grok: {}
ingest.processor_grok: {}

This comment has been minimized.

Copy link
@javanna

javanna Nov 13, 2017

Member

wondering if you were the first one ever running these tests...

This comment has been minimized.

Copy link
@javanna

javanna Nov 13, 2017

Member

I think I don't follow this change, why the underscore instead of the dot? the API is called ingest.processor.grok in the spec right? This should fail the build I think.

This comment has been minimized.

Copy link
@clintongormley

clintongormley Nov 13, 2017

Author Member

I renamed ingest.processor.grok to ingest.processor_grok in the spec too, so this should work. The reason for the underscore instead of the dot is that an underscore returns a new namespace in the REST clients, but we have only one thing in this namespace, so makes more sense just to have it at the same level as the other ingest methods.

This comment has been minimized.

Copy link
@javanna

javanna Nov 13, 2017

Member

Yes, I see that now. And the name of the api didn't match the name of the file. All good. I am going to open a PR to validate that the name of the api matches the filename, I've seen enough of these errors.

@clintongormley

This comment has been minimized.

Copy link
Member Author

commented Nov 13, 2017

jenkins test this

@clintongormley

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2017

thanks @Mpdreamz @javanna @karmi for the reviews. merging

@clintongormley clintongormley merged commit c4e8597 into 6.0 Nov 14, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

clintongormley added a commit that referenced this pull request Nov 14, 2017

Rest test fixes (#27354)
* REST: Rename ingest.processor.grok to ingest.processor_grok
* REST: Rename remote.info to cluster.remote_info
* REST: Fixed bad YAML comments
* REST: Force dummy scripts to be strings, not numbers
* REST: Fix bad YAML in search/110_field_collapsing.yml
* REST: Adjust percentile tests to work with Perl number handling

clintongormley added a commit that referenced this pull request Nov 14, 2017

Rest test fixes (#27354)
* REST: Rename ingest.processor.grok to ingest.processor_grok
* REST: Rename remote.info to cluster.remote_info
* REST: Fixed bad YAML comments
* REST: Force dummy scripts to be strings, not numbers
* REST: Fix bad YAML in search/110_field_collapsing.yml
* REST: Adjust percentile tests to work with Perl number handling

@clintongormley clintongormley added v6.0.0 and removed v6.0.0-alpha1 labels Nov 14, 2017

@clintongormley clintongormley deleted the rest_spec branch Nov 14, 2017

martijnvg added a commit that referenced this pull request Nov 15, 2017

Merge remote-tracking branch 'es/master' into ccr
* es/master:
  Logging: Unify log rotation for index/search slow log (#27298)
  wildcard query on _index (#27334)
  REST spec: Validate that api name matches file name that contains it (#27366)
  Revert "Reduce synchronization on field data cache"
  Create new handlers for every new request in GoogleCloudStorageService (#27339)
  Rest test fixes (#27354)

martijnvg added a commit that referenced this pull request Nov 15, 2017

Merge remote-tracking branch 'es/6.x' into ccr-6.x
* es/6.x:
  Updated 6.0.0 release notes to include all changes from alpha, beta, RC
  wildcard query on _index (#27334)
  REST spec: Validate that api name matches file name that contains it (#27366)
  Revert "Reduce synchronization on field data cache"
  Create new handlers for every new request in GoogleCloudStorageService (#27339)
  Rest test fixes (#27354)

@lcawl lcawl removed the v6.1.0 label Dec 12, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.