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

Fail yaml and doc tests when they receive unexpected Warning header #19793

Merged
merged 1 commit into from
Aug 4, 2016

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Aug 3, 2016

Now that using a deprecated feature in Elasticsearch returns a Warning header (#17804), we can assert that our yaml tests and the snippets in our docs don't receive unexpected warning headers. This is fairly useful in the yaml tests but it is very useful in the documentation because it immediately shows you any snippets in the documentation[0] that use the deprecated syntax. This should make it much harder to deprecate something and leave examples of it littered throughout the docs.

This PR removes most of the deprecated syntax from the snippets, replacing it with the non-deprecated syntax. In the cases where we are explicitly documenting a deprecated feature I simply add the warning to the docs so they pass. This found a few places where we were had examples for deprecated features but haven't marked them as deprecated, so I added that.

@nik9000 nik9000 added >enhancement >docs General docs changes >test Issues or PRs that are addressing/adding tests review >deprecation v5.0.0-beta1 labels Aug 3, 2016
@@ -6,7 +6,7 @@ set of documents. In order to do so, MLT selects a set of representative terms
of these input documents, forms a query using these terms, executes the query
and returns the results. The user controls the input documents, how the terms
should be selected and how the query is formed. `more_like_this` can be
shortened to `mlt` deprecated[5.0.0,use `more_like_this` instead).
shortened to `mlt` deprecated[5.0.0,use `more_like_this` instead].
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not 100% sure how I wandered to this point in the docs and fixed this, but it seems like a good thing anyway.

@nik9000
Copy link
Member Author

nik9000 commented Aug 3, 2016

Ping @elastic/es-clients because this introduces warnings feature to the yaml test runner that we use to assert that the warning headers are returned.

@nik9000
Copy link
Member Author

nik9000 commented Aug 3, 2016

We'll really only get the full benefit from this if we finish #18160, converting all of our snippets to // CONSOLE.

@javanna
Copy link
Member

javanna commented Aug 4, 2016

very nice! shall we document the new warning feature here?

@nik9000
Copy link
Member Author

nik9000 commented Aug 4, 2016

very nice! shall we document the new warning feature here?

Yeah! Let me push some docs!

@pickypg
Copy link
Member

pickypg commented Aug 4, 2016

This is awesome @nik9000. A great use of it!

@nik9000
Copy link
Member Author

nik9000 commented Aug 4, 2016

Yeah! Let me push some docs!

I pushed the docs. 😤

@javanna
Copy link
Member

javanna commented Aug 4, 2016

thanks @nik9000 LGTM

Adds `warnings` syntax to the yaml test that allows you to expect
a `Warning` header that looks like:
```
    - do:
        warnings:
            - '[index] is deprecated'
            - quotes are not required because yaml
            - but this argument is always a list, never a single string
            - no matter how many warnings you expect
        get:
            index:    test
            type:    test
            id:        1
```

These are accessible from the docs with:
```
// TEST[warning:some warning]
```

This should help to force you to update the docs if you deprecate
something. You *must* add the warnings marker to the docs or the build
will fail. While you are there you *should* update the docs to add
deprecation warnings visible in the rendered results.
@nik9000 nik9000 force-pushed the warn_on_deprecated branch from 8f6d4a2 to 1e58740 Compare August 4, 2016 19:59
@nik9000 nik9000 merged commit 1e58740 into elastic:master Aug 4, 2016
@nik9000
Copy link
Member Author

nik9000 commented Aug 4, 2016

Thanks for reviewing @javanna and @pickypg ! I've just merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>test Issues or PRs that are addressing/adding tests v5.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants