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

EQL: Add integration tests harness to test EQL feature parity with original implementation (#52248) #52675

Merged

Conversation

aleksmaus
Copy link
Contributor

This is a backport to 7.x.

The tests use the original test queries from
https://github.com/endgameinc/eql/blob/master/eql/etc/test_queries.toml
for EQL implementation correctness validation.
The file test_queries_unsupported.toml serves as a "blacklist" for the
queries that we do not support. Currently all of the queries are
blacklisted. Over the time the expectation is to eventually have an
empty "blacklist" when all of the queries are fully supported.

The tests use the original test vector from
https://raw.githubusercontent.com/endgameinc/eql/master/eql/etc/test_data.json.

Only one EQL and the response is stubbed for now to match the expected
output from that query. This part would need some tweaking after EQL is
fully wired.

Related to #49581

…iginal implementation (elastic#52248)

The tests use the original test queries from
https://github.com/endgameinc/eql/blob/master/eql/etc/test_queries.toml
for EQL implementation correctness validation.
The file test_queries_unsupported.toml serves as a "blacklist" for the
queries that we do not support. Currently all of the queries are
blacklisted. Over the time the expectation is to eventually have an
empty "blacklist" when all of the queries are fully supported.

The tests use the original test vector from
https://raw.githubusercontent.com/endgameinc/eql/master/eql/etc/test_data.json.

Only one EQL and the response is stubbed for now to match the expected
output from that query. This part would need some tweaking after EQL is
fully wired.

Related to elastic#49581
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

@astefan astefan left a comment

Choose a reason for hiding this comment

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

LGTM. Left one comment, though.


@BeforeClass
public static void checkForSnapshot() {
assumeTrue("Only works on snapshot builds for now", Build.CURRENT.isSnapshot());
Copy link
Contributor

Choose a reason for hiding this comment

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

A previous comment from @imotov in the original PR - https://github.com/elastic/elasticsearch/pull/52248/files#r379168077 - was ignored. I'm curious what was the conclusion back then regarding this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Missed this one. We still have the same check for EqlRestIT as well. No harm during development of this feature. Can be removed later when the feature is ready for release. @imotov let me know if you have a strong opinion on this.

@aleksmaus aleksmaus merged commit a7bdb0b into elastic:7.x Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants