[Defend Workflows][E2E] Isolate command e2e coverage #154465

szwarckonrad · 2023-04-05T15:54:57Z

E2E coverage of TestRail "Isolate" suite.
These tests are run against mocked documents. We intercept REST request whenever Isolate button is used and mock response action response with success.

# Conflicts: # x-pack/plugins/security_solution/common/endpoint/index_data.ts

paul-tavares

Did not have time to complete my review, but here are the comments I have thus far. Will pick this up again tomorrow.

...k/plugins/security_solution/common/endpoint/data_generators/endpoint_rule_alert_generator.ts

x-pack/plugins/security_solution/common/endpoint/data_loaders/index_endpoint_rule_alerts.ts

x-pack/plugins/security_solution/common/endpoint/generate_data.ts

x-pack/plugins/security_solution/common/endpoint/index_data.ts

...gins/security_solution/scripts/endpoint/agent_emulator/services/endpoint_response_actions.ts

# Conflicts: # x-pack/plugins/security_solution/public/management/cypress/support/data_loaders.ts # x-pack/plugins/security_solution/public/management/cypress/support/plugin_handlers/endpoint_data_loader.ts # x-pack/plugins/security_solution/public/management/cypress/tasks/index_endpoint_hosts.ts # x-pack/plugins/security_solution/scripts/endpoint/agent_emulator/services/endpoint_response_actions.ts

# Conflicts: # x-pack/test/security_solution_endpoint/apps/endpoint/endpoint_list.ts

elasticmachine · 2023-04-18T16:07:36Z

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

paul-tavares · 2023-04-20T14:57:04Z

x-pack/plugins/cases/public/components/user_actions/comment/actions.tsx

@@ -34,6 +34,7 @@ export const createActionAttachmentUserActionBuilder = ({
  // TODO: Fix this manually. Issue #123375
  // eslint-disable-next-line react/display-name
  build: () => {
+    const actionIconName = comment.actions.type === 'isolate' ? 'lock' : 'lockOpen';


Were these changes intentionally done? or a result of perhaps a merge conflict resolution?

Yes, these were done intentionally. Icon type does not propagate to the component correctly for lock variant, thats why I've used the aria field that works properly. I will create a PR that fixes this in the UI project. One of the tests from TestRail was about making sure the proper icon is being displayed, hence these changes.

paul-tavares · 2023-04-20T15:02:15Z

x-pack/plugins/security_solution/common/endpoint/index_data.ts

@@ -103,7 +105,7 @@ export async function indexHostsAndAlerts(
  }

  for (let i = 0; i < numHosts; i++) {
-    const generator = new DocGenerator(random);
+    const generator = new DocGenerator(random, undefined);


Optional: you don't need undefined as the second argument since the argument is optional

paul-tavares · 2023-04-20T15:04:41Z

x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/isolate.cy.ts

+      login();
+    });
+    it('should allow filtering endpoint by Isolated status', () => {
+      cy.visit('/app/security/administration/endpoints');


can you use the rouing method to get the page's path instead of hard coding it:

cy.visit(APP_PATH + getEndpointListPath({ name: 'endpointList' }));

paul-tavares · 2023-04-20T15:06:34Z

x-pack/plugins/security_solution/public/management/cypress/e2e/mocked_data/isolate.cy.ts

+      let isolateRequestResponse: ActionDetails;
+      let releaseRequestResponse: ActionDetails;
+
+      cy.visit('/app/security/alerts');


Same here. Please use const APP_ALERTS_PATH from:

https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/common/constants.ts#L181

paul-tavares · 2023-04-20T15:10:26Z

x-pack/plugins/security_solution/public/management/cypress/tasks/isolate.ts

+  });
+};
+
+export const isolateHostWithComment = (comment: string, hostname: string) => {


Just wondering if all of these functions (from here down) that are "screen" specific should actually be placed into screens/** folder since (at least the way I understood it), screens/ is where we would have the methods available to interact with them. I see some perhaps going into a isolation_flyout and other into a alerts file.

(FYI: In my pending PR, I have added a screens/alerts.ts, so if you create that, and depending on which one of us merges first, you might have some merge conflicts)

IMO its better to keep them per test. We usually end up with numerous separated helpers/selectors/actions that for the next person working with them are:

Really hard to find since these files tend to have hundreds of lines and dozens of different functions

Tricky in their reusability. Something that works in alerts context might not be reusable when adding alert from different context.

Ok. but I do think that it will just increase code duplication and will likely lead to more maintenance as things change in a specific page. I thought I had read in the cypress docs that this was the suggested best practice: you have your "page helpers" that are Page specific, and then you have your specs which are validating functionality and use the helps depending on what page you are in.

Being that our cypress project really has no suggested guidance on the file/code structure and organization, I guess you (us) can keep it as you see fit 🤷‍♂️

paul-tavares · 2023-04-20T15:10:43Z

x-pack/plugins/security_solution/public/management/cypress/tasks/isolate.ts

+
+import type { ActionDetails } from '../../../../common/endpoint/types';
+
+export const interceptActionRequests = (


can you please add return types to all of these functions

All these functions have no returns. I tend to separate cy selectors/actions that are being used in multiple places.

No, these have returns, even if it's a void. Yes, Typescript will infer the return types, but having a explicit return type is best as it not only clarifies to the developer what the function is suppose to return, but also help to ensure that as changes occur inside of a given function, if those break the expected return type, then you get feedback immediately.

IMO, I believe its best to ensure that functions (especially those that are export'd out) to be property typed, but if you feel strongly it is not necessary, then disregard (although, I will continue to provide this type of feedback in code reviews 😄 ).

Since you have a strong opinion on the matter and I don't, I will add these explicit returns :)

paul-tavares · 2023-04-20T15:11:11Z

x-pack/plugins/security_solution/public/management/cypress/tasks/isolate.ts

+  cb: (responseBody: ActionDetails) => void,
+  alias: string
+) => {
+  cy.intercept('POST', '/api/endpoint/action/*', (req) => {


can you use a const for the path here instead of hard coding it?

paul-tavares

👍

…nto endpoint-isolate-e2e-coverage

kibana-ci · 2023-04-21T16:23:24Z

💚 Build Succeeded

Buildkite Build
Commit: db17840

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`cases`	374.3KB	374.3KB	+44.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	394	397	+3

Total ESLint disabled count

id	before	after	diff
`securitySolution`	474	477	+3

History

💔 Build #121907 failed 62fa777
💛 Build #121471 was flaky b3a7460
💚 Build #120756 succeeded ac9e110
💔 Build #120680 failed d2fbb5d
💔 Build #120393 failed 25674ff

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

cnasikas

ResponseOps changes LGTM!

…rt (#155360) Depends on #154465 This PR backports `isolate` command e2e tests to be used with multipass.

szwarckonrad added 2 commits April 4, 2023 09:29

avatar aria label

2f884d5

isolate command e2e coverage

704886c

szwarckonrad added release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.8.0 labels Apr 5, 2023

szwarckonrad requested review from patrykkopycinski and paul-tavares April 5, 2023 15:54

szwarckonrad added 4 commits April 5, 2023 17:55

Merge branch 'main' into endpoint-isolate-e2e-coverage

feab480

typings

5db6e07

Merge branch 'main' into endpoint-isolate-e2e-coverage

51b63c3

# Conflicts: # x-pack/plugins/security_solution/common/endpoint/index_data.ts

typings

4de9906

paul-tavares reviewed Apr 10, 2023

View reviewed changes

paul-tavares reviewed Apr 11, 2023

View reviewed changes

x-pack/plugins/security_solution/common/endpoint/index_data.ts Outdated Show resolved Hide resolved

paul-tavares reviewed Apr 11, 2023

View reviewed changes

...gins/security_solution/scripts/endpoint/agent_emulator/services/endpoint_response_actions.ts Outdated Show resolved Hide resolved

paul-tavares reviewed Apr 11, 2023

View reviewed changes

...gins/security_solution/scripts/endpoint/agent_emulator/services/endpoint_response_actions.ts Outdated Show resolved Hide resolved

szwarckonrad added 9 commits April 17, 2023 15:38

cleanup

67eb92c

use custom document generator

fcc702f

Merge branch 'main' into endpoint-isolate-e2e-coverage

25674ff

Merge branch 'main' into endpoint-isolate-e2e-coverage

7c9043f

manualy refresh result list

c54509b

Merge branch 'main' into endpoint-isolate-e2e-coverage

d2fbb5d

# Conflicts: # x-pack/test/security_solution_endpoint/apps/endpoint/endpoint_list.ts

remove artifacts after endpoints.cy.ts test

9415935

Merge branch 'main' into endpoint-isolate-e2e-coverage

ac9e110

szwarckonrad marked this pull request as ready for review April 18, 2023 16:07

szwarckonrad requested review from a team as code owners April 18, 2023 16:07

szwarckonrad mentioned this pull request Apr 20, 2023

[Defend Workflows][E2E]Endpoint isolate e2e coverage multipass backport #155360

Merged

szwarckonrad added 2 commits April 20, 2023 12:12

Merge branch 'main' into endpoint-isolate-e2e-coverage

f84233b

cleanup

b3a7460

szwarckonrad requested a review from paul-tavares April 20, 2023 14:54

paul-tavares reviewed Apr 20, 2023

View reviewed changes

szwarckonrad added 2 commits April 21, 2023 11:30

tweaks

b5b6941

Merge branch 'main' into endpoint-isolate-e2e-coverage

62fa777

paul-tavares approved these changes Apr 21, 2023

View reviewed changes

szwarckonrad added 3 commits April 21, 2023 16:25

Merge branch 'main' into endpoint-isolate-e2e-coverage

27101e2

type returns of helper functions

391ed8c

Merge remote-tracking branch 'origin/endpoint-isolate-e2e-coverage' i…

db17840

…nto endpoint-isolate-e2e-coverage

cnasikas approved these changes Apr 24, 2023

View reviewed changes

szwarckonrad merged commit 792c786 into elastic:main Apr 24, 2023

kibanamachine added the backport:skip This commit does not require backporting label Apr 24, 2023

szwarckonrad added a commit that referenced this pull request Apr 25, 2023

[Defend Workflows][E2E]Endpoint isolate e2e coverage multipass backpo…

862103a

…rt (#155360) Depends on #154465 This PR backports `isolate` command e2e tests to be used with multipass.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Defend Workflows][E2E] Isolate command e2e coverage #154465

[Defend Workflows][E2E] Isolate command e2e coverage #154465

szwarckonrad commented Apr 5, 2023 •

edited

Loading

paul-tavares left a comment

elasticmachine commented Apr 18, 2023

paul-tavares Apr 20, 2023

szwarckonrad Apr 21, 2023

paul-tavares Apr 20, 2023

paul-tavares Apr 20, 2023

paul-tavares Apr 20, 2023

paul-tavares Apr 20, 2023

szwarckonrad Apr 21, 2023

paul-tavares Apr 21, 2023

paul-tavares Apr 20, 2023

szwarckonrad Apr 21, 2023

paul-tavares Apr 21, 2023

szwarckonrad Apr 21, 2023

paul-tavares Apr 20, 2023

paul-tavares left a comment

kibana-ci commented Apr 21, 2023

ESLint disabled line counts

Total ESLint disabled count

cnasikas left a comment


		import type { ActionDetails } from '../../../../common/endpoint/types';

		export const interceptActionRequests = (

[Defend Workflows][E2E] Isolate command e2e coverage #154465

[Defend Workflows][E2E] Isolate command e2e coverage #154465

Conversation

szwarckonrad commented Apr 5, 2023 • edited Loading

paul-tavares left a comment

Choose a reason for hiding this comment

elasticmachine commented Apr 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-tavares left a comment

Choose a reason for hiding this comment

kibana-ci commented Apr 21, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

cnasikas left a comment

Choose a reason for hiding this comment

szwarckonrad commented Apr 5, 2023 •

edited

Loading