Skip to content

EQL: Extract query folder tests definitions into resources#53802

Merged
aleksmaus merged 1 commit intoelastic:masterfrom
aleksmaus:improve/test_queryfolder_externalize
Mar 20, 2020
Merged

EQL: Extract query folder tests definitions into resources#53802
aleksmaus merged 1 commit intoelastic:masterfrom
aleksmaus:improve/test_queryfolder_externalize

Conversation

@aleksmaus
Copy link
Copy Markdown
Contributor

@costin, this addresses the other ask that you had for extracting the tests specs into resources.
As far as I understood we were talking about simple text file, but can change the format however needed. Let me know.

Screen Shot 2020-03-19 at 10 24 39 AM

@aleksmaus aleksmaus requested review from astefan, costin and imotov March 19, 2020 14:27
@aleksmaus aleksmaus added :Analytics/EQL EQL querying >test Issues or PRs that are addressing/adding tests labels Mar 19, 2020
@elasticmachine
Copy link
Copy Markdown
Collaborator

Pinging @elastic/es-search (:Search/EQL)

@aleksmaus aleksmaus force-pushed the improve/test_queryfolder_externalize branch from 724e772 to 2ffa9cf Compare March 19, 2020 14:38
Copy link
Copy Markdown
Contributor

@imotov imotov left a comment

Choose a reason for hiding this comment

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

I really like where it is going, and I understand that this is just a string comparison at the moment. However, I wonder if it is possible to make the testing expressions a bit more JSON-like and make it not rely on the order of fields in JSON objects.

One idea would be to just put json parse it into a map and make sure this it is "submap" of the response. So basically something like {"foo": {"baz":2"}} would match response {"foo": {"bar": 1, "baz":2"}}.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

These unclosed parentheses are tearing holes in my OCD soul. 😄

@aleksmaus
Copy link
Copy Markdown
Contributor Author

@imotov that how it was done originally in the previous PR, I loaded into maps and compared maps, so didn't have to rely on the order, but after discussion with @costin changed that back to substrings match.

@imotov
Copy link
Copy Markdown
Contributor

imotov commented Mar 19, 2020

What's the motivation?

@aleksmaus
Copy link
Copy Markdown
Contributor Author

@imotov

  1. comparing the full maps makes the test too fragile dependent on other internal ES implementation changes
  2. traversing maps tree and match submaps makes the definition of the tests specs a bit more complex
    so the decision was the substrings match was adequate, sufficient and less fragile.
    @costin might chime in more on pros on cons.

Copy link
Copy Markdown
Member

@costin costin left a comment

Choose a reason for hiding this comment

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

LGTM

@aleksmaus aleksmaus force-pushed the improve/test_queryfolder_externalize branch from 2ffa9cf to fed316d Compare March 19, 2020 18:53
@aleksmaus
Copy link
Copy Markdown
Contributor Author

Rebased against the latest master to resolve the conflicts and force pushed.

@costin
Copy link
Copy Markdown
Member

costin commented Mar 19, 2020

@aleksmaus Use merging instead of rebase to preserve history.

@aleksmaus aleksmaus merged commit 1443ba6 into elastic:master Mar 20, 2020
aleksmaus added a commit to aleksmaus/elasticsearch that referenced this pull request Mar 20, 2020
"\"term\":{\"opcode\":{\"value\":3",
}
},
{"substringFunction", "process where substring(file_name, -4) == '.exe'",
Copy link
Copy Markdown
Member

@costin costin Mar 24, 2020

Choose a reason for hiding this comment

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

This test was skipped during externalization. Discovered that during backporting - I'm working on fixing it in master and 7.x

costin added a commit that referenced this pull request Mar 24, 2020
Adds back #53935 test removed accidentally through #53802
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/EQL EQL querying >test Issues or PRs that are addressing/adding tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants