Ignore warnings related to types deprecation in REST tests.

[jtibshirani]

Relates to #15613 (comment).

Because types deprecation warnings may differ in their content + structure, we check for the prefix [types removal] which all types-related warnings should make sure to use.

[elasticmachine]

Pinging @elastic/es-core-infra

[jtibshirani]

@elasticmachine run gradle build tests

[javanna]

I wonder if this change requires some adapting on language clients test runners. I think that some clients don't support headers, hence they can't support the warning feature, so this does not affect them (they already skip all the tests around warnings and don't fail if warnings are returned). Not sure what other clients that support headers do though. If they do fail when a warning is returned, which they should, their runner should be adapted.

[jtibshirani]

Thanks @javanna for the note, I'll make sure to tag the clients team on these PRs.

[jtibshirani]

CC @elastic/es-clients

[jasontedor]

LGTM. I left one suggestion at your discretion. No need for another round either way.

[jasontedor]

The change itself is fine, I just have one comment about how it's structured. Rather than an if with an or between the two types of warnings that we are ignoring, I would rather see separate cases. For example

diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java
index c21d80602c9..7d01c6af20c 100644
--- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java
+++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java
@@ -293,18 +293,19 @@ public class DoSection implements ExecutableSection {
             if (matches) {
                 final String message = matcher.group(1);
                 // noinspection StatementWithEmptyBody
-                if ((masterVersion.before(Version.V_7_0_0)
+                if (masterVersion.before(Version.V_7_0_0)
                         && message.equals("the default number of shards will change from [5] to [1] in 7.0.0; "
                         + "if you wish to continue using the default of [5] shards, "
-                        + "you must manage this on the create index request or with an index template"))
-                    || message.startsWith("[types removal]")) {
+                        + "you must manage this on the create index request or with an index template")) {
                     /*
-                     * We ignore two classes of warning headers:
-                     * - The default number of shards warning will come back in the vast majority of our tests that
-                     * create an index when running against an older master. Rather than rewrite our tests to assert
-                     * this warning header, we assume that it is expected.
-                     * - We skip warnings related to types deprecation so that we can continue to run the many
-                     * mixed-version tests that used typed APIs.
+                     * This warning header will come back in the vast majority of our tests that create an index when running against an
+                     * older master. Rather than rewrite our tests to assert this warning header, we assume that it is expected.
+                     */
+                } else //noinspection StatementWithEmptyBody
+                    if (masterVersion.before(Version.V_7_0_0) && message.startsWith("[types removal]")) {
+                    /*
+                     * This warning header will come back in the tests that use typed APIs. Rather than rewrite our tests to assert this
+                     * warning header, we assume that it is expected.
                      */
                 } else if (expected.remove(message) == false) {
                     unexpected.add(header);

I am not tied to this at all, though.

[jtibshirani]

Thanks @jasontedor for your suggestion. I initially avoided that structure because of the awkward else noinspection ... \n if, but I now think it's important for clarity because in this format, you misread the boolean statements! The types-related check does not include masterVersion.before(Version.V_7_0_0).

[jasontedor]

Oops, I indeed misread the parentheses; I double checked even but it’s exactly because I had to strain across multiple lines to match them up I propose the change. 😇

[jasontedor]

LGTM!

[jtibshirani]

@elasticmachine run gradle build tests

[markharwood]

Shouldn't this be indented a level so the close bracket lines up with the if it relates to?

[jasontedor]

I don’t think so, think of it as a multi-line if:

 if (a
     || b
     || c) {
   // do something
 }

[markharwood]

Ignore me. We don't indent chains of else ifs and this just threw me because the else is separated from the if by a comment.