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] Omit shard failures assertion for incompatible responses #31430

Merged
merged 3 commits into from
Jun 20, 2018

Conversation

davidkyle
Copy link
Member

@davidkyle davidkyle commented Jun 19, 2018

The failures in the docs snippet testing seen in #31339 comment are due to the framework inserting the assertion is_false: _shards.failures which causes errors if the response is not a JSON object.

The ml datafeed preview API does not return a JSON object and is excluded from the check.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-docs

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

LGTM

@davidkyle
Copy link
Member Author

@elasticmachine test this please

@davidkyle
Copy link
Member Author

@nik9000 I pushed a second commit updating the regex to exclude API calls on an index such as index_name/_search etc.

@nik9000
Copy link
Member

nik9000 commented Jun 19, 2018

So now that I look a little more closely, I think we want this for things like searches. So I think the name of the regex is wrong. It should match /_search, /foo/_search, and /foo/bar/_search. Hmmmm. Maybe we should have a list of API that we don't generate this for.... Because so many of them could benefit from this assertion and most of those that don't benefit just have the assertion as a noop.

@davidkyle
Copy link
Member Author

And also _validate, _explain, _msearch etc

Yes it is a bit of a pickle and it's tricky to track as there isn't a single class for the response header BroadcastResponse and ReplicationResponse both write a similar _shards object that is a candidate for refactoring.

I'm afraid we are creating a maintenance burden whitelisting known APIs perhaps we should simply exclude paths starting with _cat or _xpack

@nik9000
Copy link
Member

nik9000 commented Jun 19, 2018

I'm afraid we are creating a maintenance burden whitelisting known APIs perhaps we should simply exclude paths starting with _cat or _xpack

I'd expluce _cat and the ml API that it doesn't work for and keep it for all of the others.

@davidkyle davidkyle changed the title [DOCS] Only assert no shard failures for doc write operations [DOCS] Omit shard failures assertion for incompatible responses Jun 19, 2018
Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

Please change the name of the test and merge when ready!

@@ -47,4 +45,10 @@ class RestTestFromSnippetsTaskTest extends GroovyTestCase {
assertEquals("\"foo\": \"bort\\n baz\"",
replaceBlockQuote("\"foo\": \"\"\"bort\n baz\"\"\""));
}

void testIsDocWriteRequest() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you change the name of the test?

@davidkyle davidkyle merged commit 6ebe6e3 into elastic:master Jun 20, 2018
davidkyle added a commit that referenced this pull request Jun 20, 2018
Filter out the assertion for _cat and _xpack/ml/datafeed APIs
davidkyle added a commit that referenced this pull request Jun 20, 2018
Filter out the assertion for _cat and _xpack/ml/datafeed APIs
dnhatn added a commit that referenced this pull request Jun 20, 2018
* 6.x:
  [DOCS] Omit shard failures assertion for incompatible responses  (#31430)
  [DOCS] Move licensing APIs to docs (#31445)
  backport of: add is-write-index flag to aliases (#30942) (#31412)
  backport of: Add rollover-creation-date setting to rolled over index (#31144) (#31413)
  [Docs] Extend Homebrew installation instructions (#28902)
  [Docs] Mention ip_range datatypes on ip type page (#31416)
  Multiplexing token filter (#31208)
  Fix use of time zone in date_histogram rewrite (#31407)
  Revert "Mute DefaultShardsIT#testDefaultShards test"
  [DOCS] Fixes code snippet testing for machine learning (#31189)
  Security: fix joining cluster with production license (#31341)
  [DOCS] Updated version in Info API example
  [DOCS] Moves the info API to docs (#31121)
  Revert "Increasing skip version for failing test on 6.x"
  Preserve response headers on cluster update task (#31421)
  [DOCS] Add code snippet testing for more ML APIs (#31404)
  Docs: Advice for reindexing many indices (#31279)
dnhatn added a commit that referenced this pull request Jun 20, 2018
* master:
  [DOCS] Omit shard failures assertion for incompatible responses  (#31430)
  [DOCS] Move licensing APIs to docs (#31445)
  Add Delete Snapshot High Level REST API
  Remove QueryCachingPolicy#ALWAYS_CACHE (#31451)
  [Docs] Extend Homebrew installation instructions (#28902)
  Choose JVM options ergonomically
  [Docs] Mention ip_range datatypes on ip type page (#31416)
  Multiplexing token filter (#31208)
  Fix use of time zone in date_histogram rewrite (#31407)
  Core: Remove index name resolver from base TransportAction (#31002)
  [DOCS] Fixes code snippet testing for machine learning (#31189)
  [DOCS] Removed  and  params from MLT. Closes #28128 (#31370)
  Security: fix joining cluster with production license (#31341)
  Unify http channels and exception handling (#31379)
  [DOCS] Moves the info API to docs (#31121)
  Preserve response headers on cluster update task (#31421)
  [DOCS] Add code snippet testing for more ML APIs (#31404)
  Do not preallocate bytes for channel buffer (#31400)
  Docs: Advice for reindexing many indices (#31279)
  Mute HttpExporterTests#testHttpExporterShutdown test Tracked by #31433
  Docs: Add note about removing prepareExecute from the java client (#31401)
  Make release notes ignore the `>test-failure` label. (#31309)
@lcawl lcawl added the >docs General docs changes label Jul 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants