[Discover] Fix flaky cell actions tests #202097

davismcphee · 2024-11-28T04:08:21Z

Summary

This PR fixes a tricky race condition when setting default Discover profile state, which was causing functional test failures such as the linked cell actions tests.

Resolves #193367.
Resolves #193400.

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
Flaky Test Runner was used on any tests changed
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

kibanamachine · 2024-11-28T07:15:22Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7522

[✅] x-pack/test_serverless/functional/test_suites/security/config.context_awareness.ts: 100/100 tests passed.
[✅] x-pack/test_serverless/functional/test_suites/observability/config.context_awareness.ts: 100/100 tests passed.

see run history

kibanamachine · 2024-11-28T19:24:25Z

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7533

[❌] x-pack/test_serverless/functional/test_suites/security/config.context_awareness.ts: 74/100 tests passed.
[✅] x-pack/test_serverless/functional/test_suites/observability/config.context_awareness.ts: 100/100 tests passed.

see run history

kibanamachine · 2024-11-29T04:53:51Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7534

[✅] x-pack/test_serverless/functional/test_suites/security/config.context_awareness.ts: 100/100 tests passed.
[✅] x-pack/test_serverless/functional/test_suites/observability/config.context_awareness.ts: 100/100 tests passed.

see run history

kibanamachine · 2024-11-29T09:07:08Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7535

[✅] x-pack/test_serverless/functional/test_suites/security/config.context_awareness.ts: 100/100 tests passed.
[✅] x-pack/test_serverless/functional/test_suites/observability/config.context_awareness.ts: 100/100 tests passed.

see run history

davismcphee · 2024-11-29T16:04:21Z

src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts

+              const { resetDefaultProfileState: currentResetDefaultProfileState } =
+                internalStateContainer.getState();
+
+              if (currentResetDefaultProfileState.resetId !== resetDefaultProfileState.resetId) {
+                return;
+              }


The issue was that there is a 100ms delay between when we update the app state data source to the new data view in changeDataView (and trigger default state reset) and when the fetch$ callback is actually called (due to the debounce in getFetch$). If within that 100ms period, the previous fetch finishes, this code was being run and clearing out the default state reset here:

kibana/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts

Lines 314 to 320 in 2213f43

// Clear the default profile state flags after the data fetching

// is done so refetches don't reset the state again

internalStateContainer.transitions.setResetDefaultProfileState({

columns: false,

rowHeight: false,

breakdownField: false,

});

This causes the following code which checks for the default state reset in the fetch$ callback to receive all false values for the subsequent fetch, and the correct default state does not get set:

kibana/src/plugins/discover/public/application/main/state_management/discover_data_state_container.ts

Lines 268 to 271 in 2213f43

const { resetDefaultProfileState, dataView } = internalStateContainer.getState();

const defaultProfileState = dataView

? getDefaultProfileState({ profilesManager, resetDefaultProfileState, dataView })

: undefined;

The solution was to ensure the default state reset call associated with this fetch is actually the latest one when this code is run, by using a resetId that gets updated on each reset call. This is not the most elegant solution to this issue and is a symptom of a deeper issue, but it was the least invasive one I could come up with. I believe the long term / correct solution to this issue is to refactor Discover data fetching so logic runs sequentially and is easier to reason about.

elasticmachine · 2024-11-29T16:05:27Z

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

elasticmachine · 2024-12-01T06:31:46Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: b53f68a

Failed CI Steps

FTR Configs #8

Test Failures

[job] [logs] FTR Configs #8 / Exception items APIs Authentication - Complete Tier @serverless @serverlessQA rule_author exception items API behaviors delete exception item should return 200 for rule_author

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
`discover`	812.8KB	812.9KB	+118.0B

History

💚 Build #255479 succeeded 2213f43
💔 Build #255471 failed 25e272f

cc @davismcphee

jughosta

LGTM 👍
Thanks for finding a solution for it!

kibanamachine · 2024-12-03T19:29:24Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12146827618

