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.

Sign up for GitHub

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

Jump to bottom

[SIEM] [Detections Engine] Import rules unit tests #57466

Merged
merged 17 commits into from
Feb 19, 2020
Merged

[SIEM] [Detections Engine] Import rules unit tests #57466

merged 17 commits into from
Feb 19, 2020

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Feb 12, 2020 •
edited
Loading

Summary

Added unit tests for import_rules_route. Moved out a small portion of route code into util to be tested and updated it so that a conflict error is reported now when two rules with the same rule-id are imported. Previously, when rules with the same rule-id were imported, no conflict was reported.

To test, tried importing 4 rules - 2 of them had the same rule-id, 1 had a syntax error.

When importing with overwrite=false:
Screen Shot 2020-02-15 at 10 33 10 AM
Screen Shot 2020-02-15 at 10 33 34 AM

When importing with overwrite=true:
Screen Shot 2020-02-15 at 10 35 26 AM
Screen Shot 2020-02-15 at 10 35 06 AM

Big thanks to @FrankHassanabad for his help figuring out how to properly mock multipart payloads!

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@yctercero
added import rules route file and basic outline of unit tests
f192c27
@yctercero
Merge branch 'master' of github.com:yctercero/kibana into import_rule…
f9b5246 
…s_unit_tests
@yctercero
Starting to fill out tests, running into issue with payload multipart…
98348d1 
… format errors
@yctercero
Import rule route unit tests written and passing
11a24cf
@yctercero
Merge branch 'master' of github.com:yctercero/kibana into import_rule…
b74c41e 
…s_unit_tests
@yctercero
Adjusted import rules route to report conflict when rules with the sa…
d386455 
…me rule-id are imported
@yctercero
moved part of code out into util and added tests. Adjusted unit and e…
5a3d544 
…2e tests
@yctercero
Merge branch 'master' of github.com:yctercero/kibana into import_rule…
f4af232 
…s_unit_tests
@yctercero
adjusted tests post merge of elastic#56814
ed4e3bd
@yctercero
Cleaned up util code
949c905
@yctercero
Merge branch 'master' of github.com:yctercero/kibana into import_rule…
62ee732 
…s_unit_tests
@yctercero yctercero marked this pull request as ready for review February 15, 2020 18:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

@yctercero yctercero self-assigned this Feb 15, 2020
@yctercero
Copy link
Contributor Author

@elasticmachine merge upstream

@yctercero yctercero changed the title Import rules unit tests [SIEM] [Detections Engine] Import rules unit tests Feb 18, 2020
FrankHassanabad
FrankHassanabad reviewed Feb 18, 2020
View reviewed changes
x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/rules/utils.ts
}, // using map (preserves ordering)
{
errors: new Map<string, BulkError>(),
rulesAcc: new Map<string, PromiseFromStreams>(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Really awesome these are typed now and you are passing this through the reducer.

This function is a bit tricky to read and reason through (some functions are always like that) but with the tests and the way it is written I think I understand it.

FrankHassanabad
FrankHassanabad approved these changes Feb 18, 2020
View reviewed changes
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.

Checked it out, test ran it by doing

  • fresh export, delete all custom rules, import
  • fresh export, import (overwrite true)
  • fresh export, import (no overwrite -- watch for errors)
  • fresh export, delete all custom rules, hand duplicate them in the text file, import them again

Thank you for the unit tests and cleaning things up this looks really good and much more type safe code for us.

@yctercero yctercero added v7.6.1 and removed v7.6.0 labels Feb 18, 2020
@dhurley14
Copy link
Contributor

@yctercero Is it possible to throw in a test that will result in the catch block being called? getIndexExists can throw an error so mocking that situation out would be good to have coverage for.

dhurley14
dhurley14 approved these changes Feb 18, 2020
View reviewed changes
Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

LGTM - my comment on adding a test for the catch block on the import test is optional. Nice job!

@yctercero
Copy link
Contributor Author

@yctercero Is it possible to throw in a test that will result in the catch block being called? getIndexExists can throw an error so mocking that situation out would be good to have coverage for.

@dhurley14 That's a good call. I added a test that throws on getIndexExists to test the catch block.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@yctercero yctercero merged commit ca3f27a into elastic:master Feb 19, 2020
@yctercero yctercero mentioned this pull request Feb 19, 2020
Merged
yctercero added a commit to yctercero/kibana that referenced this pull request Feb 19, 2020
@yctercero @elasticmachine
[SIEM] [Detections Engine] Import rules unit tests (elastic#57466)
ce09fdb 
* Added unit tests for detection engine import_rules_route and moved out small portion of import_rules_route into a util to be unit tested as well.

Co-authored-by: Elastic Machine <[email protected]>
@yctercero yctercero mentioned this pull request Feb 19, 2020
Merged
yctercero added a commit to yctercero/kibana that referenced this pull request Feb 19, 2020
@yctercero @elasticmachine
[SIEM] [Detections Engine] Import rules unit tests (elastic#57466)
0022965 
* Added unit tests for detection engine import_rules_route and moved out small portion of import_rules_route into a util to be unit tested as well.

Co-authored-by: Elastic Machine <[email protected]>
yctercero added a commit that referenced this pull request Feb 20, 2020
@yctercero @elasticmachine
[SIEM] [Detections Engine] Import rules unit tests (#57466) (#58016)
c103577 
* Added unit tests for detection engine import_rules_route and moved out small portion of import_rules_route into a util to be unit tested as well.

Co-authored-by: Elastic Machine <[email protected]>
FrankHassanabad pushed a commit that referenced this pull request Feb 20, 2020
@yctercero @elasticmachine
[7.6] [SIEM] [Detections Engine] Import rules unit tests (#57466) (#5…
d18b21b 
…8019)

* [SIEM] [Detections Engine] Import rules unit tests (#57466)

* Added unit tests for detection engine import_rules_route and moved out small portion of import_rules_route into a util to be unit tested as well.

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

* Updating tests to reflect state of 7.6. 7.7 and 8 include code from # 56814 that was not backported to 7.6

Co-authored-by: Elastic Machine <[email protected]>
@yctercero yctercero deleted the import_rules_unit_tests branch July 20, 2020 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FrankHassanabad FrankHassanabad FrankHassanabad approved these changes

@dhurley14 dhurley14 dhurley14 approved these changes

Assignees

@yctercero yctercero

Labels
release_note:enhancement Team:SIEM v7.6.1 v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yctercero @elasticmachine @dhurley14 @kibanamachine @FrankHassanabad