EQL: Add case-sensitive and insensitive integration tests #57323

rw-access · 2020-05-28T23:46:40Z

Pulled over the latest TOML integration tests from Python, which have case (in)sensitive toggles.

There's a handful more tests added, and I noticed that some of the sequence tests are failing, so I left those in the exclusions list. Also updated the HLRC to include case sensitive toggles, which was needed for the tests.

Basically to mark a test as case-sensitive only, add this to the TOML

case_sensitive = true

or this to make a test case-insensitive only.

case_insensitive = true

If the flags aren't set, then the test is assumed to work with both. case_insensitive=true and case_sensitive=true are mutually exclusive.

elasticmachine · 2020-05-28T23:46:42Z

Pinging @elastic/es-ql (:Query Languages/EQL)

rw-access · 2020-06-02T17:06:20Z

x-pack/plugin/eql/qa/common/src/main/resources/test_queries_unsupported.toml

+
+
+##########################
+# FAILING SEQUENCE TESTS #


@costin do you have any guesses why some of these sequence tests were failing?

I believe it's because of the tie breaker - the logic only looks at the timestamp but since it's the same for all events, it fails to order it by the seq_number.

Should be fixed now that the tiebreaker is in. It is disabled by default however our tests make use of it.

client/rest-high-level/src/main/java/org/elasticsearch/client/eql/EqlSearchRequest.java

aleksmaus · 2020-06-03T18:20:32Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpec.java


 public class EqlSpec {
    private String description;
    private String note;
    private String[] tags;
    private String query;
+    private Boolean caseSensitiveOnly;
+    private Boolean caseInsensitiveOnly;


can use boolean? or need 'null' for any reason?

This class is similar to that in #57568, I wonder if the TOML code could be reused. It's not a lot but not too little either. Between the spec, boilerplate code in the test for reading the spec there is something that potentially (not right now) can be placed in a separate, small utility project.
It's a bit overheard but medium term could simplify dependency management.

Either way a comment pointing out the relationship between the two would be useful so that any updates happening on one end would be useful in the other.

switched to boolean, thanks @aleksmaus. I handle null => false within the SpecLoader.

I'm not sure the ideal solution to avoid reuse while keeping it fairly simple. at the least, I can add a comment like you suggest for both classes

will leave this PR as is, and update in #57568

astefan

LGTM in general. Left some comments, though.

client/rest-high-level/src/main/java/org/elasticsearch/client/eql/EqlSearchRequest.java

astefan · 2020-06-04T16:20:23Z

...k/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/CommonEqlActionTestCase.java

@@ -195,8 +202,8 @@ else if (hits.sequences() != null) {
        }
    }

