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 Solutions] Fixes flake with cypress tests #97329

Merged
merged 4 commits into from
Apr 16, 2021

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Apr 15, 2021

Summary

Fixes some recent flakeyness with Cypress tests

  • Adds cypress.pipe() on button clicks around the area of flakes
  • Adds an alerting threshold to the utilities so we can wait for when an exact number of alerts are available on a page
  • Changes the alerts to not run again with 10 seconds, because if a test takes longer than 10 seconds, the rule can run a second time which can invalidate some of the text when running checks when timeline or other components update on their button clicks.

Checklist

@FrankHassanabad FrankHassanabad requested a review from a team as a code owner April 15, 2021 21:45
@FrankHassanabad FrankHassanabad self-assigned this Apr 15, 2021
@FrankHassanabad FrankHassanabad added v8.0.0 v7.13.0 auto-backport Deprecated - use backport:version if exact versions are needed Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes and removed Feature:Alerting labels Apr 15, 2021
@@ -332,5 +332,5 @@ export const editedRule = {
export const expectedExportedRule = (ruleResponse: Cypress.Response) => {
const jsonrule = ruleResponse.body;

return `{"id":"${jsonrule.id}","updated_at":"${jsonrule.updated_at}","updated_by":"elastic","created_at":"${jsonrule.created_at}","created_by":"elastic","name":"${jsonrule.name}","tags":[],"interval":"10s","enabled":false,"description":"${jsonrule.description}","risk_score":${jsonrule.risk_score},"severity":"${jsonrule.severity}","output_index":".siem-signals-default","author":[],"false_positives":[],"from":"now-17520h","rule_id":"rule_testing","max_signals":100,"risk_score_mapping":[],"severity_mapping":[],"threat":[],"to":"now","references":[],"version":1,"exceptions_list":[],"immutable":false,"type":"query","language":"kuery","index":["exceptions-*"],"query":"${jsonrule.query}","throttle":"no_actions","actions":[]}\n{"exported_count":1,"missing_rules":[],"missing_rules_count":0}\n`;
return `{"id":"${jsonrule.id}","updated_at":"${jsonrule.updated_at}","updated_by":"elastic","created_at":"${jsonrule.created_at}","created_by":"elastic","name":"${jsonrule.name}","tags":[],"interval":"100m","enabled":false,"description":"${jsonrule.description}","risk_score":${jsonrule.risk_score},"severity":"${jsonrule.severity}","output_index":".siem-signals-default","author":[],"false_positives":[],"from":"now-17520h","rule_id":"rule_testing","max_signals":100,"risk_score_mapping":[],"severity_mapping":[],"threat":[],"to":"now","references":[],"version":1,"exceptions_list":[],"immutable":false,"type":"query","language":"kuery","index":["exceptions-*"],"query":"${jsonrule.query}","throttle":"no_actions","actions":[]}\n{"exported_count":1,"missing_rules":[],"missing_rules_count":0}\n`;
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't the interval here ${jsonrule.interval} since this isn't hardcoded anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That I don't know. There's a lot of parts of this that don't use it. I think that's intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guess what I'm saying is I don't want to change that part right now unless I need to ;-)

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

LGTM! 👍 Thanks for discussing the changes with @yctercero and I. This should stomp out a whole class of flakes with regards to waiting on alerts. Thanks @FrankHassanabad! 🙂

rule: CustomRule,
ruleId = '1',
interval = '100m',
maxSignals = 500
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nit: we could also leave this at default 100 and just change the max_signals test to be 500. Not sure if it's that big a deal. I just figure most aren't going to be playing around with the max_signals knob.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's true. I don't think it's that big of a deal. If we start refactoring code and it gets worse or we hit more flakes then yeah I will for sure do it. Thanks for the suggestion.

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

LGTM! Super nit comment with no change needed, just a thought. Thanks so much for tackling these.

@FrankHassanabad
Copy link
Contributor Author

@elasticmachine merge upstream

@FrankHassanabad
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @FrankHassanabad

@FrankHassanabad FrankHassanabad merged commit 721f4b5 into elastic:master Apr 16, 2021
@FrankHassanabad FrankHassanabad deleted the fix-flake branch April 16, 2021 19:52
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 16, 2021
## Summary

Fixes some recent flakeyness with Cypress tests
* Adds cypress.pipe() on button clicks around the area of flakes
* Adds an alerting threshold to the utilities so we can wait for when an exact number of alerts are available on a page
* Changes the alerts to not run again with 10 seconds, because if a test takes longer than 10 seconds, the rule can run a second time which can invalidate some of the text when running checks when timeline or other components update on their button clicks.

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Apr 16, 2021
## Summary

Fixes some recent flakeyness with Cypress tests
* Adds cypress.pipe() on button clicks around the area of flakes
* Adds an alerting threshold to the utilities so we can wait for when an exact number of alerts are available on a page
* Changes the alerts to not run again with 10 seconds, because if a test takes longer than 10 seconds, the rule can run a second time which can invalidate some of the text when running checks when timeline or other components update on their button clicks.

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 19, 2021
…te-legacy-es-client

* 'master' of github.com:elastic/kibana: (102 commits)
  [Exploratory view] integerate page views to exploratory view (elastic#97258)
  Fix typo in license_api_guard README name and import http server mocks from public interface (elastic#97334)
  Avoid mutating KQL query when validating it (elastic#97081)
  Add description as title on tag badge (elastic#97109)
  Remove legacy ES client usages in `home` and `xpack_legacy` (elastic#97359)
  [Fleet] Finer-grained error information from install/upgrade API (elastic#95649)
  Rule registry bundle size (elastic#97251)
  [Partial Results] Move other bucket into Search Source (elastic#96384)
  [Dashboard] Makes lens default editor for creating new panels (elastic#96181)
  skip flaky suite (elastic#97387)
  [Asset Management] Agent picker follow up (elastic#97357)
  skip flaky suite (elastic#97382)
  [Security Solutions] Fixes flake with cypress tests (elastic#97329)
  skip flaky suite (elastic#97355)
  Skip test to try and stabilize master
  minimize number of so fild asserted in tests. it creates flakines when implementation details change (elastic#97374)
  [Search Sessions] Client side search cache (elastic#92439)
  [SavedObjects] Add aggregations support (elastic#96292)
  [Reporting] Remove legacy elasticsearch client usage from the reporting plugin (elastic#97184)
  [kbnClient] fix basePath handling and export reponse type (elastic#97277)
  ...

# Conflicts:
#	x-pack/plugins/watcher/server/lib/license_pre_routing_factory/license_pre_routing_factory.ts
#	x-pack/plugins/watcher/server/plugin.ts
#	x-pack/plugins/watcher/server/routes/api/indices/register_get_route.ts
#	x-pack/plugins/watcher/server/routes/api/license/register_refresh_route.ts
#	x-pack/plugins/watcher/server/routes/api/register_list_fields_route.ts
#	x-pack/plugins/watcher/server/routes/api/register_load_history_route.ts
#	x-pack/plugins/watcher/server/routes/api/settings/register_load_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/action/register_acknowledge_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/register_activate_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/register_deactivate_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/register_delete_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/register_execute_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/register_history_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/register_load_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/register_save_route.ts
#	x-pack/plugins/watcher/server/routes/api/watch/register_visualize_route.ts
#	x-pack/plugins/watcher/server/routes/api/watches/register_delete_route.ts
#	x-pack/plugins/watcher/server/routes/api/watches/register_list_route.ts
#	x-pack/plugins/watcher/server/shared_imports.ts
#	x-pack/plugins/watcher/server/types.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes v7.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants