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 Solution] Minimize the use of es_archiver on cypress tests #85019

Merged
merged 24 commits into from
Dec 10, 2020

Conversation

MadameSheema
Copy link
Member

@MadameSheema MadameSheema commented Dec 4, 2020

Summary

In this PR we are minimising the use of es_archiver creating the data we need when possible using our own APIs. Wit this refactor:

  • The execution of the tests is faster
  • Our tests are more reliable and less flaky
  • We need to maintain less data

@MadameSheema MadameSheema changed the title [Security Solution] Minimize es archiver [Security Solution] Minimize the use of es_archiver on cypress tests Dec 7, 2020
@MadameSheema MadameSheema self-assigned this Dec 7, 2020
@MadameSheema MadameSheema added v7.11.0 v8.0.0 Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. release_note:skip Skip the PR/issue when compiling release notes labels Dec 7, 2020
@MadameSheema MadameSheema marked this pull request as ready for review December 7, 2020 11:34
@MadameSheema MadameSheema requested review from a team as code owners December 7, 2020 11:34
…_export.spec.ts

Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>
savedObjectId: timelineId,
templateTimelineId,
} = response!.body.data.persistTimeline.timeline;
const jsonTemplate = JSON.parse(JSON.stringify(template));
Copy link
Contributor

Choose a reason for hiding this comment

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

JSON.parse is opposite to JSON.stringify, so seems you don't need any of these as template is the same as JSON.parse(JSON.stringify(template))

cy.intercept(
'POST',
'/api/detection_engine/rules/_export?exclude_export_details=false&file_name=rules_export.ndjson'
'**api/detection_engine/rules/_export?exclude_export_details=false&file_name=rules_export.ndjson*'
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for **

@@ -319,3 +320,9 @@ export const editedRule = {
description: 'Edited Rule description',
tags: [...existingRule.tags, 'edited'],
};

export const expectedExportedRule = (rule: string) => {
const jsonrule = JSON.parse(JSON.stringify(rule));
Copy link
Contributor

Choose a reason for hiding this comment

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

const jsonrule = rule; should be fine

body: {
operationName: 'DeleteTimelineMutation',
variables: {
id: [`${timelineId}`],
Copy link
Contributor

Choose a reason for hiding this comment

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

seems timelineId is a string already, so we could just do [timelineId] here

@MadameSheema
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

merge conflict between base and head

@rylnd
Copy link
Contributor

rylnd commented Dec 9, 2020

@MadameSheema I'm getting test failures on usage of these new create* helpers:

security_solution

As indicated by the error, I don't think we can return/await a promise as those helpers are currently expecting; if we need to pass information from the request to the test, we'll have to alias and retrieve the response.


before(async () => {
const createdTimeline = await createTimeline(newRule.timeline);
rule = deepMerge(newRule, { timeline: { id: createdTimeline[0] } });
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I don't think deepmerge is strictly necessary, here:

rule = { ...newRule, { timeline: { ...newRule.timeline, id: timelineId } } }

I would go that route for now and keep dependencies to a minimum.

const timelineId = response.body.data.persistTimeline.timeline.savedObjectId;
const timelineBody = response.body.data.persistTimeline.timeline;

return [timelineId, timelineBody];
Copy link
Contributor

@rylnd rylnd Dec 9, 2020

Choose a reason for hiding this comment

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

As per my comment about promises/errors, ideally these methods just return cy.request() and these custom types go away. However, in the case that they don't, I don't believe that timelineBody is a string here as the types indicate.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also don't seem to be using timelineBody, so 🤷‍♂️

@MadameSheema
Copy link
Member Author

@rylnd lots of thanks for taking a look to the code, with the latest changes the PR got really broke, I'm working to fix it ASAP

@MadameSheema
Copy link
Member Author

@MadameSheema I'm getting test failures on usage of these new create* helpers:

security_solution

As indicated by the error, I don't think we can return/await a promise as those helpers are currently expecting; if we need to pass information from the request to the test, we'll have to alias and retrieve the response.

@ryland with this library https://www.npmjs.com/package/cypress-promise we can pass variable to the tests in a better way than alias.

@MadameSheema
Copy link
Member Author

@angorayc @patrykkopycinski @rylnd I tried to follow all your suggestions, can you please take a look to the PR again? Lots of thanks 😊

NUMBER_OF_AUDITBEAT_EXCEPTIONS_ALERTS
);
});
cy.get(NUMBER_OF_ALERTS).should('have.text', NUMBER_OF_AUDITBEAT_EXCEPTIONS_ALERTS);
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@MadameSheema
Copy link
Member Author

@elasticmachine merge upstream

Copy link
Contributor

@patrykkopycinski patrykkopycinski 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 @MadameSheema 💪

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Distributable file count

id before after diff
default 47770 47769 -1

History

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

@MadameSheema MadameSheema merged commit 1b5d43b into elastic:master Dec 10, 2020
@MadameSheema MadameSheema deleted the minimize-es-archiver branch December 10, 2020 17:31
MadameSheema added a commit to MadameSheema/kibana that referenced this pull request Dec 10, 2020
…lastic#85019)

* minimizes the uses of es_archiver

* refactor

* fixes merge issue

* fixes typecheck issue

* Update x-pack/plugins/security_solution/cypress/integration/timelines_export.spec.ts

Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>

* final refactor

* final touches

* unskips skipped tests

* removes async

* fixes typo

* removes unused lines

* fixes failing test

* fixes timelines failing tests

* fixes merge issue

Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
MadameSheema added a commit that referenced this pull request Dec 11, 2020
…85019) (#85612)

* minimizes the uses of es_archiver

* refactor

* fixes merge issue

* fixes typecheck issue

* Update x-pack/plugins/security_solution/cypress/integration/timelines_export.spec.ts

Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>

* final refactor

* final touches

* unskips skipped tests

* removes async

* fixes typo

* removes unused lines

* fixes failing test

* fixes timelines failing tests

* fixes merge issue

Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Patryk Kopyciński <contact@patrykkopycinski.com>
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants