-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Fix EQL cypress tests #80440
Conversation
Pinging @elastic/siem (Team:SIEM) |
Moving back to Draft as it looks like there were some suspicious failures on the 7.x branch. Going to try and repro/fix locally. |
These _should_ be fixed with the latest ES on master, but let's see if CI disagrees.
Occasionally our tests hit a scenario where the rule has executed (its status is "succeeded"), but the generated alerts have not populated in the same time frame. In this case the test fails oddly, saying that the "alert count" element is not there when it is. I attempted to improve the error message by using a .should() with a callback, but that lead to even stranger behavior as the .should() would fail once (expected), and then not be able to find the element a second time. :( So we instead focus on fixing the real problem, here: wait until alerts populate (have a non-zero count) before performing the assertion. Because the page will not update automatically, we can't rely on cypress' retryability and must instead assert, click Refresh, and assert again, much like we're doing while waiting for the rule to execute. And like `waitForTheRuleToBeExecuted`, we're using a while loop that has no guarantee of ever exiting :(
* Uses should with a text matcher instead of using invoke('text') * Use of not.equal between a string and an element may have been a false positive
We have a few tasks that require polling for some background work to be completed. The basic form is: assert the byproduct, or refresh the page and try again. We were previously doing this with a while loop, which was not guaranteed to ever complete, leading to cryptic failures if the process ever hung. Instead, this implements a safer polling mechanism with a definite termination similar to the cypress-wait-until plugin.
* Do not automatically refresh the page * This is only necessary if we're not in the state we need. The `waitFor` helper functions automatically reload whatever needs to be reloaded, so we're delegating this task to them. * Ensure we wait for alerts to be nonzero before our assertion * Otherwise we get some strange behavior around this field's availability; see previous commits
54cb278
to
dff4645
Compare
@elasticmachine merge upstream |
@MadameSheema I ended up implementing a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of thanks for this fix @rylnd! Great work :)
Threat Match Rules introduced an additional query input, causing our CUSTOM_QUERY_INPUT to be ambiguous. However, instead of failing due to the ambiguity, the behavior of cypress seems to be to pass! While I haven't yet tracked down the cause of these false positives, disambiguating these selectors is the immediate fix.
@MadameSheema I think that the behavior causing the above test failures is also present on master; however on master it leads to a passing test! I believe I've fixed the immediate problem in 24748c9, but we need to diagnose and prevent whatever is causing these false positives in the abstract. |
Ok, I figured out the cause of the false positive: native promises. I found this issue that seemed to describe the behavior we were seeing, and sure enough, commenting out our use of Because |
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
* Unskip EQL tests These _should_ be fixed with the latest ES on master, but let's see if CI disagrees. * Wait until alerts have populated on Rule Details Occasionally our tests hit a scenario where the rule has executed (its status is "succeeded"), but the generated alerts have not populated in the same time frame. In this case the test fails oddly, saying that the "alert count" element is not there when it is. I attempted to improve the error message by using a .should() with a callback, but that lead to even stranger behavior as the .should() would fail once (expected), and then not be able to find the element a second time. :( So we instead focus on fixing the real problem, here: wait until alerts populate (have a non-zero count) before performing the assertion. Because the page will not update automatically, we can't rely on cypress' retryability and must instead assert, click Refresh, and assert again, much like we're doing while waiting for the rule to execute. And like `waitForTheRuleToBeExecuted`, we're using a while loop that has no guarantee of ever exiting :( * More robust cypress assertions * Uses should with a text matcher instead of using invoke('text') * Use of not.equal between a string and an element may have been a false positive * Perform cypress loops in a manner guaranteed to exit We have a few tasks that require polling for some background work to be completed. The basic form is: assert the byproduct, or refresh the page and try again. We were previously doing this with a while loop, which was not guaranteed to ever complete, leading to cryptic failures if the process ever hung. Instead, this implements a safer polling mechanism with a definite termination similar to the cypress-wait-until plugin. * Update other specs that are asserting on alerts * Do not automatically refresh the page * This is only necessary if we're not in the state we need. The `waitFor` helper functions automatically reload whatever needs to be reloaded, so we're delegating this task to them. * Ensure we wait for alerts to be nonzero before our assertion * Otherwise we get some strange behavior around this field's availability; see previous commits * Remove unused import * Fix false positive in Rule Creation specs Threat Match Rules introduced an additional query input, causing our CUSTOM_QUERY_INPUT to be ambiguous. However, instead of failing due to the ambiguity, the behavior of cypress seems to be to pass! While I haven't yet tracked down the cause of these false positives, disambiguating these selectors is the immediate fix. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts
* Unskip EQL tests These _should_ be fixed with the latest ES on master, but let's see if CI disagrees. * Wait until alerts have populated on Rule Details Occasionally our tests hit a scenario where the rule has executed (its status is "succeeded"), but the generated alerts have not populated in the same time frame. In this case the test fails oddly, saying that the "alert count" element is not there when it is. I attempted to improve the error message by using a .should() with a callback, but that lead to even stranger behavior as the .should() would fail once (expected), and then not be able to find the element a second time. :( So we instead focus on fixing the real problem, here: wait until alerts populate (have a non-zero count) before performing the assertion. Because the page will not update automatically, we can't rely on cypress' retryability and must instead assert, click Refresh, and assert again, much like we're doing while waiting for the rule to execute. And like `waitForTheRuleToBeExecuted`, we're using a while loop that has no guarantee of ever exiting :( * More robust cypress assertions * Uses should with a text matcher instead of using invoke('text') * Use of not.equal between a string and an element may have been a false positive * Perform cypress loops in a manner guaranteed to exit We have a few tasks that require polling for some background work to be completed. The basic form is: assert the byproduct, or refresh the page and try again. We were previously doing this with a while loop, which was not guaranteed to ever complete, leading to cryptic failures if the process ever hung. Instead, this implements a safer polling mechanism with a definite termination similar to the cypress-wait-until plugin. * Update other specs that are asserting on alerts * Do not automatically refresh the page * This is only necessary if we're not in the state we need. The `waitFor` helper functions automatically reload whatever needs to be reloaded, so we're delegating this task to them. * Ensure we wait for alerts to be nonzero before our assertion * Otherwise we get some strange behavior around this field's availability; see previous commits * Remove unused import * Fix false positive in Rule Creation specs Threat Match Rules introduced an additional query input, causing our CUSTOM_QUERY_INPUT to be ambiguous. However, instead of failing due to the ambiguity, the behavior of cypress seems to be to pass! While I haven't yet tracked down the cause of these false positives, disambiguating these selectors is the immediate fix. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
* Unskip EQL tests These _should_ be fixed with the latest ES on master, but let's see if CI disagrees. * Wait until alerts have populated on Rule Details Occasionally our tests hit a scenario where the rule has executed (its status is "succeeded"), but the generated alerts have not populated in the same time frame. In this case the test fails oddly, saying that the "alert count" element is not there when it is. I attempted to improve the error message by using a .should() with a callback, but that lead to even stranger behavior as the .should() would fail once (expected), and then not be able to find the element a second time. :( So we instead focus on fixing the real problem, here: wait until alerts populate (have a non-zero count) before performing the assertion. Because the page will not update automatically, we can't rely on cypress' retryability and must instead assert, click Refresh, and assert again, much like we're doing while waiting for the rule to execute. And like `waitForTheRuleToBeExecuted`, we're using a while loop that has no guarantee of ever exiting :( * More robust cypress assertions * Uses should with a text matcher instead of using invoke('text') * Use of not.equal between a string and an element may have been a false positive * Perform cypress loops in a manner guaranteed to exit We have a few tasks that require polling for some background work to be completed. The basic form is: assert the byproduct, or refresh the page and try again. We were previously doing this with a while loop, which was not guaranteed to ever complete, leading to cryptic failures if the process ever hung. Instead, this implements a safer polling mechanism with a definite termination similar to the cypress-wait-until plugin. * Update other specs that are asserting on alerts * Do not automatically refresh the page * This is only necessary if we're not in the state we need. The `waitFor` helper functions automatically reload whatever needs to be reloaded, so we're delegating this task to them. * Ensure we wait for alerts to be nonzero before our assertion * Otherwise we get some strange behavior around this field's availability; see previous commits * Remove unused import * Fix false positive in Rule Creation specs Threat Match Rules introduced an additional query input, causing our CUSTOM_QUERY_INPUT to be ambiguous. However, instead of failing due to the ambiguity, the behavior of cypress seems to be to pass! While I haven't yet tracked down the cause of these false positives, disambiguating these selectors is the immediate fix. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts
* Unskip EQL tests These _should_ be fixed with the latest ES on master, but let's see if CI disagrees. * Wait until alerts have populated on Rule Details Occasionally our tests hit a scenario where the rule has executed (its status is "succeeded"), but the generated alerts have not populated in the same time frame. In this case the test fails oddly, saying that the "alert count" element is not there when it is. I attempted to improve the error message by using a .should() with a callback, but that lead to even stranger behavior as the .should() would fail once (expected), and then not be able to find the element a second time. :( So we instead focus on fixing the real problem, here: wait until alerts populate (have a non-zero count) before performing the assertion. Because the page will not update automatically, we can't rely on cypress' retryability and must instead assert, click Refresh, and assert again, much like we're doing while waiting for the rule to execute. And like `waitForTheRuleToBeExecuted`, we're using a while loop that has no guarantee of ever exiting :( * More robust cypress assertions * Uses should with a text matcher instead of using invoke('text') * Use of not.equal between a string and an element may have been a false positive * Perform cypress loops in a manner guaranteed to exit We have a few tasks that require polling for some background work to be completed. The basic form is: assert the byproduct, or refresh the page and try again. We were previously doing this with a while loop, which was not guaranteed to ever complete, leading to cryptic failures if the process ever hung. Instead, this implements a safer polling mechanism with a definite termination similar to the cypress-wait-until plugin. * Update other specs that are asserting on alerts * Do not automatically refresh the page * This is only necessary if we're not in the state we need. The `waitFor` helper functions automatically reload whatever needs to be reloaded, so we're delegating this task to them. * Ensure we wait for alerts to be nonzero before our assertion * Otherwise we get some strange behavior around this field's availability; see previous commits * Remove unused import * Fix false positive in Rule Creation specs Threat Match Rules introduced an additional query input, causing our CUSTOM_QUERY_INPUT to be ambiguous. However, instead of failing due to the ambiguity, the behavior of cypress seems to be to pass! While I haven't yet tracked down the cause of these false positives, disambiguating these selectors is the immediate fix. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> # Conflicts: # x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts (cherry picked from commit 3fc1f8c)
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This EQL suite was previously skipped. While these were skipped, a bug was introduced in elasticsearch that broke EQL rules. This bug should be fixed in elastic/elasticsearch#63573, which should fix these tests, but let's see if CI disagrees.
Checklist
For maintainers