-
Notifications
You must be signed in to change notification settings - Fork 24.7k
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
Conversation
cc @polyfractal |
In my exuberance I didn't describe the net result of the patch: if you stick It also adds |
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; | |||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. I'll fix.
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. |
Thanks for that!
Sure! I'll go leave comments at them. |
public class RestTestsFromSnippetsTask extends SnippetsTask { | ||
|
||
@Input | ||
String docRoot = project.file('.') // There should be a better way! |
There was a problem hiding this comment.
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.
OK! I've just finished getting (almost) all the It is still worth going through the docs and making sure that lots of places use |
@@ -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); |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
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.
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? |
I'm removing the @clintongormley will have to decide what the right way to label this is though - I just put Anyway, I have two things left on my list to do, but I'd love a review in the mean time:
|
* structure as they input files relative to this directory. | ||
*/ | ||
@Input | ||
String docRoot = project.file('.') // There should be a better way! |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
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 |
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! |
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.
thanks for doing this, great idea |
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!