-    protected EqlSearchResponse runQuery(String index, String query) throws Exception {
-        EqlSearchRequest request = new EqlSearchRequest(testIndexName, query);
+    protected EqlSearchResponse runQuery(String index, String query, boolean isCaseSensitive) throws Exception {


Maybe add two more methods - runQueryCaseSensitive and runQueryCaseInsensitive - that should "hide" the boolean parameter choice and call these instead of runQuery(...., true/false)?

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpecLoader.java

astefan · 2020-06-04T16:50:37Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpecLoader.java

@@ -55,6 +55,14 @@ private static String getTrimmedString(TomlTable table, String key) {
            spec.note(getTrimmedString(table, "note"));
            spec.description(getTrimmedString(table, "description"));

+            // default these values to false
+            spec.caseSensitiveOnly(Boolean.TRUE.equals(table.getBoolean("case_sensitive")));


I find the "parameter" from the TOML tests a bit confusing. At Java level it's caseSensitiveOnly and in tests is case_sensitive. Imo, it's an important difference between something being "case sensitive ONLY" and "case sensitive".

A test that is "case sensitive" can, also, be "case insensitive", meaning a test that doesn't care about case sensitivity at all. For example, a test where no string functions are involved: this one could run in whatever configuration. My suggestion is to have in .toml files the only bit added to parameters to make a bit more clear.

in 254b5c3, I removed the implicit behavior for and the concept of "case (in)sensitive only".
now every test requires case_sensitive=true and case_insensitive=true if it should be tested with both

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpecLoader.java

matriv

LGTM, left a comment

client/rest-high-level/src/main/java/org/elasticsearch/client/eql/EqlSearchRequest.java

aleksmaus

Couple of nits, otherwise LGTM

aleksmaus · 2020-06-18T14:42:27Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpec.java

+
+        return Objects.equals(this.query(), that.query())
+                && Objects.equals(this.caseSensitive, that.caseSensitive)
+                && Objects.equals(this.caseInsensitive, that.caseInsensitive);


nit: missing description, expectedEventIds?

aleksmaus · 2020-06-18T14:42:48Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpec.java

+
+    @Override
+    public int hashCode() {
+        return Objects.hash(this.query, this.caseSensitive, this.caseInsensitive);


same nit as above

costin

If the flags aren't set, then the test is assumed to work with both. case_insensitive=true and case_sensitive=true are mutually exclusive.

Looking at the case, this doesn't seem to be the case. A lot of tests have both parameters specified, that shouldn't be the case though.

costin · 2020-06-18T20:33:20Z

...k/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/CommonEqlActionTestCase.java

            }
-            if (Strings.isNullOrEmpty(name)) {
-                name = "" + (counter.get() + 1);
+            else if (Strings.isNullOrEmpty(spec.note()) == false) {


Unless all tests have a note, please keep the name counter since otherwise the name becomes the query which makes it impossible to rerun the test (since it's not a correct method name).

costin · 2020-06-18T20:35:52Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpec.java

+    private boolean caseSensitive = false;
+    private boolean caseInsensitive = false;


As on the previous issue, having a Boolean object should cover all the cases.
TRUE - sensitive, FALSE - insensitive, null - unspecified (apply both).

costin · 2020-06-18T20:40:33Z

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpecLoader.java

+            if (spec.caseInsensitive() == false && spec.caseSensitive() == false) {
+                throw new IllegalArgumentException("Test must support case-sensitivity or case-insensitivity for test: " + spec.toString());
+            }


Instead of forcing each query to specify a case sensitivity, the default should be missing an argument means that parameter is true.
That is instead of specifying:

case_sensitive = true case_insensitive = true

don't specify anything. If something is just case sensitive, it means it's insensitive and vice-versa and when none is specified, apply both (case irrelevant).
This makes the test declaration easier.

This sounds like what I used to have, and simplified writing the tests but the behavior didn't seem to be well liked within EqlSpec.

Instead of forcing each query to specify a case sensitivity, the default should be missing an argument means that parameter is true.

This was the original behavior

[[queries]] # this was an error case_sensitive = true case_insensitive = true [[queries]] # test will be checked with case sensitive mode only case_sensitive = true [[queries]] # test will be checked with case insensitive mode only case_insensitive = true [[queries]] # test will be checked with both case sensitive and case-insensitive mode # case_sensitive = null # case_insensitive = null

But if missing an argument "means that parameter is true", then you basically are inverting the flags. Then you have to negate the logic to denote which tests are not allowed

[[queries]] # the only way to run a test as case-insensitive only case_sensitive = false

I think the most desired behavior is this?
If both case_sensitive and case_insensitive are null, then set them to both to true

I think the most desired behavior is this?
If both case_sensitive and case_insensitive are null, then set them to both to true

Correct. Looking at the individual properties in isolation and setting them to true won't work since they both have to be missing.

astefan · 2020-06-30T13:45:43Z

Superseded by #58624

EQL: Add case-sensitive and insensitive integration tests

28a3f55

rw-access added the :Analytics/EQL EQL querying label May 28, 2020

rw-access requested review from costin, aleksmaus and astefan May 28, 2020 23:46

elasticmachine added the Team:QL (Deprecated) Meta label for query languages team label May 28, 2020

rw-access requested a review from matriv May 28, 2020 23:48

rw-access added 2 commits May 28, 2020 17:49

EQL: Remove whitespace from EqlSearchRequest.java

125f19d

EQL: Update EqlSpec.equals

bbc7215

rw-access commented Jun 2, 2020

View reviewed changes

rw-access mentioned this pull request Jun 2, 2020

EQL: Integrate TOML tests for function folding #57568

Closed

aleksmaus reviewed Jun 3, 2020

View reviewed changes

EQL: Unbox Boolean for testing

48d3b72

astefan approved these changes Jun 4, 2020

View reviewed changes

astefan reviewed Jun 4, 2020

View reviewed changes

x-pack/plugin/eql/qa/common/src/main/java/org/elasticsearch/test/eql/EqlSpecLoader.java Outdated Show resolved Hide resolved

rw-access added 5 commits June 16, 2020 10:10

EQL: Make case_sensitive/case_insensitive required

254b5c3

Merge branch 'master' into eql/case-sensitive-tests

e74da39

EQL: Update queries blacklist

28d9a76

EQL: Remove unnecessary blacklists

545ab4a

EQL: Unbox EqlSearchRequest.isCaseSensitive

b8d16d3

rw-access requested a review from aleksmaus June 16, 2020 17:08

EQL: Lint/nitpick feedback

0e742de

matriv approved these changes Jun 16, 2020

View reviewed changes

client/rest-high-level/src/main/java/org/elasticsearch/client/eql/EqlSearchRequest.java Outdated Show resolved Hide resolved

EQL: Fix EqlSearchRequest merge conflict duplicate line

2de36a2

aleksmaus approved these changes Jun 18, 2020

View reviewed changes

costin requested changes Jun 18, 2020

View reviewed changes

astefan self-assigned this Jun 26, 2020

This was referenced Jun 26, 2020

EQL: case sensitivity aware integration testing #58624

Merged

EQL: Integrate TOML tests for function folding #58748

Merged

astefan closed this Jun 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EQL: Add case-sensitive and insensitive integration tests #57323

EQL: Add case-sensitive and insensitive integration tests #57323

rw-access commented May 28, 2020 •

edited

Loading

elasticmachine commented May 28, 2020

rw-access Jun 2, 2020

costin Jun 3, 2020

costin Jun 11, 2020

aleksmaus Jun 3, 2020

costin Jun 4, 2020

rw-access Jun 4, 2020

rw-access Jun 16, 2020

astefan left a comment

astefan Jun 4, 2020

astefan Jun 4, 2020

rw-access Jun 16, 2020

matriv left a comment

aleksmaus left a comment

aleksmaus Jun 18, 2020

aleksmaus Jun 18, 2020

costin left a comment

costin Jun 18, 2020

costin Jun 18, 2020

costin Jun 18, 2020

rw-access Jun 18, 2020 •

edited

Loading

rw-access Jun 18, 2020 •

edited

Loading

costin Jun 22, 2020

astefan commented Jun 30, 2020

		private boolean caseSensitive = false;
		private boolean caseInsensitive = false;

EQL: Add case-sensitive and insensitive integration tests #57323

EQL: Add case-sensitive and insensitive integration tests #57323

Conversation

rw-access commented May 28, 2020 • edited Loading

elasticmachine commented May 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matriv left a comment

Choose a reason for hiding this comment

aleksmaus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

costin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rw-access Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

rw-access Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astefan commented Jun 30, 2020

rw-access commented May 28, 2020 •

edited

Loading

rw-access Jun 18, 2020 •

edited

Loading

rw-access Jun 18, 2020 •

edited

Loading