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

cnasikas · 2024-02-26T12:27:28Z

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.

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

For maintainers

This was checked for breaking API changes and was labeled appropriately

This reverts commit cf942f2.

cnasikas · 2024-02-26T12:27:45Z

/ci

cnasikas · 2024-02-29T15:05:27Z

/ci

cnasikas · 2024-03-05T20:28:59Z

/ci

cnasikas · 2024-03-06T08:58:34Z

/ci

cnasikas · 2024-03-06T08:58:50Z

x-pack/plugins/security_solution/public/common/store/store.ts

      ...subPlugins.timelines.store.initialState.timeline!,
      timelineById: {
        ...subPlugins.timelines.store.initialState.timeline.timelineById,
+        ...addNewTimeline({


If you go to OsQuery or ML and try to attach anything to cases a type error occurs when you navigate to the case by clicking the "View case" link in the toaster. This is happening only to Security solution. The reason is, that a timeline is not initialized early enough and the timeline object is empty. Specifically in this line here

kibana/x-pack/plugins/security_solution/public/timelines/components/modal/header/index.tsx

Line 83 in 7745d36

const { activeTab, dataProviders, timelineType, filters, kqlMode } = useSelector(

the selector returns undefined. This is why we need to initialize the store with a default timeline.

Before it was not an issue, because cases were waiting for the appId to be available before rendering the CasesContext and all its children (which included the timeline). This gave enough time to the security solution to run some useEffects and initialize the timeline. In this PR, to stop the flakiness on our tests and optimize the cases app, the CasesContext and all its children render immediately.

This makes sense @cnasikas, timelineById creations are dispatched from the components in the rendering cycle, so we can not assume a timeline is always created, if we do so and access it before the creation action is dispatched the app crashes.

@kqualters-elastic @michaelolo24
The approach implemented here, having the timeline defaults created in the store initialization, makes sense to me, but if we follow this approach maybe we should do the same for all the timeline ids, including the TimelineId.casePage defined above, and the test timeline, right?

And if we do it this way, I think it would be better to define all the timelines in a static way in the initialTimelineState constant that now defaults to EMPTY_TIMELINE_BY_ID = {}:

kibana/x-pack/plugins/security_solution/public/timelines/store/reducer.ts

Lines 111 to 115 in a79a4d8

export const initialTimelineState: TimelineState = {

timelineById: EMPTY_TIMELINE_BY_ID,

showCallOutUnauthorizedMsg: false,

insertTimeline: null,

};

what do you think?

I mentioned to @cnasikas that imo we should just remove TimelineId.cases, this id was from when ids were tied to all of the tables in the app, and all tables were 'timelines'. The timeline in the bottom bar on the cases page is TimelineId.active, and tables have a separate id if needed. Only table on the cases page is the alerts table. TimelineId.test shouldn't ever be a thing outside of unit tests as I understand, but I guess it can't hurt? ha. And yes I agree on the main point, defining a bare minimum set of state statically so that the app doesn't crash we should absolutely do. Creating the store immediately with a minimal static initial state, and then populating things like sourcerer data and timelines if needed via actions is tech debt/feature/bug depending on who is asking and something I'd like to clean up soon.

Okay, let's keep this as it is for now, and move/clean stuff when we refactor the timeline store.

cnasikas · 2024-03-06T10:25:02Z

/ci

elasticmachine · 2024-03-06T10:25:33Z

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

cnasikas · 2024-03-06T14:35:55Z

.buildkite/scripts/steps/functional/apm_cypress.sh

 cd "$XPACK_DIR"

-node plugins/apm/scripts/test/e2e.js \
+node plugins/observability_solution/apm/scripts/test/e2e.js \


Tests failed with the following error when applying the ci:all-cypress-suites label.

node:internal/modules/cjs/loader:1147 throw err; ^ Error: Cannot find module '/var/lib/buildkite-agent/builds/kb-n2-4-spot-861f752882a4b7f7/elastic/kibana-pull-request/kibana/x-pack/plugins/apm/scripts/test/e2e.js' at Module._resolveFilename (node:internal/modules/cjs/loader:1144:15) at Module._load (node:internal/modules/cjs/loader:985:27) at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:135:12) at node:internal/main/run_main_module:28:49 { code: 'MODULE_NOT_FOUND', requireStack: [] }

Will be also fixed in this: #177949 - so if your PR won't be merged before that one, you can rely on the update from main, and won't need Operations' our approval

cnasikas · 2024-03-06T19:07:32Z

@elasticmachine merge upstream

adcoelho · 2024-03-07T13:12:57Z

x-pack/plugins/cases/public/common/use_cases_local_storage.tsx

+
+  if (lsKeyPrefix != null && !isStorageInitialized.current) {
+    isStorageInitialized.current = true;
+    setItem(getStorageItem(lsKey, initialValue));


two questions:

if value has a default wouldn't it be enough to do saveItemToStorage(lsKey, newValue);?

why the lsKeyPrefix != null?

if value has a default wouldn't it be enough to do saveItemToStorage(lsKey, newValue);?

I think you mean value returned from the useState, right? If that's the case, yes it should be enough. I tried to play it safe here just in case the value changed for some reason which it shouldn't.

why the lsKeyPrefix != null?

The lsKeyPrefix is defined as owner.length > 0 ? owner.join('.') : appId;. The appId will be undefined on the first render. if there is no owner and no appId we do not want to create a record to the local storage. We have to wait until we have one of them (owner or appId) available. Does it make sense?

I think you mean value returned from the useState, right? If that's the case, yes it should be enough.

yeah I meant from useState

I tried to play it safe here just in case the value changed for some reason which it shouldn't.

all good 👍

adcoelho

Cases changes LGTM 👍 I left some small comments.

I tested the local storage in cases, and it seems to be working fine.

You are probably aware but this will clear the saved preferences for cases in Observability. The previous local storage key was observability-overview.

Let's see if this finally fixes our flaky tests.

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

adcoelho · 2024-03-07T13:23:08Z

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

 const CaseOwnerSelector: React.FC<Props> = ({ availableOwners, isLoading }) => {
+  const defaultValue = availableOwners.includes(SECURITY_SOLUTION_OWNER)
+    ? SECURITY_SOLUTION_OWNER
+    : availableOwners[0] ?? SECURITY_SOLUTION_OWNER;


this might be just for the types but why is the security solution the backup owner??

Hmm, good point. Let's default it to Stack cases.

Ok, I remembered that we default to the security solution because is the first shown in the UI.

x-pack/plugins/cases/public/components/user_actions/user_actions_list.tsx

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

x-pack/plugins/cases/public/components/all_cases/utility_bar.tsx

x-pack/plugins/cases/public/common/use_cases_local_storage.test.tsx

kqualters-elastic

security solution changes lgtm 👍 can change to defining it in the static initial state if you'd like or not, think that change will happen soon either way

cnasikas · 2024-03-08T07:21:17Z

security solution changes lgtm 👍 can change to defining it in the static initial state if you'd like or not, think that change will happen soon either way

Thanks, Kevin. Do you mean in the timelines plugin?

cnasikas · 2024-03-08T07:44:46Z

You are probably aware but this will clear the saved preferences for cases in Observability. The previous local storage key was observability-overview.

Yes, I am aware. I think it is fine. The local storage is not guaranteed to be consistent and permanent.

semd

LGTM!

…revert_flaky_fixes

cnasikas · 2024-03-08T14:57:37Z

/ci

cnasikas · 2024-03-09T11:04:40Z

@elasticmachine merge upstream

kibana-ci · 2024-03-09T12:16:06Z

💚 Build Succeeded

Buildkite Build
Commit: 87bb9ad

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`	456.9KB	456.0KB	-879.0B
`securitySolution`	13.0MB	12.8MB	-164.5KB
total			-165.4KB

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.7KB	171.0KB	+341.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`cases`	57	58	+1
`securitySolution`	478	479	+1
total			+2

Total ESLint disabled count

id	before	after	diff
`cases`	73	74	+1
`securitySolution`	552	553	+1
total			+2

History

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

cc @cnasikas

## Summary This PR unskip below flaky tests to confirm the fix made in #177798 Fixes #174682 Fixes #176336 Fixes #177334 Fixes #171600 Fixes #171601 Fixes #177791 Fixes #177792 Fixes #177793 Fixes #177794 Fixes #177795 Fixes #177796 Fixes #171605 Fixes #171606 Fixes #171607 Fixes #171608 Fixes #178119 Fixes #174525 Fixes #174526 Fixes #174527 Fixes #174528 Fixes #146394 Fixes #176805 Fixes #175112 Fixes #176671 Fixes #176672 Fixes #175841 Fixes #174667 Fixes #174384 Fixes #175310 ### 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

cnasikas added 2 commits February 23, 2024 09:58

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

51479ca

This reverts commit cf942f2.

Remove appId and appTitle from the cases context

40bbd4f

cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// Feature:Cases Cases feature v8.14.0 labels Feb 26, 2024

cnasikas self-assigned this Feb 26, 2024

cnasikas added 3 commits February 29, 2024 15:38

Init store with an empty timeline

5d39329

Remove onComponentInitialized callback

11eb1fa

Merge branch 'main' into revert_flaky_fixes

6d21dd5

cnasikas added 4 commits March 3, 2024 13:36

Merge branch 'main' into revert_flaky_fixes

68f0eea

Owner instead of appId

c9ce275

Use cases local storage

9d5e20c

Use the owner and fallback to the appId if missing

b655ac1

cnasikas added 3 commits March 6, 2024 09:41

Merge branch 'main' into revert_flaky_fixes

bef5634

Fix e2e tests

c6c07dc

Initiate cases timeline

81b7464

cnasikas force-pushed the revert_flaky_fixes branch from df466a2 to 81b7464 Compare March 6, 2024 08:42

cnasikas commented Mar 6, 2024

View reviewed changes

Fix test

61de14f

cnasikas marked this pull request as ready for review March 6, 2024 10:25

cnasikas requested review from a team as code owners March 6, 2024 10:25

cnasikas commented Mar 6, 2024

View reviewed changes

Merge branch 'main' into revert_flaky_fixes

5377562

cnasikas removed the request for review from a team March 6, 2024 15:45

Merge branch 'main' into revert_flaky_fixes

690ac19

adcoelho reviewed Mar 7, 2024

View reviewed changes

adcoelho approved these changes Mar 7, 2024

View reviewed changes

kqualters-elastic approved these changes Mar 7, 2024

View reviewed changes

semd approved these changes Mar 8, 2024

View reviewed changes

cnasikas added 2 commits March 8, 2024 14:56

Merge branch 'main' into revert_flaky_fixes

7f7bdeb

Merge branch 'revert_flaky_fixes' of github.com:cnasikas/kibana into …

3e7aa57

…revert_flaky_fixes

cnasikas enabled auto-merge (squash) March 8, 2024 13:42

cnasikas removed the ci:all-cypress-suites label Mar 8, 2024

Fix unit tests

7749612

cnasikas disabled auto-merge March 8, 2024 18:35

cnasikas added ci:all-cypress-suites and removed ci:all-cypress-suites labels Mar 8, 2024

Merge branch 'main' into revert_flaky_fixes

87bb9ad

cnasikas merged commit 48d8230 into elastic:main Mar 9, 2024

cnasikas deleted the revert_flaky_fixes branch March 9, 2024 13:41

kibanamachine added the backport:skip This PR does not require backporting label Mar 9, 2024

js-jankisalvi mentioned this pull request Mar 11, 2024

[Cases] Unskip flaky tests #178370

Merged

1 task

	export const initialTimelineState: TimelineState = {
	timelineById: EMPTY_TIMELINE_BY_ID,
	showCallOutUnauthorizedMsg: false,
	insertTimeline: null,
	};

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

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

Uh oh!

Conversation

cnasikas commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

cnasikas commented Feb 26, 2024

Uh oh!

cnasikas commented Feb 29, 2024

Uh oh!

cnasikas commented Mar 5, 2024

Uh oh!

cnasikas commented Mar 6, 2024

Uh oh!

cnasikas Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Mar 6, 2024

Uh oh!

elasticmachine commented Mar 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Mar 6, 2024

Uh oh!

adcoelho Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cnasikas Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adcoelho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kqualters-elastic left a comment

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Mar 8, 2024

Uh oh!

cnasikas commented Mar 8, 2024

Uh oh!

semd left a comment

Choose a reason for hiding this comment

Uh oh!

cnasikas commented Mar 8, 2024

Uh oh!

cnasikas commented Mar 9, 2024

Uh oh!

kibana-ci commented Mar 9, 2024

cnasikas commented Feb 26, 2024 •

edited

Loading

cnasikas Mar 6, 2024 •

edited

Loading

adcoelho Mar 7, 2024 •

edited

Loading

cnasikas Mar 8, 2024 •

edited

Loading