Maybe we should execute the snippets in our docs? by nik9000 · Pull Request #18075 · elastic/elasticsearch

nik9000 · 2016-04-30T02:27:42Z

I think we should execute the snippets we have in our docs as tests. We have this handy dandy rest test framework. Why not cram those snippets into it? Well, the snippets are in JSON and the rest framework is in YAML. But we can still do it with the power of really big strings!

This PR is both terrible and wonderful. It is full of horrible, horrible hacks. They provide a way for us to actually test our documentation to make sure it doesn't go out of date. Please, review this and suggest ways to have less hacks.

If we end up going this way (I hope we do!) then this is more of a beginning step. This starts to give us tools to make decisions about the snippets in our docs at build time.

Edit: mostly we've resolved the hacks now. So the PR is probably not terrible!

nik9000 · 2016-04-30T02:28:45Z

cc @polyfractal

nik9000 · 2016-04-30T02:51:49Z

In my exuberance I didn't describe the net result of the patch: if you stick // TEST under a snippet in the docs it'll generate a rest test that runs the command. This isn't a hugely useful test, but it is nice. It only really works with // AUTOSENSE style snippets.

It also adds // TESTRESPONSE which you can stick on a snippet. If you do it'll add an assertion to the test generated by the last // TEST snippet that checks if the response matches the response.

dadoonet · 2016-04-30T05:56:13Z

IMHO we should do the opposite.

Build doc from existing tests.

I described this there elastic/docs#4

That said, that's nice.

jpountz · 2016-05-02T07:55:31Z

test/framework/src/main/java/org/elasticsearch/test/rest/client/RestClient.java

weird to have a static import in between regular imports?

Weird. I'll fix.

jpountz · 2016-05-02T08:05:12Z

I am not familiar enough with gradle to fully understand the build part, but in general I like the PR. Maybe you can list the hacks you are the least happy with so that we can think about how to make things better?

Related to #12583.

nik9000 · 2016-05-02T12:35:14Z

Related to #12583.

Thanks for that!

list the hacks you are the least happy with

Sure! I'll go leave comments at them.

nik9000 · 2016-05-02T12:38:05Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/RestTestsFromSnippetsTask.groovy

This is the start of the first hack I don't like. I use it to make the name and path of the file containing the tests generated from the docs. SnippetsTask already has a reference to the FileTree that is the files I'm going to iterate. I guess I'm not happy that you have to define it twice.

nik9000 · 2016-05-03T19:30:54Z

OK! I've just finished getting (almost) all the // AUTOSENSE snippets passing! They generate tests. Most pass all on their own. Sometimes we do tricks like sneak a PUT my_index into the front of the tests they make to get them working and not change the output.

It is still worth going through the docs and making sure that lots of places use // AUTOSENSE just so we can actually catch stuff.

nik9000 · 2016-05-03T19:45:33Z

core/src/main/java/org/elasticsearch/index/mapper/MapperService.java

The tests generated by the docs caught this. I'm not sure what extra testing I should add for this though.

nik9000 · 2016-05-03T19:53:28Z

docs/reference/indices/put-mapping.asciidoc

Here I broke the snippet up because I couldn't easily cram it into the (arbitrary) rules I made for the tests.

jpountz · 2016-05-04T08:04:27Z

This looks wonderful. There are some hacks indeed, but they look manageable to me. It would be great if someone who is better educated about gradle than I am could take a look.

@clintongormley this would be a big change to our docs, do you have any opinions?

nik9000 · 2016-05-04T12:36:22Z

I'm removing the discuss label in that case.

@clintongormley will have to decide what the right way to label this is though - I just put docs and build and test because it touches those things in major ways but I think docs is reserved for PRs that only update docs so the script that builds that release notes can skip them?

Anyway, I have two things left on my list to do, but I'd love a review in the mean time:

Get that one less file running. Most of it has to be skipped because it modifies the cluster in bad ways. We should really (separately) have a way to reset the cluster settings between these tests. I think that'd unblock a few tests.
Add a notification to the failures giving you some hint what file the tests were generated from.

rjernst · 2016-05-04T22:09:05Z

