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

[DOCS] Test examples in Graph API #31447

Closed
wants to merge 4 commits into from
Closed

Conversation

lcawl
Copy link
Contributor

@lcawl lcawl commented Jun 19, 2018

Related to #30665

This PR enables testing of the examples in the Graph API reference.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search-aggs

@lcawl lcawl requested a review from markharwood June 20, 2018 00:00
type: keyword
query_time:
type: date
'''
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs some data adding in here?

@@ -204,6 +208,7 @@ POST clicklogs/_xpack/graph/_explore
}
--------------------------------------------------
// CONSOLE
// TEST[setup:clicklogs_index]
<1> Seed the exploration with a query. This example is searching
clicklogs for people who searched for the term "midi".
<2> Identify the vertices to include in the graph. This example is looking for
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this query refers to a "query.raw" which is a field not likely to be in the clicklogs index mapping as it is currently defined in the build.gradle file

@markharwood
Copy link
Contributor

I saw a couple of things in the setup - missing data and fields

@@ -56,7 +56,7 @@ Elasticsearch query. For example:
}
}
--------------------------------------------------

// NOTCONSOLE
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd be much better off writing a real request with just the query part in it or something like that. We had a lot of "unrooted" JSON in our docs before we had the docs tests and removing it all made the docs a ton more clear. No more wondering "where does this go"? Also, the way this is it doesn't get any test coverage.

Copy link
Contributor

Choose a reason for hiding this comment

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

I recall the response parts were tricky to test given the scores produced and how they might change with Lucene versions etc. and the difficulty involved in masking them from any "actual vs expected" result comparisons.

Copy link
Member

Choose a reason for hiding this comment

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

It is possible to ignore the numbers with tricks like // TESTRESPONSE[s/12.3/$body.$_path/].

@colings86
Copy link
Contributor

@markharwood @lcawl Is this still something we want to do? If so what do we need to do to progress it?

@markharwood
Copy link
Contributor

If so what do we need to do to progress it?

I can take a closer look at Nik's suggestion for masking scores but the elements returned are also liable to change order too. I'm not sure the path expressions for string replacements would be expressive enough to cope with this.
The challenge here was always the reliance on the doc framework's strict string-based comparisons of expected vs actual on fully serialized results. Robust tests are hard to configure when trying to patch the variations in result strings

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants