[Cases] Fix cases flaky tests #176863

cnasikas · 2024-02-13T19:40:29Z

Summary

With the help of @umbopepato, @js-jankisalvi, and @adcoelho, we realized that one possible source of flakiness is the CasesContextProvider that every test uses under the hood. In this PR I refactor the CasesContextProvider to render immediately its children and not wait for the appId and appTitle to be defined. I make them optional and make all changes needed in the code to accommodate the presence of an optional appId. Also, I refactored some components that I believed could introduce flakiness.

Issues

Fixes #175570
Fixes #175956
Fixes #175935
Fixes #175934
Fixes #175655
Fixes #176643
Fixes #175204

Flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5255, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5261, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5264, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5267

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenario

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2024-02-13T19:40:53Z

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

elasticmachine · 2024-02-13T19:40:53Z

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

cnasikas · 2024-02-14T09:32:12Z

x-pack/plugins/cases/public/components/cases_context/index.tsx

-   * `appId` and `appTitle` are dynamically retrieved from kibana context.
-   * We need to update the state if any of these values change, the rest of props are never updated.
-   */
-  useEffect(() => {


Removing the useEffects to save re-render in the hope of making tests more resilient as all of them rely on the CasesProvider. Also, they are redundant.

cnasikas · 2024-02-14T09:33:16Z

x-pack/plugins/cases/public/components/create/form_context.tsx

@@ -41,7 +41,6 @@ const initialCaseValue: FormProps = {
  connectorId: NONE_CONNECTOR_ID,
  fields: null,
  syncAlerts: true,
-  selectedOwner: null,


The default value is set by the component that renders the solution selection.

cnasikas · 2024-02-14T09:33:39Z

x-pack/plugins/cases/public/components/create/owner_selector.tsx

@@ -61,16 +61,6 @@ const OwnerSelector = ({

  const onChange = useCallback((val: string) => field.setValue(val), [field]);

-  useEffect(() => {


Redundant. defaultValue can produce the same results.

cnasikas · 2024-02-14T09:35:12Z

x-pack/plugins/cases/public/components/create/severity.tsx

@@ -20,33 +20,38 @@ interface Props {
  isLoading: boolean;
 }

-const SeverityFieldFormComponent = ({ isLoading }: { isLoading: boolean }) => {


Moved inside UseField and remove all the watch logic.

js-jankisalvi

Nice refactoring 👍
Should we loop all tests just to verify once?

cnasikas · 2024-02-19T13:20:39Z

Nice refactoring 👍 Should we loop all tests just to verify once?

Good point. Added here 775c87e (#176863).

…Provider

adcoelho · 2024-02-22T08:48:30Z

x-pack/plugins/cases/public/components/cases_context/index.tsx

+      externalReferenceAttachmentTypeRegistry,
+      features,
+      owner,
+      permissions,


Did you end up checking if this was fine for the permissions? useMemo only uses Object.is for comparisons.

Edit: after reading the PR further I think this might be fine. It seems all we cared about here seemed to be whether or not the permissions were null.

I think is fine as it is at the moment. If we see unnecessary renders we can optimize further.

I am ok with leaving it at is but just wanted to highlight that

If we see unnecessary renders we can optimize further.

The issue is rather the permission changing in a way that does not trigger a rerender when it should be cause we only look for superficial changes to the permissions object.

I see what you mean. This could happen if someone does permissions.create = true instead of replacing the whole object. Also, most applications initiate the ContextProvider with the permissions being returned by our own helper function here https://github.com/elastic/kibana/blob/56887ac1f85dd06bf89453637eb1a0fb562a43b9/x-pack/plugins/cases/public/client/helpers/can_use_cases.ts. The function will always return a new object. Given all that, you are right. Let's take this into account and make it more explicit.

Done in a70ae15 (#176863)

adcoelho · 2024-02-22T09:04:04Z

x-pack/plugins/cases/public/components/user_profiles/user_tooltip.test.tsx

@@ -34,12 +33,12 @@ describe.skip('UserToolTip', () => {
      </UserToolTip>
    );

-    fireEvent.mouseOver(screen.getByText('case user'));
+    fireEvent.mouseOver(await screen.findByText('case user'));


nit: userEvent is the recommended class but I wonder if it takes some extra milliseconds so I don't mind keeping fireEvent.

adcoelho · 2024-02-22T09:11:01Z

x-pack/plugins/cases/public/components/cases_context/index.tsx

@@ -148,8 +137,8 @@ export const CasesProvider: React.FC<{ value: CasesContextProps }> = ({
    [getFilesClient, owner]
  );

-  return isCasesContextValue(value) ? (


In the end, what needed to be changed to remove this check?

I hadn't realized before that isCasesContextValue also checked for value.permissions != null.

Missing appId or appTitle might be ok but can't this have worse consequences when rendering cases?

In the end, what needed to be changed to remove this check?

Some changes in the PR take into account undefined appId, mostly in navigation and when we construct the local storage keys. In practice, the appId will never be undefined when the cases app is rendered.

I hadn't realized before that isCasesContextValue also checked for value.permissions != null

The permissions is coming as a prop and it is required. I am not sure why we had the check but it will never be undefined.

js-jankisalvi

Looks good, Great cleanup 🎉

kibana-ci · 2024-02-22T13:24:49Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: a70ae15

Failed CI Steps

Test Failures

[job] [logs] FTR Configs #48 / Actions and Triggers app Rules list rules list bulk actions should apply filters to bulk actions when using the select all button

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`	457.0KB	456.8KB	-198.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`cases`	170.6KB	170.8KB	+168.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`cases`	57	58	+1

Total ESLint disabled count

id	before	after	diff
`cases`	73	74	+1

History

💚 Build #194917 succeeded 5fc05b1
💛 Build #194761 was flaky d8a58fc
💚 Build #194516 succeeded 31340d7
💔 Build #194428 failed e35f95b
💔 Build #194169 failed 8d0840c
💔 Build #193532 failed 64b6824

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

cc @cnasikas

## Summary PR #176863 improved the cases context provided which we assumed caused some flakiness in our tests. I am unskipping a couple of tests to test if the changes on that PR stabilized the tests. Fixes #176805 Fixes #174528 Fixes #174527 Fixes #174526 Fixes #174525 Fixes #176600 Fixes #146394 ### 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)

## Summary As a follow-up to #176863 unskipping filterPopOver and propertyActions tests Fixes #176679 Fixes #176680 Fixes #176681 Fixes #176682 Fixes #176683 Fixes #176684 Fixes #176685 Fixes #176686 Fixes #174384 Fixes #174667 Fixes #175310

## Summary As a follow-up to #176863 I am unskipping these severity tests to check if the changes on that PR stabilized them. Fixes #176336

This reverts commit 0dd21e5.

This reverts commit cf942f2.

@umbopepato

## Summary With the help of @umbopepato, @js-jankisalvi, and @adcoelho, we realized that one possible source of flakiness is the `CasesContextProvider` that every test uses under the hood. In this PR I refactor the `CasesContextProvider` to render immediately its children and not wait for the `appId` and `appTitle` to be defined. I make them optional and make all changes needed in the code to accommodate the presence of an optional `appId`. Also, I refactored some components that I believed could introduce flakiness. ## Issues Fixes elastic#175570 Fixes elastic#175956 Fixes elastic#175935 Fixes elastic#175934 Fixes elastic#175655 Fixes elastic#176643 Fixes elastic#175204 Flaky test runner: https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5255, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5261, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5264, https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5267 ### 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 scenario ### 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: Umberto Pepato <[email protected]> Co-authored-by: kibanamachine <[email protected]>

## Summary PR elastic#176863 improved the cases context provided which we assumed caused some flakiness in our tests. I am unskipping a couple of tests to test if the changes on that PR stabilized the tests. Fixes elastic#176805 Fixes elastic#174528 Fixes elastic#174527 Fixes elastic#174526 Fixes elastic#174525 Fixes elastic#176600 Fixes elastic#146394 ### 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)

…177611) ## Summary As a follow-up to elastic#176863 unskipping filterPopOver and propertyActions tests Fixes elastic#176679 Fixes elastic#176680 Fixes elastic#176681 Fixes elastic#176682 Fixes elastic#176683 Fixes elastic#176684 Fixes elastic#176685 Fixes elastic#176686 Fixes elastic#174384 Fixes elastic#174667 Fixes elastic#175310

## Summary As a follow-up to elastic#176863 I am unskipping these severity tests to check if the changes on that PR stabilized them. Fixes elastic#176336

This reverts commit 0dd21e5.

## Summary This reverts commit cf942f2. It also moves the `appId` and `appTitle` outside the cases context. The `useApplication` hook is used when needed to get the `appId` and `appTitle`. Lastly, a new hook called `useCasesLocalStorage` was created to be used when interacting with the local storage. It will use the `owner` to namespace the keys of the local storage and fallback to the `appId` if the `owner` is empty. Fixes #175204 Fixes #175570 ### 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 <[email protected]>

cnasikas added 3 commits February 13, 2024 15:52

Refactor tags

2bebc6f

Refactor severity

e98671d

Refactor owner

db0cca9

cnasikas requested a review from a team as a code owner February 13, 2024 19:40

cnasikas self-assigned this Feb 13, 2024

cnasikas added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.14.0 labels Feb 13, 2024

cnasikas added v8.13.0 and removed v8.14.0 labels Feb 13, 2024

cnasikas added 2 commits February 13, 2024 20:02

Unskip test

bbb8bdc

Fix user tooltip

86ea983

cnasikas added the release_note:skip Skip the PR/issue when compiling release notes label Feb 13, 2024

Remove effects from the case context provider

64b6824

cnasikas commented Feb 14, 2024

View reviewed changes

js-jankisalvi reviewed Feb 14, 2024

View reviewed changes

cnasikas added 2 commits February 19, 2024 14:15

Remove unused hook

d326c2c

Add for-loops

775c87e

cnasikas and others added 4 commits February 19, 2024 14:23

Merge branch 'main' into fix_various_tests

8d0840c

Remove waiting for app details to be ready

494f57d

[Cases] Avoid empty renders of CasesProvider

df984b7

[Cases] Improve useStatefulObservable, remove unnecessary QueryClient…

e35f95b

…Provider

cnasikas mentioned this pull request Feb 20, 2024

[Cases] Avoid empty renders of CasesProvider #177271

Closed

Handle undefined appId

db7c0c0

adcoelho reviewed Feb 22, 2024

View reviewed changes

adcoelho approved these changes Feb 22, 2024

View reviewed changes

PR feedback

5fc05b1

cnasikas enabled auto-merge (squash) February 22, 2024 10:17

js-jankisalvi approved these changes Feb 22, 2024

View reviewed changes

cnasikas disabled auto-merge February 22, 2024 10:59

Destruct permissions in dependency array

a70ae15

cnasikas enabled auto-merge (squash) February 22, 2024 12:45

cnasikas merged commit 0dd21e5 into elastic:main Feb 22, 2024
21 checks passed

cnasikas deleted the fix_various_tests branch February 22, 2024 13:24

kibanamachine added the backport:skip This commit does not require backporting label Feb 22, 2024

js-jankisalvi mentioned this pull request Feb 22, 2024

Failing test: Jest Tests.x-pack/plugins/cases/public/components/custom_fields - CustomFields renders correctly #176805

Closed

cnasikas mentioned this pull request Feb 22, 2024

[Cases] Unskip flyout and custom fields flaky tests #177590

Merged

2 tasks

adcoelho mentioned this pull request Feb 22, 2024

[Cases] Unskip severity flaky test #177597

Merged

js-jankisalvi mentioned this pull request Feb 22, 2024

[Cases] Unskip filter popover, property actions flaky tests #177611

Merged

adcoelho added a commit that referenced this pull request Feb 22, 2024

[Cases] Unskip severity flaky test (#177597)

7ad7aff

## Summary As a follow-up to #176863 I am unskipping these severity tests to check if the changes on that PR stabilized them. Fixes #176336

tomsonpl mentioned this pull request Feb 22, 2024

Fix Cases Context issue #177643

Closed

jbudz added a commit that referenced this pull request Feb 22, 2024

Revert "[Cases] Fix cases flaky tests (#176863)"

cf942f2

This reverts commit 0dd21e5.

cnasikas added a commit to cnasikas/kibana that referenced this pull request Feb 26, 2024

Reapply "[Cases] Fix cases flaky tests (elastic#176863)"

51479ca

This reverts commit cf942f2.

fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024

Revert "[Cases] Fix cases flaky tests (elastic#176863)"

3fd9316

This reverts commit 0dd21e5.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Fix cases flaky tests #176863

[Cases] Fix cases flaky tests #176863

cnasikas commented Feb 13, 2024 •

edited

Loading

elasticmachine commented Feb 13, 2024

elasticmachine commented Feb 13, 2024

cnasikas Feb 14, 2024 •

edited

Loading

cnasikas Feb 14, 2024

cnasikas Feb 14, 2024 •

edited

Loading

cnasikas Feb 14, 2024 •

edited

Loading

js-jankisalvi left a comment

cnasikas commented Feb 19, 2024

adcoelho Feb 22, 2024 •

edited

Loading

cnasikas Feb 22, 2024

adcoelho Feb 22, 2024

cnasikas Feb 22, 2024

cnasikas Feb 22, 2024

adcoelho Feb 22, 2024

adcoelho Feb 22, 2024

cnasikas Feb 22, 2024

js-jankisalvi left a comment

kibana-ci commented Feb 22, 2024

ESLint disabled line counts

Total ESLint disabled count

		@@ -61,16 +61,6 @@ const OwnerSelector = ({

		const onChange = useCallback((val: string) => field.setValue(val), [field]);

		useEffect(() => {

[Cases] Fix cases flaky tests #176863

[Cases] Fix cases flaky tests #176863

Conversation

cnasikas commented Feb 13, 2024 • edited Loading

Summary

Issues

Checklist

For maintainers

elasticmachine commented Feb 13, 2024

elasticmachine commented Feb 13, 2024

cnasikas Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

cnasikas Feb 14, 2024 • edited Loading

Choose a reason for hiding this comment

js-jankisalvi left a comment

Choose a reason for hiding this comment

cnasikas commented Feb 19, 2024

adcoelho Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js-jankisalvi left a comment

Choose a reason for hiding this comment

kibana-ci commented Feb 22, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

cnasikas commented Feb 13, 2024 •

edited

Loading

cnasikas Feb 14, 2024 •

edited

Loading

cnasikas Feb 14, 2024 •

edited

Loading

cnasikas Feb 14, 2024 •

edited

Loading

adcoelho Feb 22, 2024 •

edited

Loading