buildSrc/src/main/groovy/org/elasticsearch/gradle/RestTestsFromSnippetsTask.groovy

Why wouldn't this be File docRoot = project.projectDir?

Actually maybe @InputDirectory? That will make it dependent on the contents of the directory, instead of just the path.

This isn't dependent on the contents of the directory. It is just used to form the output files. That is why I hate it. The superclass as an @InputDirectory.

But it should be project.projectDir. I just forgot that was a thing.

rjernst · 2016-05-04T23:19:54Z

This looks good to me to get in so we can iterate and improve on it. The one comment which I have is about simplifying the configuration. I think we should use ConfigurableFileTree, and this should allow removing this extra root dir input which confused me.

nik9000 · 2016-05-05T14:33:34Z

OK! I've cleaned up quite a lot this morning. I've handled all of @rjernst's points. His help was super useful and let me resolve the issue around the really annoying/confusing extra root.

I'm going ton keep using the exploded way of printing error messages for mismatches in the body for now. I don't really like it too much but it produces nice error messages.

I'm going to get that one last file running and then squash and rebase. There will be conflicts but I expect them just to be with docs files. Then I think it is time to merge!

nik9000 · 2016-05-05T17:36:01Z

get that one last file running

Yeah, that'll have to wait: #18159

Adds infrastructure so `gradle :docs:check` will extract tests from snippets in the documentation and execute the tests. This is included in `gradle check` so it should happen on CI and during a normal build. By default each `// AUTOSENSE` snippet creates a unique REST test. These tests are executed in a random order and the cluster is wiped between each one. If multiple snippets chain together into a test you can annotate all snippets after the first with `// TEST[continued]` to have the generated tests for both snippets joined. Snippets marked as `// TESTRESPONSE` are checked against the response of the last action. See docs/README.asciidoc for lots more. Closes elastic#12583. That issue is about catching bugs in the docs during build. This catches *some* bugs in the docs during build which is a good start.

rmuir · 2016-05-05T18:50:06Z

thanks for doing this, great idea

nik9000 added >docs General docs changes >test Issues or PRs that are addressing/adding tests review :Delivery/Build Build or test infrastructure discuss v5.0.0-alpha3 labels Apr 30, 2016

jpountz reviewed May 2, 2016
View reviewed changes

nik9000 reviewed May 2, 2016
View reviewed changes

nik9000 force-pushed the snippet_fun branch from 6c5d32d to f3dc34e Compare May 2, 2016 18:34

nik9000 added review and removed review labels May 2, 2016

nik9000 force-pushed the snippet_fun branch from 82a4fe3 to 8efd5ce Compare May 3, 2016 19:36

nik9000 reviewed May 3, 2016
View reviewed changes

nik9000 removed the discuss label May 4, 2016

rjernst reviewed May 4, 2016
View reviewed changes

nik9000 force-pushed the snippet_fun branch from 11e1159 to e9d3e44 Compare May 5, 2016 17:35

nik9000 force-pushed the snippet_fun branch from e9d3e44 to 4b1c116 Compare May 5, 2016 17:58

nik9000 mentioned this pull request May 5, 2016

Convert all the docs to // CONSOLE #18160

Closed

nik9000 merged commit 4b1c116 into elastic:master May 5, 2016

danielmitterdorfer mentioned this pull request May 11, 2016

Proposal: Elasticsearch doctest #13739

Closed

MaineC mentioned this pull request May 17, 2016

Maybe we should make our REST test examples visible in our docs? #18397

Closed

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Conversation

nik9000 commented Apr 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nik9000 commented Apr 30, 2016

Uh oh!

nik9000 commented Apr 30, 2016

Uh oh!

dadoonet commented Apr 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz commented May 2, 2016

Uh oh!

nik9000 commented May 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented May 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpountz commented May 4, 2016

Uh oh!

nik9000 commented May 4, 2016

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!

rjernst commented May 4, 2016

Uh oh!

nik9000 commented May 5, 2016

Uh oh!

nik9000 commented May 5, 2016

Uh oh!

rmuir commented May 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

nik9000 commented Apr 30, 2016 •

edited

Loading