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

[Cases] Improve functional tests #150117

Conversation

jonathan-buttner
Copy link
Contributor

@jonathan-buttner jonathan-buttner commented Feb 1, 2023

This PR tries to improve how often our functional tests succeeds. I also tried cleaning up a few things that seemed to be slowing the tests down and also causing errors when the tests were run individually.

Fixes: #145271

Notable changes:

  • I added a value to the property-actions* in most places so that the functional tests can distinguish between a description, comment, or the case ellipses this seems to work consistently where other methods have not

Flaky test run: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1871

@jonathan-buttner jonathan-buttner added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) v8.7.0 v8.6.2 labels Feb 1, 2023

PropertyActions.displayName = 'PropertyActions';

const makeDataTestSubjPrepend = (customDataTestSubj?: string) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows us to pass in a specific portion of the data test subject

@@ -84,7 +84,7 @@ const ActionsComponent: React.FC<CaseViewActions> = ({ caseData, currentExternal

return (
<EuiFlexItem grow={false} data-test-subj="case-view-actions">
<PropertyActions propertyActions={propertyActions} />
<PropertyActions propertyActions={propertyActions} customDataTestSubj={'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.

For the single case view we pass in the case value so the data test subject ellipses is unique

const UserActionPropertyActionsComponent: React.FC<Props> = ({
isLoading,
propertyActions,
customDataTestSubj = 'user-action',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's default all the user action property actions to user-action. In the future we may want to make these more specific. One particular scenario is the description so that some of the functional tests can be more specific.

<UserActionPropertyActions
isLoading={isLoading}
propertyActions={propertyActions}
customDataTestSubj={'description'}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding description to the data test subject for the functional tests

await caseActions.click();
await testSubjects.existOrFail('property-actions-trash');
await common.clickAndValidate('property-actions-trash', 'confirmModalConfirmButton');
await testSubjects.click('property-actions-case-ellipses');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now we can look specifically for the case ellipses.

'[data-test-subj*="property-actions-ellipses"]'
);

propertyActions[propertyActions.length - 1].click();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was trying to account for the description ellipses, now we can call it out directly.

@@ -379,7 +363,7 @@ export default ({ getPageObject, getService }: FtrProviderContext) => {
describe('Assignees field', () => {
before(async () => {
await createUsersAndRoles(getService, users, roles);
await cases.api.activateUserProfiles([casesAllUser]);
await cases.api.activateUserProfiles([casesAllUser, casesAllUser2]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without casesAllUser2 the tests below this fail if run by themselves 😬

@@ -81,9 +81,6 @@ export function CasesTableServiceProvider(
rows = await find.allByCssSelector('[data-test-subj*="cases-table-row-"', 100);
if (rows.length > 0) {
await this.bulkDeleteAllCases();
// wait for a second
await new Promise((r) => setTimeout(r, 1000));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cnasikas this felt like it was slowly down the tests. Do we need to 1 second wait if we're going to call waitUntilLoadingHasFinished anyway?

Also the call to header.waitUntilLoadingHasFinished(); was happening twice I think, once at like 86 and then it loops since rows.length > 0 to get in this if block and again at line 79 above. So I just removed 86 since it'll happen above anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure why the timeout was there. I agree with your changes. The flaky test runner will verify that it is ok to remove it.

@jonathan-buttner jonathan-buttner marked this pull request as ready for review February 2, 2023 18:10
@jonathan-buttner jonathan-buttner requested a review from a team as a code owner February 2, 2023 18:10
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

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.

Great job! The flaky test runner failed two tests. It seems that the case deletion test is still flaky.

@kibana-ci
Copy link
Collaborator

💚 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
cases 391.9KB 392.1KB +241.0B

History

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

@jonathan-buttner jonathan-buttner merged commit c3ea5e5 into elastic:main Feb 6, 2023
@jonathan-buttner jonathan-buttner deleted the cases-fix-delete-case-row-action branch February 6, 2023 20:38
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.6 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 150117

Questions ?

Please refer to the Backport tool documentation

@jonathan-buttner jonathan-buttner removed backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) v8.6.2 labels Feb 7, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Feb 7, 2023
@jonathan-buttner
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

jonathan-buttner added a commit to jonathan-buttner/kibana that referenced this pull request Feb 7, 2023
This PR tries to improve how often our functional tests succeeds. I also
tried cleaning up a few things that seemed to be slowing the tests down
and also causing errors when the tests were run individually.

Fixes: elastic#145271

Notable changes:
- I added a value to the `property-actions*` in most places so that the
functional tests can distinguish between a description, comment, or the
case ellipses this seems to work consistently where other methods have
not

Flaky test run:
https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1871

(cherry picked from commit c3ea5e5)

# Conflicts:
#	x-pack/plugins/cases/public/components/case_action_bar/actions.test.tsx
#	x-pack/plugins/cases/public/components/case_action_bar/index.test.tsx
#	x-pack/plugins/cases/public/components/property_actions/index.tsx
#	x-pack/plugins/cases/public/components/user_actions/index.test.tsx
#	x-pack/test/functional_with_es_ssl/apps/cases/view_case.ts
@jonathan-buttner jonathan-buttner added v8.6.2 and removed backport:skip This commit does not require backporting labels Feb 7, 2023
jonathan-buttner added a commit that referenced this pull request Feb 7, 2023
# Backport

This will backport the following commits from `main` to `8.6`:
- [[Cases] Improve functional tests
(#150117)](#150117)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Jonathan
Buttner","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-02-06T20:38:36Z","message":"[Cases]
Improve functional tests (#150117)\n\nThis PR tries to improve how often
our functional tests succeeds. I also\r\ntried cleaning up a few things
that seemed to be slowing the tests down\r\nand also causing errors when
the tests were run individually.\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/145271\r\n\r\nNotable
changes:\r\n- I added a value to the `property-actions*` in most places
so that the\r\nfunctional tests can distinguish between a description,
comment, or the\r\ncase ellipses this seems to work consistently where
other methods have\r\nnot\r\n\r\nFlaky test
run:\r\nhttps://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1871","sha":"c3ea5e5b3a2f0e13e743eea059404c81a07576c1","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:skip","Team:ResponseOps","Feature:Cases","v8.7.0"],"number":150117,"url":"https://github.com/elastic/kibana/pull/150117","mergeCommit":{"message":"[Cases]
Improve functional tests (#150117)\n\nThis PR tries to improve how often
our functional tests succeeds. I also\r\ntried cleaning up a few things
that seemed to be slowing the tests down\r\nand also causing errors when
the tests were run individually.\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/145271\r\n\r\nNotable
changes:\r\n- I added a value to the `property-actions*` in most places
so that the\r\nfunctional tests can distinguish between a description,
comment, or the\r\ncase ellipses this seems to work consistently where
other methods have\r\nnot\r\n\r\nFlaky test
run:\r\nhttps://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1871","sha":"c3ea5e5b3a2f0e13e743eea059404c81a07576c1"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/150117","number":150117,"mergeCommit":{"message":"[Cases]
Improve functional tests (#150117)\n\nThis PR tries to improve how often
our functional tests succeeds. I also\r\ntried cleaning up a few things
that seemed to be slowing the tests down\r\nand also causing errors when
the tests were run individually.\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/145271\r\n\r\nNotable
changes:\r\n- I added a value to the `property-actions*` in most places
so that the\r\nfunctional tests can distinguish between a description,
comment, or the\r\ncase ellipses this seems to work consistently where
other methods have\r\nnot\r\n\r\nFlaky test
run:\r\nhttps://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/1871","sha":"c3ea5e5b3a2f0e13e743eea059404c81a07576c1"}}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.6.2 v8.7.0
Projects
None yet
5 participants