[Docs] Convert more doc snippets #26404

tlrx · 2017-08-28T13:40:37Z

This pull request converts some more remaining doc snippets so that they are now testable.

Note: I think it's perfectly fine to have some // NOTCONSOLE here and there as long as they are associated to an explanation on why it cannot be tested.

This commit converts some remaining doc snippets so that they are now testable.

tlrx · 2017-08-28T13:42:33Z

docs/reference/search/profile.asciidoc

+                                  ]
+                                },
+                                {
+                                  "name": "BucketCollector: [[org.elasticsearch.search.profile.aggregation.ProfilingAggregator@222b076, org.elasticsearch.search.profile.aggregation.ProfilingAggregator@3000ab31]]",


ProfilingAggregator.toString() changed? I'd expect to find the name here.

I bet it did. This would have caught that it broke user output too.... Might be a nice followup I think.

Yeah, not sure what's going on here. I don't think ProfilingAggregator ever overrode toString() (at least I don't see it in the history anywhere), so I'm not sure how this worked before either :)

In either case we should just delegate to the wrapped collector's name. I'll investigate and open an issue/PR if appropriate.

In either case we should just delegate to the wrapped collector's name. I'll investigate and open an issue/PR if appropriate.

Thanks @polyfractal :)

tlrx · 2017-08-28T13:44:08Z

docs/reference/search/profile.asciidoc


 1. The first `TermQuery` (message:search) represents the main `term` query
 2. The second `TermQuery` (my_field:foo) represents the `post_filter` query
-3. There is a  `MatchAllDocsQuery` (\*:*) query which is being executed as a second, distinct search.  This was


The MatchAllDocsQuery does not appear in the original search query, neither in the updated one. I think the explanation is now obsolete but I'd be happy if someone can confirm/infirm this.

This is kinda odd... I think we have to execute a secondary collector for the global agg (since the first is being filtered by the query).

I think modifying the docs right now is fine, but I'll start chasing why this changed because it could be something wrong with the profiler itself.

nik9000

I can't comment on the profiling snippets. Maybe @polyfractal can have a look?

nik9000 · 2017-08-28T13:52:04Z

docs/reference/cluster/pending.asciidoc

  ]
 }
 --------------------------------------------------
+// NOTCONSOLE


I'd originally thought of // NOTCONSOLE as only for requests that looked like they should be converted to // CONSOLE but can't for some reason, like maybe we're demonstrating how to use curl in the _bulk API.

While the name // NOTCONSOLE isn't really right for this the idea makes sense. We can't assert anything about this response.

So I'm 👍 on doing this here.

nik9000 · 2017-08-28T13:54:56Z

docs/reference/docs/reindex.asciidoc

  "failures" : [ ]
 }
 --------------------------------------------------
+// NOTCONSOLE


I think we can actually test this and the advantage of doing so is that we'll catch if the output of reindex changes.

Right, this snippet is really out of date

nik9000 · 2017-08-28T13:55:13Z

docs/reference/docs/update-by-query.asciidoc

  "failures" : [ ]
 }
 --------------------------------------------------
+// NOTCONSOLE


I think we can test this too.

nik9000 · 2017-08-28T13:56:20Z

docs/reference/index-modules/similarity.asciidoc

 [source,js]
 --------------------------------------------------
-PUT /my_index
+PUT index


I've mostly been starting with the / before the first element so it feels more like a url. At first I liked it better without but @clintongormley liked it better with so I went with his way and I've grown used to it.

Sadly we're not coherent in the docs. I changed it here so that it is coherent on the same page.

nik9000 · 2017-08-28T13:57:13Z

docs/reference/index-modules/similarity.asciidoc

+
+[source,js]
+--------------------------------------------------
+PUT index/_settings


Maybe just put the POST /index/_close request in the snippet so the users sees it. We tell them to close but it is a fine reminder of how.

nik9000 · 2017-08-28T13:57:35Z

docs/reference/index-modules/similarity.asciidoc

    }
  }
 }
 --------------------------------------------------


You could even add an _open here.

nik9000 · 2017-08-28T13:58:54Z

docs/reference/index-modules/translog.asciidoc

 Creating new empty translog at [data/nodes/0/indices/P45vf_YQRhqjfwLMUvSqDw/0/translog/translog-3.tlog]
 Done.
 --------------------------------------------------
+// NOTCONSOLE


Here I'd change the snippet from [source,js] to [source,txt] which will cause it to skip the detection so you don't need // NOTCONSOLE.

Good catch, I don't remember why I added this NOTCONSOLE

nik9000 · 2017-08-28T14:00:53Z

docs/reference/search/profile.asciidoc

+                                  ]
+                                },
+                                {
+                                  "name": "BucketCollector: [[org.elasticsearch.search.profile.aggregation.ProfilingAggregator@222b076, org.elasticsearch.search.profile.aggregation.ProfilingAggregator@3000ab31]]",


I bet it did. This would have caught that it broke user output too.... Might be a nice followup I think.

polyfractal · 2017-08-28T14:29:12Z

docs/reference/search/profile.asciidoc

+        ...
+        "aggregations": [
+          {
+            "type": "org.elasticsearch.search.aggregations.bucket.terms.TermsAggregatorFactory$1",


Looks like there are more naming issues to iron out in the profiler :(

The docs tests are lovely, aren't they? :)

polyfractal · 2017-08-28T14:30:07Z

Profiler stuff looks fine to me. There are some oddities for sure, but not a problem with the docs... likely issues with the profiler code itself. I'll chase those down. Thanks for adding these tests @tlrx!

nik9000 · 2017-08-29T15:56:35Z

Thanks for doing this!

tlrx · 2017-08-30T07:35:39Z

Thanks @nik9000 @polyfractal

This commit converts some remaining doc snippets so that they are now testable.

[Docs] Convert more doc snippets

a91a0cd

This commit converts some remaining doc snippets so that they are now testable.

tlrx added >docs General docs changes review >test Issues or PRs that are addressing/adding tests v6.0.0 v7.0.0 v6.1.0 labels Aug 28, 2017

tlrx commented Aug 28, 2017

View reviewed changes

nik9000 approved these changes Aug 28, 2017

View reviewed changes

$polyfractal$

polyfractal reviewed Aug 28, 2017

View reviewed changes

$@polyfractal$ polyfractal mentioned this pull request Aug 28, 2017

Investigate profiling naming issues #26405

Closed

Apply feedback

3c88cec

nik9000 approved these changes Aug 29, 2017

View reviewed changes

tlrx merged commit db54c4d into elastic:master Aug 30, 2017

tlrx added a commit that referenced this pull request Aug 30, 2017

[Docs] Convert more doc snippets (#26404)

719e794

This commit converts some remaining doc snippets so that they are now testable.

tlrx added a commit that referenced this pull request Aug 30, 2017

[Docs] Convert more doc snippets (#26404)

428c1ac

This commit converts some remaining doc snippets so that they are now testable.

tlrx deleted the more-docs branch August 30, 2017 08:18

colings86 added v6.0.0-rc1 and removed v6.0.0 labels Sep 22, 2017

lcawl removed the v6.1.0 label Dec 12, 2017

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

[Docs] Convert more doc snippets #26404

[Docs] Convert more doc snippets #26404

Uh oh!

Conversation

tlrx commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polyfractal commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik9000 commented Aug 29, 2017

Uh oh!

tlrx commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tlrx commented Aug 28, 2017 •

edited

Loading

polyfractal commented Aug 28, 2017 •

edited

Loading