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

[Security Solution] [Detections] Improves custom query rule upgrade test #114454

Merged
merged 6 commits into from
Oct 13, 2021
Merged

[Security Solution] [Detections] Improves custom query rule upgrade test #114454

merged 6 commits into from
Oct 13, 2021

Conversation

MadameSheema
Copy link
Member

Summary

In this PR we are improving the custom query rule upgrade test.

Note: This PR should be merged after https://github.com/elastic/elastic-stack-testing/pull/1055

@MadameSheema MadameSheema requested a review from a team as a code owner October 11, 2021 09:59
@MadameSheema MadameSheema self-assigned this Oct 11, 2021
@MadameSheema MadameSheema added Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detections and Resp Security Detection Response Team labels Oct 11, 2021
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@MadameSheema MadameSheema added release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0 labels Oct 11, 2021
@MadameSheema
Copy link
Member Author

@elasticmachine merge upstream

@MadameSheema
Copy link
Member Author

@elasticmachine merge upstream

@MadameSheema
Copy link
Member Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @MadameSheema

@peluja1012 peluja1012 requested a review from a team October 12, 2021 19:03
rylnd
rylnd approved these changes Oct 13, 2021
View reviewed changes
Copy link
Contributor

@rylnd rylnd left a comment

Choose a reason for hiding this comment

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

Looks good in general, but I had one question about the behavior of tests without explicit assertions. Approving independent of an answer there, though.

We should update the cypress README to match these changes, if possible.

x-pack/plugins/security_solution/package.json
@@ -11,6 +11,7 @@
"cypress:open": "yarn cypress open --config-file ./cypress/cypress.json",
"cypress:open:ccs": "yarn cypress:open --config integrationFolder=./cypress/ccs_integration",
"cypress:open-as-ci": "node ../../../scripts/functional_tests --config ../../test/security_solution_cypress/visual_config.ts",
"cypress:open:upgrade": "yarn cypress:open --config integrationFolder=./cypress/upgrade_integration",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should document this new script in our README.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. The overall README of the upgrade tests is still in progress.

The nature of these upgrade tests is completely different from the ones we are use to write. Everything is originated on the elastic stack repo, it creates a cloud deployment, generates the test data needed, and then upgrades to a different version. All of these, are done using gradle tasks.

Once all those steps are finished, it executes our upgrade cypress tests.

The best practices here are going to be slightly different from the ones we tend to use. I.E. We are not going to be able to clean the data between tests, since the time cost of spin-up an instance and upgrade it is huge.

At this point, I don't know what is the best way of writing these types of tests (as well as the data creation) for us and what should be the best practices. This is why I'm adding new tests, in order to explore and find it.

x-pack/plugins/security_solution/cypress/upgrade_integration/custom_query_rule.spec.ts
});
});

it('Displays the alert details', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since there are no explicit assertions in this test (just .contains and .then), what is the behavior here when an element is not present?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using here the contains instead of the have.text assertion, because the elements are returning something like elementText Row1 Column1. I tried different ways to get the text I wanted but was not able to do it.

Contains yields to the element that has the text we are expecting, then, we scroll left using the keyword. We have to scroll, because not all the elements are displayed on the screen making them not accessible for Cypress.

In case that the element is not present, the test fails (see below screenshot).

Screenshot 2021-10-13 at 15 23 39

I would be more than happy to run a pair-programming session to find a better solution :)

@MadameSheema
Copy link
Member Author

Lots of thanks for the review @rylnd!! I replied to all your comments. Merging the PR but more than happy to improve the scenario and the overall upgrade automation in further PRs.

@MadameSheema MadameSheema merged commit 9fb16d5 into elastic:master Oct 13, 2021
@MadameSheema MadameSheema deleted the improves-upgrade-test branch October 13, 2021 13:28
@MadameSheema MadameSheema mentioned this pull request Oct 13, 2021
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 13, 2021
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into reportingg/gu…
32c6f05 
…ide-users-to-saving-ux

* 'master' of github.com:elastic/kibana: (133 commits)
  [DOCS] Indicate reports are a subscription feature (elastic#114653)
  Update namespace for indices (elastic#114612)
  [DOCS] Adds Logstash pipeline settings (elastic#114648)
  Bump EPR snapshot version used for tests (elastic#114529)
  [Security Solution] [Endpoint] Fleet summary card adjustments (elastic#114291)
  skip flaky suite (elastic#68400)
  [Visualizations] fix usage of optional dependencies (elastic#114286)
  [Security Solution] [Detections] Improves custom query rule upgrade test (elastic#114454)
  [fleet] Add Integration Preference selector (elastic#114432)
  [Reporting] Add new `data-render-error` attribute (elastic#114472)
  Replace EuiCodeEditor with CodeEditor in app-services code (elastic#114316)
  [data views] add getDefaultDataView method  (elastic#113891)
  [Security Solution] [Endpoint] Event filters uses the new card design (elastic#114126)
  [fleet] Tweak Header UI (elastic#114704)
  [APM] Filter on tx metrics for instance stats (elastic#114758)
  [APM] Fix typo in linting docs (elastic#114764)
  [Discover] Removing SavedObject usage for savedSearch (elastic#112983)
  [Fleet] Add Integration Policy Page Improvements (elastic#114556)
  [Lens] Keep the custom label when transitioning to/from Formula (elastic#114270)
  [Security Solution][Endpoint] Host Isolation API changes (elastic#113621)
  ...
MadameSheema added a commit that referenced this pull request Oct 13, 2021
@MadameSheema @kibanamachine
[Security Solution] [Detections] Improves custom query rule upgrade t…
e335205 
…est (#114454) (#114801)

* improves upgrade test

* fixes type check issues

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.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
Reviewers

@rylnd rylnd rylnd approved these changes

@peluja1012 peluja1012 Awaiting requested review from peluja1012

Assignees

@MadameSheema MadameSheema

Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v7.16.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MadameSheema @elasticmachine @kibanamachine @rylnd