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

[RAC] create functional tests for add to case #114075

Merged
merged 11 commits into from
Oct 15, 2021

Conversation

mgiota
Copy link
Contributor

@mgiota mgiota commented Oct 6, 2021

Fixes #110642

Notes for the reviewer

  • This PR doesn't include tests for checking that rules behave the same when created from either within the solution or from the central rule management screen. I had a few ideas/questions there, we could further discuss if we want to include this in this PR cc @weltenwort

  • This is not part of this PR, but if time allows I would like a comment regarding an issue I faced and how we could fix this in the future. I was trying to refactor observability service and move workflowStatus related stuff to another file. Apparently what I tried to do is not possible, because I ended up getting a Circular reference to Service(observability). Example code and the error I got can be found here mgiota@d8405f9#r57919232

@mgiota mgiota force-pushed the 110642_functional_tests_add_to_case branch from 8b78bda to 5b1d489 Compare October 11, 2021 21:06
@mgiota mgiota added Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Theme: rac label obsolete v7.16.0 auto-backport Deprecated - use backport:version if exact versions are needed v8.0.0 labels Oct 12, 2021
@mgiota mgiota self-assigned this Oct 12, 2021
@mgiota mgiota marked this pull request as ready for review October 12, 2021 17:49
@mgiota mgiota requested a review from a team as a code owner October 12, 2021 17:49
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@mgiota mgiota marked this pull request as draft October 12, 2021 17:50
@mgiota
Copy link
Contributor Author

mgiota commented Oct 13, 2021

@elasticmachine merge upstream

@mgiota mgiota marked this pull request as ready for review October 13, 2021 06:59
@mgiota mgiota added the release_note:skip Skip the PR/issue when compiling release notes label Oct 13, 2021
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.

Cases changes LGTM

@weltenwort weltenwort self-requested a review October 13, 2021 10:18
Copy link
Member

@weltenwort weltenwort 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 👍 I left only a few minor questions below.

const CREATE_CASE_FLYOUT = 'create-case-flyout';
const SELECT_CASE_MODAL = 'all-cases-modal';

export function ObservabilityAlertsAddToCaseProvider({ getService }: FtrProviderContext) {
Copy link
Member

Choose a reason for hiding this comment

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

If you want to access functions from common in here you could just inject them as an additional parameter. As you discovered, getService('observability') doesn't work here because this code is executed in order to construct that exact service.


it('opens a flyout when Add to new case is clicked', async () => {
await retry.try(async () => {
await observability.alerts.common.openActionsMenuForRow(0);
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the menu already open from the previous step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Actually removing this line of code also fixed the issue you mentioned below (click being part of the retry). Now I moved click outside of the retry and everything works like a charm!

it('opens a flyout when Add to new case is clicked', async () => {
await retry.try(async () => {
await observability.alerts.common.openActionsMenuForRow(0);
await observability.alerts.addToCase.addToNewCaseButtonClick();
Copy link
Member

Choose a reason for hiding this comment

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

Should this click be part of the retry? Do we want to try clicking repeatedly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the beginning I didn't have it as part of the retry and the test was failing. By moving it inside the retry, test stopped failing, but what I noticed in the browser is a long delay between clicking on the menu item and opening the flyout. I will try one more time to move it outside of the retry and see what happens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested again and if click is in not part of the retry test fails. I get debg finding element 'By(css selector, [data-test-subj="add-new-case-item"])' again, 2 attempts left │ debg --- retry.try error: no such element: Unable to locate element: {"method":"css selector","selector":"[data-test-subj="add-new-case-item"]"}, because the overflow closes immediately and it can not find the element. I will leave it here for now and I could investigate again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Update: I moved it out of the retry. See my comment above

@mgiota mgiota enabled auto-merge (squash) October 15, 2021 06:07
@mgiota
Copy link
Contributor Author

mgiota commented Oct 15, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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
timelines 240.3KB 240.3KB -10.0B

History

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

cc @mgiota

@mgiota mgiota merged commit c11b38d into elastic:master Oct 15, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Oct 15, 2021
* [RAC] create functional tests for add to case

* use observability test helpers for user creation

* basic tests for add to case options

* add two more cases

* test case for clicking on add to new case button

* remove unused expect statement

* clicking on add to existing case should open a modal

* move add to case functionality in a separate file

* address comments in the PR review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Oct 15, 2021
* [RAC] create functional tests for add to case

* use observability test helpers for user creation

* basic tests for add to case options

* add two more cases

* test case for clicking on add to new case button

* remove unused expect statement

* clicking on add to existing case should open a modal

* move add to case functionality in a separate file

* address comments in the PR review

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: mgiota <giota85@gmail.com>
FrankHassanabad added a commit that referenced this pull request Oct 15, 2021
## Summary

Fixes flake cypress test

Fixes #115245 
See also: #114075, #115245

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 15, 2021
## Summary

Fixes flake cypress test

Fixes elastic#115245 
See also: elastic#114075, elastic#115245

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine added a commit that referenced this pull request Oct 16, 2021
## Summary

Fixes flake cypress test

Fixes #115245 
See also: #114075, #115245

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Frank Hassanabad <frank.hassanabad@elastic.co>
@mgiota mgiota deleted the 110642_functional_tests_add_to_case branch January 4, 2022 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Theme: rac label obsolete v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RAC] [Observability] Create functional tests for alerts table add-to-case actions
5 participants