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] Test case_view_activity.test.tsx suite #160639

Merged
merged 15 commits into from
Jun 29, 2023

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Jun 27, 2023

Summary

Fixes flaky tests in x-pack/plugins/cases/public/components/case_view/components/case_view_activity.test.tsx

Successful runs:

Fixes: #151981, #151979, #151980, #152209, #152207, #152208, #152203, #152203, #152201, #152206, #152202, #152204, #152200

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas cnasikas 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 labels Jun 27, 2023
@cnasikas cnasikas self-assigned this Jun 27, 2023
@cnasikas cnasikas force-pushed the case_activity_test branch from 1ec0e14 to a7a19c1 Compare June 29, 2023 09:44
This was linked to issues Jun 29, 2023
@cnasikas cnasikas marked this pull request as ready for review June 29, 2023 13:21
@cnasikas cnasikas requested a review from a team as a code owner June 29, 2023 13:21
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

const result = appMockRender.render(<CaseViewActivity {...caseProps} />);
expect(result.queryByTestId('case-view-edit-connector')).toBeNull();
appMockRender.render(<CaseViewActivity {...caseProps} />);
expect(screen.queryByTestId('case-view-edit-connector')).toBeNull();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: to be consistent, maybe

Suggested change
expect(screen.queryByTestId('case-view-edit-connector')).toBeNull();
expect(screen.queryByTestId('case-view-edit-connector')).not.toBeInTheDocument();

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I will change it.

expect(await screen.findByTestId('case-tags')).toBeInTheDocument();
expect(await screen.findByTestId('cases-categories')).toBeInTheDocument();
expect(await screen.findByTestId('connector-edit-header')).toBeInTheDocument();
expect(await screen.findByTestId('case-view-status-action-button')).toBeInTheDocument();

await waitForComponentToUpdate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and in many others we have this at the end of the test, why?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I remember correctly we had it to add to remove some warnings regarding the state getting updated after the end of the test. You can read more about it here https://kentcdodds.com/blog/fix-the-not-wrapped-in-act-warning

