[Security Solution][Detections] Adds exception modal tests #74596

dplumlee · 2020-08-06T20:28:21Z

Summary

Adds jest tests for the add and edit exception modals to cover all current state variations based on props

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2020-08-14T19:28:41Z

Pinging @elastic/siem (Team:SIEM)

FrankHassanabad · 2020-08-17T15:10:35Z

...plugins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx

@@ -259,7 +259,8 @@ export const EditExceptionModal = memo(function EditExceptionModal({
 <ModalBodySection>
 <EuiFormRow fullWidth>
 <EuiCheckbox
- id="close-alert-on-add-add-exception-checkbox"
+ data-test-subj="close-alert-on-add-edit-exception-checkbox"
+ id="close-alert-on-add-edit-exception-checkbox"


I don't think you need this id here do you? Looks like above you're using data-test-subj successfully?

i believe the id is a required field for the EuiCheckbox, would spit out a console warning and potentially cause usage problems if not there.

Yeah looking at the docs and searching around this looks like it is required.

FrankHassanabad · 2020-08-17T15:14:42Z

...ins/security_solution/public/common/components/exceptions/add_exception_modal/index.test.tsx

+ ruleIndices={['filebeat-*']}
+ ruleName={ruleName}
+ exceptionListType={'endpoint'}
+ onCancel={() => {}}


nit: If you search through the code base most of the time people across the plugins and code base do this when stubbing out no operations:

jest.fn()

Makes it easier as a stub that it won't blow up later on as it's more accepting of what can call it, etc...

For example:

onCancel={jest.fn()}

FrankHassanabad · 2020-08-17T15:26:00Z

...ins/security_solution/public/common/components/exceptions/add_exception_modal/index.test.tsx

+ wrapper.find('button[data-test-subj="add-exception-confirm-button"]').getDOMNode()
+ ).not.toBeDisabled();
+ });
+


nit: Seems weird to have a new line break here but not between the describe blocks or other it blocks?

I would change the newlines you have to be consistent. The new lines between test/it/describe blocks seem to be what most people do everywhere across plugins. Some people don't though (which is not a biggie to me which way to choose as long as consistency within a single file is maintained.

For a "science based reason" if I was going to make the argument of using new line blocks between it blocks and it/describe blocks is that spacing is a "pre-attentive attribute" meaning that when reading blocks of code we will naturally group things based on spacial distance between things before having to think consciously so it makes it easier to read them. Several posts about this and the other preattentive attributes is out there and good reads IMHO.

http://daydreamingnumbers.com/blog/preattentive-attributes-example/

makes kind of sense when you think about how the IDE adds color in some aspects to make things stand out.

FrankHassanabad · 2020-08-17T15:28:09Z

x-pack/plugins/security_solution/public/common/components/exceptions/add_exception_comments.tsx

