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

Maybe we should execute the snippets in our docs? #18075

Merged
merged 1 commit into from May 5, 2016

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 30, 2016

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 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
@nik9000
Copy link
Member Author

nik9000 commented Apr 30, 2016

cc @polyfractal

@nik9000
Copy link
Member Author

nik9000 commented Apr 30, 2016

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
Copy link
Member

IMHO we should do the opposite.

Build doc from existing tests.

I described this there elastic/docs#4

That said, that's nice.

@@ -19,6 +19,9 @@
package org.elasticsearch.test.rest.client;

import com.carrotsearch.randomizedtesting.RandomizedTest;

import static java.util.Objects.requireNonNull;

Copy link
Contributor

Choose a reason for hiding this comment

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

weird to have a static import in between regular imports?

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird. I'll fix.

@jpountz
Copy link
Contributor

jpountz commented May 2, 2016

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
Copy link
Member Author

nik9000 commented May 2, 2016

Related to #12583.

Thanks for that!

list the hacks you are the least happy with

Sure! I'll go leave comments at them.

public class RestTestsFromSnippetsTask extends SnippetsTask {

@Input
String docRoot = project.file('.') // There should be a better way!
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

nik9000 commented May 3, 2016

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.

@@ -92,7 +92,7 @@
Setting.longSetting("index.mapping.depth.limit", 20L, 1, Property.Dynamic, Property.IndexScope);
public static final boolean INDEX_MAPPER_DYNAMIC_DEFAULT = true;
public static final Setting<Boolean> INDEX_MAPPER_DYNAMIC_SETTING =
Setting.boolSetting("index.mapper.dynamic", INDEX_MAPPER_DYNAMIC_DEFAULT, Property.IndexScope);
Setting.boolSetting("index.mapper.dynamic", INDEX_MAPPER_DYNAMIC_DEFAULT, Property.Dynamic, Property.IndexScope);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

<1> Create an index with two types, both of which contain a `text` field which have the same mapping.
<2> Trying to update the `search_analyzer` just for `type_one` throws an exception like `"Merge failed with failures..."`.

But this then running this succeeds:
Copy link
Member Author

Choose a reason for hiding this comment

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

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

@jpountz
Copy link
Contributor

jpountz commented May 4, 2016

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 nik9000 removed the discuss label May 4, 2016
@nik9000
Copy link
Member Author

nik9000 commented May 4, 2016

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.

* structure as they input files relative to this directory.
*/
@Input
String docRoot = project.file('.') // There should be a better way!
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@rjernst
Copy link
Member

rjernst commented May 4, 2016

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
Copy link
Member Author

nik9000 commented May 5, 2016

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
Copy link
Member Author

nik9000 commented May 5, 2016

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
Copy link
Contributor

rmuir commented May 5, 2016

thanks for doing this, great idea

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >docs General docs changes Team:Delivery Meta label for Delivery team >test Issues or PRs that are addressing/adding tests v5.0.0-alpha3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants