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

[Security Solution][Detections] Re-enable skipped integration test #87254

Merged
merged 4 commits into from
Jan 5, 2021

Conversation

rylnd
Copy link
Contributor

@rylnd rylnd commented Jan 4, 2021

Address #86709. This failure was due to this change in EQL. This was an expected change, and the solution here is simply to update the integration tests to reflect the new behavior.

For maintainers

This failure may still be present/intermittent, but it passes reliably
locally; trying again on CI to check status.
@rylnd rylnd added v8.0.0 v7.11.0 v7.12.0 Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Jan 4, 2021
@rylnd rylnd self-assigned this Jan 4, 2021
This should fail on the latest snapshot
@rylnd
Copy link
Contributor Author

rylnd commented Jan 5, 2021

After a little more investigation, these were still passing locally on master, which I believed was due to the lack of a new snapshot. These tests failed consistently locally on both the 7.11 and 7.x branches, which both have newer snapshots. Indeed, once a new 8.0.0 snapshot became available today, master failed similarly.

This failure was due to the behavior change in this commit, which was not present in the master snapshot until today. This change was discussed and so the integration failure was expected and merely needs to be updated to reflect the new filtering logic in EQL.

We were previously using what is effectively `results | head` to
retrieve the desired amount of results. The default behavior was changed
in elastic/elasticsearch#66387, which caused these tests to fail as
different results were returned over such a large dataset.
@rylnd
Copy link
Contributor Author

rylnd commented Jan 5, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47253 48013 +760

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@rylnd rylnd marked this pull request as ready for review January 5, 2021 20:44
@rylnd rylnd requested review from a team as code owners January 5, 2021 20:44
@rylnd rylnd added the release_note:skip Skip the PR/issue when compiling release notes label Jan 5, 2021
Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

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

LGTM

@rylnd rylnd merged commit e954306 into elastic:master Jan 5, 2021
@rylnd rylnd deleted the unskip_eql_integration_test branch January 5, 2021 22:08
rylnd added a commit to rylnd/kibana that referenced this pull request Jan 5, 2021
…lastic#87254)

* Re-enable test skipped due to temporary failure

This failure may still be present/intermittent, but it passes reliably
locally; trying again on CI to check status.

* Triggering build

This should fail on the latest snapshot

* Update EQL integration tests to reflect new default pipe

We were previously using what is effectively `results | head` to
retrieve the desired amount of results. The default behavior was changed
in elastic/elasticsearch#66387, which caused these tests to fail as
different results were returned over such a large dataset.

Co-authored-by: Kibana Machine <[email protected]>
rylnd added a commit to rylnd/kibana that referenced this pull request Jan 5, 2021
…lastic#87254)

* Re-enable test skipped due to temporary failure

This failure may still be present/intermittent, but it passes reliably
locally; trying again on CI to check status.

* Triggering build

This should fail on the latest snapshot

* Update EQL integration tests to reflect new default pipe

We were previously using what is effectively `results | head` to
retrieve the desired amount of results. The default behavior was changed
in elastic/elasticsearch#66387, which caused these tests to fail as
different results were returned over such a large dataset.

Co-authored-by: Kibana Machine <[email protected]>
rylnd added a commit that referenced this pull request Jan 6, 2021
…87254) (#87397)

* Re-enable test skipped due to temporary failure

This failure may still be present/intermittent, but it passes reliably
locally; trying again on CI to check status.

* Triggering build

This should fail on the latest snapshot

* Update EQL integration tests to reflect new default pipe

We were previously using what is effectively `results | head` to
retrieve the desired amount of results. The default behavior was changed
in elastic/elasticsearch#66387, which caused these tests to fail as
different results were returned over such a large dataset.

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
rylnd added a commit that referenced this pull request Jan 6, 2021
…87254) (#87396)

* Re-enable test skipped due to temporary failure

This failure may still be present/intermittent, but it passes reliably
locally; trying again on CI to check status.

* Triggering build

This should fail on the latest snapshot

* Update EQL integration tests to reflect new default pipe

We were previously using what is effectively `results | head` to
retrieve the desired amount of results. The default behavior was changed
in elastic/elasticsearch#66387, which caused these tests to fail as
different results were returned over such a large dataset.

Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.11.0 v7.12.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants