Add support for regex in REST test warnings and allowed_warnings #69501

jakelandis · 2021-02-24T01:13:40Z

This commit adds support for two new REST test features.
warnings_regex and allowed_warnings_regex.

This is a near mirror of the warnings and allowed_warnings
warnings feature where the test can be instructed to allow
or require HTTP warnings. The difference with these new features
is that is allows the match to be based on a regular expression.

Also included in this PR is a new qa project rest-test-test
which can be used to exercise and validate the features of the
rest-test themselves. A rest test for the rest tests.

jakelandis · 2021-02-24T21:10:38Z

@elasticmachine update branch

elasticmachine · 2021-02-24T21:21:38Z

Pinging @elastic/es-delivery (Team:Delivery)

jakelandis · 2021-02-24T21:23:31Z

cc @elastic/es-clients -> this PR introduces two new features for the rest tests and ideally would be support across all of the clients tests.

jakelandis · 2021-02-24T21:24:37Z

test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/DoSectionTests.java

@@ -51,7 +52,7 @@ public void testWarningHeaders() {
        }

        final String testHeader = HeaderWarning.formatWarning("test");
-        final String anotherHeader = HeaderWarning.formatWarning("another");
+        final String anotherHeader = HeaderWarning.formatWarning("another \"with quotes and \\ backslashes\"");


no actual changes here...just realized we didn't have any coverage for escaped character matching.

sethmlarson · 2021-02-24T21:25:06Z

Thanks for the ping! Btw @jakelandis we're moving away from es-clients in favor of @elastic/clients-team.

jakelandis · 2021-02-24T21:28:17Z

Also note there will be a followup PR to allow injecting these for transformation, near identical to #69010

jakelandis · 2021-02-24T21:32:29Z

@elasticmachine run elasticsearch-ci/1

nik9000 · 2021-02-24T21:35:57Z

This commit adds support for two new REST test features.
warnings_regex and allowed_warnings_regex.

In other places in the framework we use / delimiters on strings to signal regexes. Its not the most precise thing but it never bothered me. Do we want to do _regex here?

jakelandis · 2021-02-24T21:41:29Z

In other places in the framework we use / delimiters on strings to signal regexes. Its not the most precise thing but it never bothered me. Do we want to do _regex here?

I considered it, but it would be sneaky feature that could break the clients testing. For example, without support from the clients test could add a /Foo something .*/ would try to match the literal string and fail. This approach, while more verbose allows it live behind a feature gate.

mark-vieira · 2021-02-24T22:53:14Z

.../rest-test-test/src/yamlRestTest/java/org/elasticsearch/rest/test/ClientYamlTestSuiteIT.java

