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] [Cases] Case API tests #65777

Merged
merged 11 commits into from
May 11, 2020
Merged

[SIEM] [Cases] Case API tests #65777

merged 11 commits into from
May 11, 2020

Conversation

stephmilovic
Copy link
Contributor

Summary

Long overdue API integration tests for the case API

Checklist

Delete any items that are not applicable to this PR.

@stephmilovic stephmilovic added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.9.0 labels May 7, 2020
@stephmilovic stephmilovic requested review from XavierM and cnasikas May 7, 2020 21:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

cnasikas
cnasikas reviewed May 8, 2020
View reviewed changes
x-pack/test/case_api_integration/basic/tests/cases/comments/delete_comment.ts
await deleteCasesUserActions(es);
});

it('should delete a comment', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add two tests that check error handling: 1) test if the comment does not exist 2) test if the comment does not belong to the specific case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we're actually not allowing this in the UI and could remove the endpoint entirely.... it's lucky to get one test

Copy link
Contributor

Choose a reason for hiding this comment

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

That's true but users can call the API directly if they want to, so we should test the API independently of the UI, to make sure that the API is doing what we expect without considering the UI.

cnasikas
cnasikas reviewed May 8, 2020
View reviewed changes
x-pack/test/case_api_integration/basic/tests/cases/find_cases.ts
});
});

it('filters by tags', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should at a test that filters by reporters or one that filters by both tags and reporters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we need to figure out how to post from multiple users in the func tests to do this

cnasikas
cnasikas reviewed May 8, 2020
View reviewed changes
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Very good job creating those integration tests! Thank you!

Some comments:

  1. I think we should test unhappy paths to the tests.
  2. User actions should be tested more. We can do this by either checking that the user action has been created on a specific route like (post_case) or by checking all possible user actions in get_all_user_actions test
  3. Some of the routes depend of the connector id in some way. Shouldn't we test that?

@stephmilovic stephmilovic changed the title [SIEM] [Cases] Case API tests [SIEM] [Cases] [skip-ci] Case API tests May 8, 2020
@stephmilovic
Copy link
Contributor Author

skipping CI while i work on the user actions tests

@stephmilovic
Copy link
Contributor Author

Very good job creating those integration tests! Thank you!

Some comments:

  1. I think we should test unhappy paths to the tests.
  2. User actions should be tested more. We can do this by either checking that the user action has been created on a specific route like (post_case) or by checking all possible user actions in get_all_user_actions test
  3. Some of the routes depend of the connector id in some way. Shouldn't we test that?
  1. Done
  2. Done
  3. We are testing for the presence of connector_id on every route where it is. It defaults to 'none'. I also added a test in user actions that checks for the updated value

@stephmilovic stephmilovic changed the title [SIEM] [Cases] [skip-ci] Case API tests [SIEM] [Cases] Case API tests May 8, 2020
@stephmilovic
Copy link
Contributor Author

@elasticmachine merge upstream

cnasikas
cnasikas approved these changes May 11, 2020
View reviewed changes
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Thank you for making all the changes!! The tests in this PR are amazing!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@stephmilovic stephmilovic merged commit 2082648 into elastic:master May 11, 2020
@stephmilovic stephmilovic deleted the case-api-tests branch May 11, 2020 20:27
@stephmilovic stephmilovic mentioned this pull request May 11, 2020
Merged
stephmilovic added a commit to stephmilovic/kibana that referenced this pull request May 11, 2020
@stephmilovic
[SIEM] [Cases] Case API tests (elastic#65777)
58fc469
stephmilovic added a commit that referenced this pull request May 12, 2020
@stephmilovic
[SIEM] [Cases] Case API tests (#65777) (#66117)
f7eb711
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 12, 2020
@gmmorris
Merge branch 'master' into alerting/lazy-load-components
073a924 
* master: (46 commits)
  [Drilldowns][chore] Remove some any's from components. Remove `PlaceContext` from components (elastic#65854)
  [functional/services] import By/until from module (elastic#66015)
  [Drilldowns][IE] fix welcome bar layout in IE (elastic#65676)
  Inspect action shows on dashboard for every chart (elastic#65998)
  Fix heigt calc in calc issue for ie11 (elastic#66010)
  [Flights] Delay Bucket - Error notification on opening sample visualization (elastic#66028)
  [SIEM] [Security] unified code structure phase 0 (elastic#65965)
  [Maps] Organize layers into subfolders (elastic#65513)
  skip flaky suite (elastic#59849)
  Cleanup prefill and edit flow. (elastic#66105)
  Fix major severity service map ring colors (elastic#66124)
  [DOCS] Improves formatting in action types (elastic#65932)
  [DOCS] APM Agent config: Setting values must be string (elastic#65875)
  Change default cert age limit value. (elastic#65918)
  [DOCS] Removed saved object options (elastic#66072)
  [SIEM] [Cases] Case API tests (elastic#65777)
  Add example of of local plugin installation (elastic#65986)
  skip flaky suite (elastic#65741)
  [SIEM][Detections] Restrict ML rule modification to ML Admins (elastic#65583)
  [Reporting/Test] Add Functional test for download CSV (elastic#65401)
  ...
jloleysens added a commit that referenced this pull request May 12, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into feature/pipel…
6316956 
…ine-editor

* 'master' of github.com:elastic/kibana: (37 commits)
  [APM] Correct relative paths in scripts (#66159)
  [Uptime] Enable deselection of stale filters (#65523)
  [Drilldowns][chore] Remove some any's from components. Remove `PlaceContext` from components (#65854)
  [functional/services] import By/until from module (#66015)
  [Drilldowns][IE] fix welcome bar layout in IE (#65676)
  Inspect action shows on dashboard for every chart (#65998)
  Fix heigt calc in calc issue for ie11 (#66010)
  [Flights] Delay Bucket - Error notification on opening sample visualization (#66028)
  [SIEM] [Security] unified code structure phase 0 (#65965)
  [Maps] Organize layers into subfolders (#65513)
  skip flaky suite (#59849)
  Cleanup prefill and edit flow. (#66105)
  Fix major severity service map ring colors (#66124)
  [DOCS] Improves formatting in action types (#65932)
  [DOCS] APM Agent config: Setting values must be string (#65875)
  Change default cert age limit value. (#65918)
  [DOCS] Removed saved object options (#66072)
  [SIEM] [Cases] Case API tests (#65777)
  Add example of of local plugin installation (#65986)
  skip flaky suite (#65741)
  ...
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cnasikas cnasikas cnasikas approved these changes

@XavierM XavierM Awaiting requested review from XavierM

Assignees
No one assigned
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stephmilovic @elasticmachine @kibanamachine @XavierM @cnasikas @MindyRS