@@ -112,7 +112,7 @@ export const AddExceptionComments = memo(function AddExceptionComments({
 <EuiFlexItem grow={1}>
 <EuiTextArea
 placeholder={i18n.ADD_COMMENT_PLACEHOLDER}
- aria-label="Use aria labels when no actual label is in use"
+ aria-label="Comment Input"


Nice catch! 👍

kibanamachine · 2020-08-17T18:02:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4cf0023

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	7.2MB	+439.0B	7.2MB

History

💚 Build #68254 succeeded 94f0ca3
💔 Build #68247 failed 1695699
💚 Build #68229 succeeded 2346c39
💚 Build #68021 succeeded 3a57767
💚 Build #67813 succeeded 4452cd7

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

FrankHassanabad

LGTM, thanks for the good catches on aria-label, explanations on the id, and the unit tests

peluja1012

Thanks for adding this coverage!

…4596)

* master: Skip failing test in CI (elastic#75266) [Task Manager] time out work when it overruns in poller (elastic#74980) [Drilldowns] misc improvements & fixes (elastic#75276) Small README note on bumping memory for builds (elastic#75247) [Security Solution][Detections] Adds exception modal tests (elastic#74596) [Dashboard] Sample data link does not work (elastic#75262) [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905) [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166) convert processor labels to sentence case (elastic#75278) [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160) Clarify no documents error message when filtering by is_training (elastic#75227) [Lens] Fix crash when two layers xychart switches to pie (elastic#75267) [Observability Homepage] Fix console error because of side effect (elastic#75258) [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146) [ML] Functional tests - re-activate DFA test suites (elastic#75257) GS providers improvements (elastic#75174) [Visualize] First version of by-value visualize editor (elastic#72256)

…75304)

…emove-header * saved-objects/version-on-create: (59 commits) remove version when loading sample data omit version from SO import/export Skip failing test in CI (elastic#75266) [Task Manager] time out work when it overruns in poller (elastic#74980) [Drilldowns] misc improvements & fixes (elastic#75276) Small README note on bumping memory for builds (elastic#75247) [Security Solution][Detections] Adds exception modal tests (elastic#74596) Revert "Revert "added missing core docs"" Revert "Revert "added version to saved object bulk creation"" [Dashboard] Sample data link does not work (elastic#75262) [Dashboard First] Unlink from Library Action With ReferenceOrValueEmbeddable (elastic#74905) [Form lib] Fix issue where serializer on fields are called on every change (elastic#75166) convert processor labels to sentence case (elastic#75278) [Monaco] Refactor the way XJSON grammar checker gets registered (elastic#75160) Clarify no documents error message when filtering by is_training (elastic#75227) [Lens] Fix crash when two layers xychart switches to pie (elastic#75267) [Observability Homepage] Fix console error because of side effect (elastic#75258) [Usage Collection] Add `legacy=true` option to the /api/stats request in the docs (elastic#75146) [ML] Functional tests - re-activate DFA test suites (elastic#75257) GS providers improvements (elastic#75174) ...

elasticmachine · 2021-09-23T14:27:42Z

Pinging @elastic/security-solution (Team: SecuritySolution)

dplumlee force-pushed the exceptions-modal-tests branch 2 times, most recently from 7ebdf20 to 4b54bdb Compare August 10, 2020 18:19

dplumlee added Feature:Detection Rules Anything related to Security Solution's Detection Rules release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.10.0 v8.0.0 labels Aug 14, 2020

dplumlee marked this pull request as ready for review August 14, 2020 19:28

dplumlee requested review from a team as code owners August 14, 2020 19:28

dplumlee force-pushed the exceptions-modal-tests branch from 9c1524b to 2346c39 Compare August 14, 2020 19:29

FrankHassanabad reviewed Aug 17, 2020

View reviewed changes

dplumlee added 8 commits August 17, 2020 12:05

adds modal tests

2b458dc

adds comments to tests

8774520

splits up into code blocks

ecc0dea

switches tests to use internal mock

a17eaa6

mocks onchange

18ab207

adds more tests

73fd344

fixes typecheck

5ef03ec

addresses comments

4cf0023

dplumlee force-pushed the exceptions-modal-tests branch from 94f0ca3 to 4cf0023 Compare August 17, 2020 16:20

FrankHassanabad approved these changes Aug 17, 2020

View reviewed changes

peluja1012 approved these changes Aug 18, 2020

View reviewed changes

dplumlee merged commit bfdaacd into elastic:master Aug 18, 2020

dplumlee deleted the exceptions-modal-tests branch August 18, 2020 15:20

dplumlee mentioned this pull request Aug 18, 2020

[7.x] [Security Solution][Detections] Adds exception modal tests (#74596) #75304

Merged

dplumlee added a commit to dplumlee/kibana that referenced this pull request Aug 18, 2020

[Security Solution][Detections] Adds exception modal tests (elastic#7…

eddf9cc

…4596)

dplumlee added a commit that referenced this pull request Aug 18, 2020

[Security Solution][Detections] Adds exception modal tests (#74596) (#…

a508c65

…75304)

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Adds exception modal tests #74596

[Security Solution][Detections] Adds exception modal tests #74596

dplumlee commented Aug 6, 2020 •

edited

Loading

elasticmachine commented Aug 14, 2020

FrankHassanabad Aug 17, 2020

dplumlee Aug 17, 2020

FrankHassanabad Aug 17, 2020

FrankHassanabad Aug 17, 2020

FrankHassanabad Aug 17, 2020

FrankHassanabad Aug 17, 2020

kibanamachine commented Aug 17, 2020

FrankHassanabad left a comment

peluja1012 left a comment

elasticmachine commented Sep 23, 2021

[Security Solution][Detections] Adds exception modal tests #74596

[Security Solution][Detections] Adds exception modal tests #74596

Conversation

dplumlee commented Aug 6, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Aug 14, 2020

FrankHassanabad Aug 17, 2020

Choose a reason for hiding this comment

dplumlee Aug 17, 2020

Choose a reason for hiding this comment

FrankHassanabad Aug 17, 2020

Choose a reason for hiding this comment

FrankHassanabad Aug 17, 2020

Choose a reason for hiding this comment

FrankHassanabad Aug 17, 2020

Choose a reason for hiding this comment

FrankHassanabad Aug 17, 2020

Choose a reason for hiding this comment

kibanamachine commented Aug 17, 2020

💚 Build Succeeded

Build metrics

async chunks size

History

FrankHassanabad left a comment

Choose a reason for hiding this comment

peluja1012 left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 23, 2021

dplumlee commented Aug 6, 2020 •

edited

Loading