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

Can't use both warning and catch in a yamlRestTest #76316

Open
joegallo opened this issue Aug 10, 2021 · 2 comments
Open

Can't use both warning and catch in a yamlRestTest #76316

joegallo opened this issue Aug 10, 2021 · 2 comments
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team

Comments

@joegallo
Copy link
Contributor

joegallo commented Aug 10, 2021

I discovered this while I was working on #75951. For one of the tests I wanted to write there, the expected outcome is that I'd get both a json body error from ES, and also a warning header. It turns out, though, if you use a catch, then any associated warning elements from the test are ignored.

It's easy enough to see why that is in the code. checkWarningHeaders is called after a successful response, but error responses go into the catch block for catch processing, and there's no associated call to checkWarningHeaders there.

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 0223ef27b6c..0fe690be379 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
@@ -367,6 +367,11 @@ public class DoSection implements ExecutableSection {
             } else {
                 throw new UnsupportedOperationException("catch value [" + catchParam + "] not supported");
             }
+
+            final String testPath = executionContext.getClientYamlTestCandidate() != null
+                ? executionContext.getClientYamlTestCandidate().getTestPath()
+                : null;
+            checkWarningHeaders(restTestResponse.getWarningHeaders(), testPath);
         }
     }

^ appears to do the trick, but then there are failures in the yamlRestCompatTests -- I suppose that means that some of our tests incidentally happen to rely on the current behavior.

@joegallo joegallo added >bug :Core/Infra/REST API REST infrastructure and utilities needs:triage Requires assignment of a team area label labels Aug 10, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label Aug 10, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@nik9000
Copy link
Member

nik9000 commented Aug 13, 2021

Neat! I'm not surprised we ignored warnings when we didn't have to mark them.

@DJRickyB DJRickyB removed the needs:triage Requires assignment of a team area label label Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Core/Infra/REST API REST infrastructure and utilities Team:Core/Infra Meta label for core/infra team
Projects
None yet
Development

No branches or pull requests

4 participants