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 slow CaseViewPage jest tests #150952

Closed

Conversation

jonathan-buttner
Copy link
Contributor

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

Fixes: 149782, 149781, 149780, 149778, 149779, 149777, 149776, 149775, 149773, 149774, 149772, 149771, 149770, 149769

This PR attempts to speed up some slow tests for the CaseViewPage component.

The should update connector test was reporting a failure for exceeding the 5000 timeout. I tried moving some things out of the beforeEach, using screen, and adding skipPointerEventsCheck: true which according to the docs can be a performance improvement

image

The other thing I noticed was that when a test exceeds the timeout it can cause tests after it to fail. I'm not sure why but I was able to reproduce this:

CaseViewPage › should update connector

    thrown: "Exceeded timeout of 5000 ms for a test.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      307 |   });
      308 |
    > 309 |   it('should update connector', async () => {
          |   ^
      310 |     appMockRenderer.render(
      311 |       <CaseViewPage
      312 |         {...caseProps}

      at it (x-pack/plugins/cases/public/components/case_view/case_view_page.test.tsx:309:3)
      at Object.describe (x-pack/plugins/cases/public/components/case_view/case_view_page.test.tsx:96:1)

  ● CaseViewPage › should call show alert details with expected arguments

    TestingLibraryElementError: Unable to find an element by: [data-test-subj="comment-action-show-alert-alert-action-id"]

    Ignored nodes: comments, script, style
    <body>
      <div />
    </body>

      393 |     );
      394 |
    > 395 |     userEvent.click(result.getByTestId('comment-action-show-alert-alert-action-id'));
          |                            ^
      396 |
      397 |     await waitFor(() => {
      398 |       expect(showAlertDetails).toHaveBeenCalledWith('alert-id-1', 'alert-index-1');

      at Object.getElementError (node_modules/@testing-library/dom/dist/config.js:40:19)
      at node_modules/@testing-library/dom/dist/query-helpers.js:90:38
      at node_modules/@testing-library/dom/dist/query-helpers.js:62:17
      at node_modules/@testing-library/dom/dist/query-helpers.js:111:19
      at Object.getByTestId (x-pack/plugins/cases/public/components/case_view/case_view_page.test.tsx:395:28)

I think that's what's happening with the CI failure here: https://buildkite.com/elastic/kibana-on-merge/builds/26147#0185f527-6859-4d01-ac37-e5483a535e48

Example performance improvements

New performance

 PASS  x-pack/plugins/cases/public/components/case_view/case_view_page.test.tsx (38.937 s)
  CaseViewPage
    ✓ should render CaseViewPage (944 ms)
    ✓ should show closed indicators in header when case is closed (503 ms)
    ✓ should update status (1093 ms)
    ✓ should display EditableTitle isLoading (480 ms)
    ✓ should display description isLoading (461 ms)
    ✓ should display tags isLoading (446 ms)
    ✓ should update title (822 ms)
    ✓ should push updates on button click (574 ms)
    ✓ should disable the push button when connector is invalid (475 ms)
    ✓ should update connector (959 ms) <--- *****Note This one
    ✓ should call onComponentInitialized on mount (473 ms)
    ✓ should show loading content when loading user actions (162 ms)
    ✓ should call show alert details with expected arguments (686 ms)
    ✓ should show the rule name (461 ms)
    ✓ should update settings (583 ms)
    ✓ should show the correct connector name on the push button (454 ms)
    Callouts
      ✓ it shows the danger callout when a connector has been deleted (420 ms)
      ✓ it does NOT shows the danger callout when connectors are loading (369 ms)
    Tabs
      ✓ renders tabs correctly (433 ms)
      ✓ renders the activity tab by default (699 ms)
      ✓ renders the alerts tab when the query parameter tabId has alerts (78 ms)
      ✓ renders the activity tab when the query parameter tabId has activity (476 ms)
      ✓ renders the activity tab when the query parameter tabId has an unknown value (465 ms)
      ✓ navigates to the activity tab when the activity tab is clicked (673 ms)
      ✓ navigates to the alerts tab when the alerts tab is clicked (494 ms)
      ✓ should display the alerts tab when the feature is enabled (365 ms)
      ✓ should not display the alerts tab when the feature is disabled (530 ms)
      ✓ should not show the experimental badge on the alerts table (376 ms)
      ✓ should show the experimental badge on the alerts table (364 ms)

Old performance

 PASS  x-pack/plugins/cases/public/components/case_view/case_view_page.test.tsx (43.926 s)
  CaseViewPage
    ✓ should render CaseViewPage (997 ms)
    ✓ should show closed indicators in header when case is closed (629 ms)
    ✓ should update status (1429 ms)
    ✓ should display EditableTitle isLoading (805 ms)
    ✓ should display description isLoading (771 ms)
    ✓ should display tags isLoading (741 ms)
    ✓ should update title (1239 ms)
    ✓ should push updates on button click (786 ms)
    ✓ should disable the push button when connector is invalid (745 ms)
    ✓ should update connector (1635 ms) <--- ****Nearly double
    ✓ should call onComponentInitialized on mount (750 ms)
    ✓ should show loading content when loading user actions (361 ms)
    ✓ should call show alert details with expected arguments (1002 ms)
    ✓ should show the rule name (1024 ms)
    ✓ should update settings (800 ms)
    ✓ should show the correct connector name on the push button (859 ms)
    Callouts
      ✓ it shows the danger callout when a connector has been deleted (552 ms)
      ✓ it does NOT shows the danger callout when connectors are loading (523 ms)
    Tabs
      ✓ renders tabs correctly (759 ms)
      ✓ renders the activity tab by default (662 ms)
      ✓ renders the alerts tab when the query parameter tabId has alerts (84 ms)
      ✓ renders the activity tab when the query parameter tabId has activity (693 ms)
      ✓ renders the activity tab when the query parameter tabId has an unknown value (678 ms)
      ✓ navigates to the activity tab when the activity tab is clicked (705 ms)
      ✓ navigates to the alerts tab when the alerts tab is clicked (690 ms)
      ✓ should display the alerts tab when the feature is enabled (508 ms)
      ✓ should not display the alerts tab when the feature is disabled (483 ms)
      ✓ should not show the experimental badge on the alerts table (621 ms)
      ✓ should show the experimental badge on the alerts table (487 ms)

@jonathan-buttner jonathan-buttner added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature labels Feb 10, 2023
@jonathan-buttner jonathan-buttner requested a review from a team as a code owner February 10, 2023 22:05
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

This was linked to issues Feb 10, 2023
});

userEvent.click(result.getByTestId('edit-connectors-submit'));
expect(await screen.findByTestId('connector-fields-resilient')).toBeInTheDocument();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

findByTestId does a waitFor internally so this may be a performance improvement 🤷‍♂️ maybe more a readability improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

This is amazing @jonathan-buttner! I learned so much 🙂. Inspired by @mikecote, maybe you can add a for-loop around the test to run it on the CI multiple times and then remove it to be sure the test is not flaky. Example f9372b2.


const dropdown = screen.getByTestId('case-view-status-dropdown');
userEvent.click(dropdown.querySelector('button')!, undefined, {
skipPointerEventsCheck: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great!!

Copy link
Contributor

@js-jankisalvi js-jankisalvi left a comment

Choose a reason for hiding this comment

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

Awesome work!! 🎉

@kibana-ci
Copy link
Collaborator

kibana-ci commented Feb 13, 2023

💔 Build Failed

Failed CI Steps

Metrics [docs]

✅ unchanged

History

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

const license = licensingMock.createLicense({
license: { type: 'platinum' },
});
for (let i = 0; i < 500; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget to remove this 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience 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.7.0 v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view - CaseViewPage Tabs renders the alerts tab when the query parameter tabId has alerts Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view - CaseViewPage Tabs renders the activity tab by default Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view - CaseViewPage Tabs renders tabs correctly Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view - CaseViewPage Callouts it shows the danger callout when a connector has been deleted Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view - CaseViewPage should update settings
6 participants