## Summary This PR fixes a tricky race condition when setting default Discover profile state, which was causing functional test failures such as the linked cell actions tests. Resolves elastic#193367. Resolves elastic#193400. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) (cherry picked from commit 01de887)

kibanamachine · 2024-12-03T19:34:16Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

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

Questions ?

Please refer to the Backport tool documentation

# Backport This will backport the following commits from `main` to `8.x`: - [[Discover] Fix flaky cell actions tests (#202097)](#202097)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Davis McPhee <[email protected]>

## Summary This PR fixes a tricky race condition when setting default Discover profile state, which was causing functional test failures such as the linked cell actions tests. Resolves elastic#193367. Resolves elastic#193400. ### Checklist - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [x] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

Attempt to fix flaky cell actions tests

351e02b

davismcphee self-assigned this Nov 28, 2024

Merge branch 'main' into fix-flaky-cell-actions-tests

f4269d2

Attempt to keep profile resets in sync

25e272f

davismcphee added 3 commits November 29, 2024 02:24

Fix types and tests, remove temp code

6c68149

Remove comment

984bc38

Remove addLog

2213f43

davismcphee commented Nov 29, 2024

View reviewed changes

davismcphee marked this pull request as ready for review November 29, 2024 16:05

davismcphee requested a review from a team as a code owner November 29, 2024 16:05

Merge branch 'main' into fix-flaky-cell-actions-tests

b53f68a

Merge branch 'main' into fix-flaky-cell-actions-tests

1a611db

jughosta approved these changes Dec 3, 2024

View reviewed changes

davismcphee merged commit 01de887 into elastic:main Dec 3, 2024
8 checks passed

davismcphee deleted the fix-flaky-cell-actions-tests branch December 3, 2024 19:29

kibanamachine added the v9.0.0 label Dec 3, 2024

kibanamachine mentioned this pull request Dec 3, 2024

[8.x] [Discover] Fix flaky cell actions tests (#202097) #202805

Merged

kibanamachine added the v8.18.0 label Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Fix flaky cell actions tests #202097

[Discover] Fix flaky cell actions tests #202097

davismcphee commented Nov 28, 2024 •

edited by kibanamachine

Loading

kibanamachine commented Nov 28, 2024

kibanamachine commented Nov 28, 2024

kibanamachine commented Nov 29, 2024

kibanamachine commented Nov 29, 2024

davismcphee Nov 29, 2024 •

edited

Loading

elasticmachine commented Nov 29, 2024

elasticmachine commented Dec 1, 2024

jughosta left a comment

kibanamachine commented Dec 3, 2024

kibanamachine commented Dec 3, 2024

	// Clear the default profile state flags after the data fetching
	// is done so refetches don't reset the state again
	internalStateContainer.transitions.setResetDefaultProfileState({
	columns: false,
	rowHeight: false,
	breakdownField: false,
	});

	const { resetDefaultProfileState, dataView } = internalStateContainer.getState();
	const defaultProfileState = dataView
	? getDefaultProfileState({ profilesManager, resetDefaultProfileState, dataView })
	: undefined;

[Discover] Fix flaky cell actions tests #202097

[Discover] Fix flaky cell actions tests #202097

Conversation

davismcphee commented Nov 28, 2024 • edited by kibanamachine Loading

Summary

Checklist

kibanamachine commented Nov 28, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7522

kibanamachine commented Nov 28, 2024

Flaky Test Runner Stats

🟠 Some tests failed. - kibana-flaky-test-suite-runner#7533

kibanamachine commented Nov 29, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7534

kibanamachine commented Nov 29, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7535

davismcphee Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 29, 2024

elasticmachine commented Dec 1, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

jughosta left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 3, 2024

kibanamachine commented Dec 3, 2024

💚 All backports created successfully

Questions ?

davismcphee commented Nov 28, 2024 •

edited by kibanamachine

Loading

davismcphee Nov 29, 2024 •

edited

Loading