+import org.elasticsearch.test.rest.yaml.ClientYamlTestCandidate;
+import org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase;
+
+public class ClientYamlTestSuiteIT extends ESClientYamlSuiteTestCase {


We have a number of tests in the :test:framework project itself to exercising the test framework. Is there a reason we can't put this stuff there? It seems like a much more appropriate place than introducing a new project to test a very specific piece of functionality that lives somewhere else. It would seem to me we could write an internal cluster test for this, which would remove the need to for an explicit plugin project just to declare a mock endpoint.

Looking more at what currently exists in terms of test coverage, I think just adding unit tests to the exist suite for this is sufficient. I think end-to-end testing with an external cluster is overkill here given we have no such equivalent testing for any of the other yaml rest testing functionality.

I think end-to-end testing with an external cluster is overkill

Fair point especially since there little development in this area. Will remove the additional qa project. The unit tests are already included and provide pretty comprehensive coverage.

done on 71f7b99 (#69501)

nik9000

I left a small thing. Makes sense to me.

nik9000 · 2021-02-25T13:02:23Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/ClientYamlTestSuite.java

@@ -152,6 +152,15 @@ public void validate() {
                "without a corresponding [\"skip\": \"features\": \"warnings\"] so runners that do not support the [warnings] " +
                "section can skip the test at line [" + section.getLocation().lineNumber + "]");

+        errors = Stream.concat(errors, sections.stream().filter(section -> section instanceof DoSection)


I wonder if all this would become more readable if it were a for loop. Its certainly faster to copy and paste the blocks but maybe its time to redo?

nik9000 · 2021-02-25T13:03:45Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/DoSection.java

- *      allowed_warnings:
+ *          - The non _regex version exact matches against the warning text and no need to worry about escaped quotes or backslashes
+ *          - The _regex version matches against the raw value of the warning text which may include backlashes and quotes escaped
+ *      allowed_warnings|allowed_warnings_regex:


I might break out the regex ones just to show what a regex will look like. I agree on preferring the non-regex one. Its nice not to have to think about what punctuation means.

…stic#69501) This commit adds support for two new REST test features. warnings_regex and allowed_warnings_regex. This is a near mirror of the warnings and allowed_warnings warnings feature where the test can be instructed to allow or require HTTP warnings. The difference with these new features is that is allows the match to be based on a regular expression. # Conflicts: # test/framework/src/main/java/org/elasticsearch/test/rest/yaml/Features.java # test/framework/src/test/java/org/elasticsearch/test/rest/yaml/section/DoSectionTests.java

#69501) (#69754) This commit adds support for two new REST test features. warnings_regex and allowed_warnings_regex. This is a near mirror of the warnings and allowed_warnings warnings feature where the test can be instructed to allow or require HTTP warnings. The difference with these new features is that is allows the match to be based on a regular expression.

This commit adds support to for injecting warnings_regex and allowed_warnings_regex via test transformations used by compatible testing. related: #69501 related: #69010

jakelandis added 2 commits February 23, 2021 17:14

initial draft

02ef8db

beefed up tests

7f69c32

jakelandis added the WIP label Feb 24, 2021

jakelandis added 2 commits February 24, 2021 13:27

clean up

7c96771

clean up

e3286d8

Merge branch 'master' into warning_regex

d9bb614

jakelandis marked this pull request as ready for review February 24, 2021 21:20

jakelandis requested review from nik9000 and pgomulka February 24, 2021 21:20

jakelandis added :Delivery/Build Build or test infrastructure >non-issue v7.13.0 v8.0.0 and removed WIP labels Feb 24, 2021

elasticmachine added the Team:Delivery Meta label for Delivery team label Feb 24, 2021

jakelandis commented Feb 24, 2021

View reviewed changes

jakelandis added 2 commits February 24, 2021 15:58

add some doc

9f5fbd9

typo

d0bdd8b

mark-vieira reviewed Feb 24, 2021

View reviewed changes

remove qa test

71f7b99

pgomulka approved these changes Feb 25, 2021

View reviewed changes

nik9000 approved these changes Feb 25, 2021

View reviewed changes

jakelandis merged commit 13915bc into elastic:master Mar 1, 2021

jakelandis deleted the warning_regex branch March 1, 2021 21:40

jakelandis added the backport pending label Mar 1, 2021

jakelandis mentioned this pull request Mar 1, 2021

[7.x] Add support for regex in REST test warnings and allowed_warnings (#69501) #69754

Merged

jakelandis removed the backport pending label Mar 2, 2021

jakelandis mentioned this pull request Mar 13, 2021

support regex warnings via REST test transform #70359

Merged

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for regex in REST test warnings and allowed_warnings #69501

Add support for regex in REST test warnings and allowed_warnings #69501

jakelandis commented Feb 24, 2021 •

edited

Loading

jakelandis commented Feb 24, 2021

elasticmachine commented Feb 24, 2021

jakelandis commented Feb 24, 2021

jakelandis Feb 24, 2021

sethmlarson commented Feb 24, 2021

jakelandis commented Feb 24, 2021 •

edited

Loading

jakelandis commented Feb 24, 2021

nik9000 commented Feb 24, 2021

jakelandis commented Feb 24, 2021

mark-vieira Feb 24, 2021

mark-vieira Feb 24, 2021

jakelandis Feb 24, 2021

jakelandis Feb 24, 2021

nik9000 left a comment

nik9000 Feb 25, 2021

nik9000 Feb 25, 2021

Add support for regex in REST test warnings and allowed_warnings #69501

Add support for regex in REST test warnings and allowed_warnings #69501

Conversation

jakelandis commented Feb 24, 2021 • edited Loading

jakelandis commented Feb 24, 2021

elasticmachine commented Feb 24, 2021

jakelandis commented Feb 24, 2021

jakelandis Feb 24, 2021

Choose a reason for hiding this comment

sethmlarson commented Feb 24, 2021

jakelandis commented Feb 24, 2021 • edited Loading

jakelandis commented Feb 24, 2021

nik9000 commented Feb 24, 2021

jakelandis commented Feb 24, 2021

mark-vieira Feb 24, 2021

Choose a reason for hiding this comment

mark-vieira Feb 24, 2021

Choose a reason for hiding this comment

jakelandis Feb 24, 2021

Choose a reason for hiding this comment

jakelandis Feb 24, 2021

Choose a reason for hiding this comment

nik9000 left a comment

Choose a reason for hiding this comment

nik9000 Feb 25, 2021

Choose a reason for hiding this comment

nik9000 Feb 25, 2021

Choose a reason for hiding this comment

jakelandis commented Feb 24, 2021 •

edited

Loading

jakelandis commented Feb 24, 2021 •

edited

Loading