@@ -162,19 +168,21 @@ describe('Case View Page activity tab', () => {
beforeEach(() => {
jest.clearAllMocks();
appMockRender = createAppMockRenderer();
Copy link
Contributor

Choose a reason for hiding this comment

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

not related but since we are here, this looks irrelevant no? Every test seems to re-initialize the appMockRenderer anyway

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but I would like to keep it in the case in the future we add a test and do not re-initialize the appMockRenderer .

Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

LGTM all changes are reasonable but reviewing I can't really tell if this fixes the flaky test definitely 😄

@cnasikas
Copy link
Member Author

cnasikas commented Jun 29, 2023

LGTM all changes are reasonable but reviewing I can't really tell if this fixes the flaky test definitely 😄

😄 The process I follow is the following:

  1. I put a for-loop to run the test multiple times and create a PR.
  2. I check the failed tests in the CI and try to fix them. Most of the time is about either timeouts or the test cannot find an item. await find* usually solves the issue. Removing unnecessary waitFor also helps.
  3. I push again and see if there are any failures. Repeat until I do not see any failures in the CI
  4. I run the tests 3 x (45-50) times and then I remove the for-loop and push again.

The Successful runs section in the description gives us pretty good confident that the test is not flaky anymore.

@cnasikas cnasikas enabled auto-merge (squash) June 29, 2023 15:01
@cnasikas cnasikas disabled auto-merge June 29, 2023 15:12
@cnasikas cnasikas enabled auto-merge (squash) June 29, 2023 15:13
@adcoelho
Copy link
Contributor

  1. I put a for-loop to run the test multiple times and create a PR.
  2. I check the failed tests in the CI and try to fix them. Most of the time is about either timeouts or the test cannot find an item. await find* usually solves the issue. Removing unnecessary waitFor also helps.
  3. I push again and see if there are any failures. Repeat until I do not see any failures in the CI
  4. I run the tests 3 x (45-50) times and then I remove the for-loop and push again.

giphy

Sounds good! I was just wondering if this time there was something else that might have escaped me 👍

@cnasikas cnasikas merged commit 6349724 into elastic:main Jun 29, 2023
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 14 16 +2
securitySolution 413 417 +4
total +6

Total ESLint disabled count

id before after diff
enterpriseSearch 15 17 +2
securitySolution 492 496 +4
total +6

History

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

cc @cnasikas

@cnasikas cnasikas deleted the case_activity_test branch June 29, 2023 16:33
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jun 29, 2023
## Summary

Fixes flaky tests in
`x-pack/plugins/cases/public/components/case_view/components/case_view_activity.test.tsx`

Successful runs:

-
elastic@a7a19c1
(40 times)
-
elastic@f5e0645
(45 times)
-
elastic@d9838cc
(40 times)

Fixes: elastic#151981,
elastic#151979,
elastic#151980,
elastic#152209,
elastic#152207,
elastic#152208,
elastic#152203,
elastic#152203,
elastic#152201,
elastic#152206,
elastic#152202,
elastic#152204,
elastic#152200

### Checklist

Delete any items that are not applicable to this PR.

- [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

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 6349724)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.9

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jun 30, 2023
…0930)

# Backport

This will backport the following commits from `main` to `8.9`:
- [[Cases] Test `case_view_activity.test.tsx` suite
(#160639)](#160639)

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

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

<!--BACKPORT [{"author":{"name":"Christos
Nasikas","email":"christos.nasikas@elastic.co"},"sourceCommit":{"committedDate":"2023-06-29T16:33:20Z","message":"[Cases]
Test `case_view_activity.test.tsx` suite (#160639)\n\n##
Summary\r\n\r\nFixes flaky tests
in\r\n`x-pack/plugins/cases/public/components/case_view/components/case_view_activity.test.tsx`\r\n\r\nSuccessful
runs:\r\n\r\n-\r\nhttps://github.com//pull/160639/commits/a7a19c1a74df5ca9202dcb1516b9284d3faf2b2a\r\n(40
times)\r\n-\r\nhttps://github.com//pull/160639/commits/f5e0645ebd3f8bbfedab52bea02669d3e1fe9b06\r\n(45
times)\r\n-\r\nhttps://github.com//pull/160639/commits/d9838cc71ba130612b3e6610df453987d39bfe42\r\n(40
times)\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/151981,\r\nhttps://github.com/elastic/kibana/issues/151979,\r\nhttps://github.com/elastic/kibana/issues/151980,\r\nhttps://github.com/elastic/kibana/issues/152209,\r\nhttps://github.com/elastic/kibana/issues/152207,\r\nhttps://github.com/elastic/kibana/issues/152208,\r\nhttps://github.com/elastic/kibana/issues/152203,\r\nhttps://github.com/elastic/kibana/issues/152203,\r\nhttps://github.com/elastic/kibana/issues/152201,\r\nhttps://github.com/elastic/kibana/issues/152206,\r\nhttps://github.com/elastic/kibana/issues/152202,https://github.com/elastic/kibana/issues/152205,\r\nhttps://github.com/elastic/kibana/issues/152204,\r\nhttps://github.com/elastic/kibana/issues/152200\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [x] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"6349724ff26d23fe927d8cb69fef642d76b396df","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","Feature:Cases","v8.9.0","v8.10.0"],"number":160639,"url":"https://github.com/elastic/kibana/pull/160639","mergeCommit":{"message":"[Cases]
Test `case_view_activity.test.tsx` suite (#160639)\n\n##
Summary\r\n\r\nFixes flaky tests
in\r\n`x-pack/plugins/cases/public/components/case_view/components/case_view_activity.test.tsx`\r\n\r\nSuccessful
runs:\r\n\r\n-\r\nhttps://github.com//pull/160639/commits/a7a19c1a74df5ca9202dcb1516b9284d3faf2b2a\r\n(40
times)\r\n-\r\nhttps://github.com//pull/160639/commits/f5e0645ebd3f8bbfedab52bea02669d3e1fe9b06\r\n(45
times)\r\n-\r\nhttps://github.com//pull/160639/commits/d9838cc71ba130612b3e6610df453987d39bfe42\r\n(40
times)\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/151981,\r\nhttps://github.com/elastic/kibana/issues/151979,\r\nhttps://github.com/elastic/kibana/issues/151980,\r\nhttps://github.com/elastic/kibana/issues/152209,\r\nhttps://github.com/elastic/kibana/issues/152207,\r\nhttps://github.com/elastic/kibana/issues/152208,\r\nhttps://github.com/elastic/kibana/issues/152203,\r\nhttps://github.com/elastic/kibana/issues/152203,\r\nhttps://github.com/elastic/kibana/issues/152201,\r\nhttps://github.com/elastic/kibana/issues/152206,\r\nhttps://github.com/elastic/kibana/issues/152202,https://github.com/elastic/kibana/issues/152205,\r\nhttps://github.com/elastic/kibana/issues/152204,\r\nhttps://github.com/elastic/kibana/issues/152200\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [x] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"6349724ff26d23fe927d8cb69fef642d76b396df"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"8.9","label":"v8.9.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/160639","number":160639,"mergeCommit":{"message":"[Cases]
Test `case_view_activity.test.tsx` suite (#160639)\n\n##
Summary\r\n\r\nFixes flaky tests
in\r\n`x-pack/plugins/cases/public/components/case_view/components/case_view_activity.test.tsx`\r\n\r\nSuccessful
runs:\r\n\r\n-\r\nhttps://github.com//pull/160639/commits/a7a19c1a74df5ca9202dcb1516b9284d3faf2b2a\r\n(40
times)\r\n-\r\nhttps://github.com//pull/160639/commits/f5e0645ebd3f8bbfedab52bea02669d3e1fe9b06\r\n(45
times)\r\n-\r\nhttps://github.com//pull/160639/commits/d9838cc71ba130612b3e6610df453987d39bfe42\r\n(40
times)\r\n\r\nFixes:
https://github.com/elastic/kibana/issues/151981,\r\nhttps://github.com/elastic/kibana/issues/151979,\r\nhttps://github.com/elastic/kibana/issues/151980,\r\nhttps://github.com/elastic/kibana/issues/152209,\r\nhttps://github.com/elastic/kibana/issues/152207,\r\nhttps://github.com/elastic/kibana/issues/152208,\r\nhttps://github.com/elastic/kibana/issues/152203,\r\nhttps://github.com/elastic/kibana/issues/152203,\r\nhttps://github.com/elastic/kibana/issues/152201,\r\nhttps://github.com/elastic/kibana/issues/152206,\r\nhttps://github.com/elastic/kibana/issues/152202,https://github.com/elastic/kibana/issues/152205,\r\nhttps://github.com/elastic/kibana/issues/152204,\r\nhttps://github.com/elastic/kibana/issues/152200\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n\r\n### For
maintainers\r\n\r\n- [x] This was checked for breaking API changes and
was
[labeled\r\nappropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<42973632+kibanamachine@users.noreply.github.com>","sha":"6349724ff26d23fe927d8cb69fef642d76b396df"}}]}]
BACKPORT-->

Co-authored-by: Christos Nasikas <christos.nasikas@elastic.co>
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.9.0 v8.10.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/components - Case View Page activity tab filter activity should render by desc sort order Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view/components - Case View Page activity tab filter activity should show history filter as active Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view/components - Case View Page activity tab Case users User actions renders the user action users correctly Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view/components - Case View Page activity tab Case users User actions renders the assigned users correctly Failing test: Jest Tests.x-pack/plugins/cases/public/components/case_view/components - Case View Page activity tab Case users User actions renders the unassigned users correctly
6 participants