Skip to content

Commit

Permalink
[8.13] Fix how sample data test install state is determined in test (#…
Browse files Browse the repository at this point in the history
…178529) (#178601)

# Backport

This will backport the following commits from `main` to `8.13`:
- [Fix how sample data test install state is determined in test
(#178529)](#178529)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Eyo O.
Eyo","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-03-12T19:16:13Z","message":"Fix
how sample data test install state is determined in test (#178529)\n\n##
Summary\r\nCloses
https://github.com/elastic/kibana/issues/112103\r\n\r\nMake sample data
install status available to be read by test util, as\r\ndocumented by
@gsoldevila in the issue referenced above. The issue\r\nhappens because
there's a slight delay that really can't be walked\r\naround where the
install status in this particular instance is still\r\n'installed' but
the call to mark the sample data completes so there's\r\nthat flicker
where the `remove` element is displayed momentarily because\r\nthe
component doesn't quite received the update to the sample
data's\r\ninstall status immediately.\r\n\r\nThe proposed fix opts to
complement the current way of determining if\r\nany sample data is
installed using the newly introduced `data-status`\r\nattribute, here we
wait till the result of clicking the remove button\r\nactually triggers
a change in the value of the install state of said\r\nsample data, which
in turn is reflected in the value of `data-status`\r\nalongside checking
that the remove button exists.\r\n\r\n### Checklist\r\n<!--\r\nDelete
any items that are not applicable to this PR.\r\n\r\n- [ ] Any text
added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials -->\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- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n<!--\r\n- [ ] Any UI touched in this
PR is usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n###
Risk Matrix\r\n\r\nDelete this section if it is not applicable to this
PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other
developers to\r\nidentify risks that should be tested prior to the
change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider
some of the following examples\r\nand how they may potentially impact
the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes
|\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n|
Multiple Spaces&mdash;unexpected behavior in non-default Kibana
Space.\r\n| Low | High | Integration tests will verify that all features
are still\r\nsupported in non-default Kibana Space and when user
switches between\r\nspaces. |\r\n| Multiple nodes&mdash;Elasticsearch
polling might have race conditions\r\nwhen multiple Kibana nodes are
polling for the same tasks. | High | Low\r\n| Tasks are idempotent, so
executing them multiple times will not result\r\nin logical error, but
will degrade performance. To test for this case we\r\nadd plenty of unit
tests around this logic and document manual testing\r\nprocedure. |\r\n|
Code should gracefully handle cases when feature X or plugin Y
are\r\ndisabled. | Medium | High | Unit tests will verify that any
feature flag\r\nor plugin combination still results in our service
operational. |\r\n| [See more potential
risk\r\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
|\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] 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-->","sha":"910188e4e0baf6b2dbfbab88f4f4466a077f87c2","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix","backport:skip","Team:SharedUX","v8.13.0","v8.14.0"],"number":178529,"url":"https://github.com/elastic/kibana/pull/178529","mergeCommit":{"message":"Fix
how sample data test install state is determined in test (#178529)\n\n##
Summary\r\nCloses
https://github.com/elastic/kibana/issues/112103\r\n\r\nMake sample data
install status available to be read by test util, as\r\ndocumented by
@gsoldevila in the issue referenced above. The issue\r\nhappens because
there's a slight delay that really can't be walked\r\naround where the
install status in this particular instance is still\r\n'installed' but
the call to mark the sample data completes so there's\r\nthat flicker
where the `remove` element is displayed momentarily because\r\nthe
component doesn't quite received the update to the sample
data's\r\ninstall status immediately.\r\n\r\nThe proposed fix opts to
complement the current way of determining if\r\nany sample data is
installed using the newly introduced `data-status`\r\nattribute, here we
wait till the result of clicking the remove button\r\nactually triggers
a change in the value of the install state of said\r\nsample data, which
in turn is reflected in the value of `data-status`\r\nalongside checking
that the remove button exists.\r\n\r\n### Checklist\r\n<!--\r\nDelete
any items that are not applicable to this PR.\r\n\r\n- [ ] Any text
added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials -->\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- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n<!--\r\n- [ ] Any UI touched in this
PR is usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n###
Risk Matrix\r\n\r\nDelete this section if it is not applicable to this
PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other
developers to\r\nidentify risks that should be tested prior to the
change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider
some of the following examples\r\nand how they may potentially impact
the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes
|\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n|
Multiple Spaces&mdash;unexpected behavior in non-default Kibana
Space.\r\n| Low | High | Integration tests will verify that all features
are still\r\nsupported in non-default Kibana Space and when user
switches between\r\nspaces. |\r\n| Multiple nodes&mdash;Elasticsearch
polling might have race conditions\r\nwhen multiple Kibana nodes are
polling for the same tasks. | High | Low\r\n| Tasks are idempotent, so
executing them multiple times will not result\r\nin logical error, but
will degrade performance. To test for this case we\r\nadd plenty of unit
tests around this logic and document manual testing\r\nprocedure. |\r\n|
Code should gracefully handle cases when feature X or plugin Y
are\r\ndisabled. | Medium | High | Unit tests will verify that any
feature flag\r\nor plugin combination still results in our service
operational. |\r\n| [See more potential
risk\r\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
|\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] 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-->","sha":"910188e4e0baf6b2dbfbab88f4f4466a077f87c2"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","labelRegex":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/178529","number":178529,"mergeCommit":{"message":"Fix
how sample data test install state is determined in test (#178529)\n\n##
Summary\r\nCloses
https://github.com/elastic/kibana/issues/112103\r\n\r\nMake sample data
install status available to be read by test util, as\r\ndocumented by
@gsoldevila in the issue referenced above. The issue\r\nhappens because
there's a slight delay that really can't be walked\r\naround where the
install status in this particular instance is still\r\n'installed' but
the call to mark the sample data completes so there's\r\nthat flicker
where the `remove` element is displayed momentarily because\r\nthe
component doesn't quite received the update to the sample
data's\r\ninstall status immediately.\r\n\r\nThe proposed fix opts to
complement the current way of determining if\r\nany sample data is
installed using the newly introduced `data-status`\r\nattribute, here we
wait till the result of clicking the remove button\r\nactually triggers
a change in the value of the install state of said\r\nsample data, which
in turn is reflected in the value of `data-status`\r\nalongside checking
that the remove button exists.\r\n\r\n### Checklist\r\n<!--\r\nDelete
any items that are not applicable to this PR.\r\n\r\n- [ ] Any text
added follows [EUI's
writing\r\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\r\nsentence case text and includes
[i18n\r\nsupport](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)\r\n-
[
]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas
added for features that require explanation or tutorials -->\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- [x] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n<!--\r\n- [ ] Any UI touched in this
PR is usable by keyboard only (learn more\r\nabout [keyboard
accessibility](https://webaim.org/techniques/keyboard/))\r\n- [ ] Any UI
touched in this PR does not create any new axe failures\r\n(run axe in
browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n-
[ ] If a plugin configuration key changed, check if it needs to
be\r\nallowlisted in the cloud and added to the
[docker\r\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\r\n-
[ ] This renders correctly on smaller devices using a
responsive\r\nlayout. (You can test this [in
your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n-
[ ] This was checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)\r\n\r\n\r\n###
Risk Matrix\r\n\r\nDelete this section if it is not applicable to this
PR.\r\n\r\nBefore closing this PR, invite QA, stakeholders, and other
developers to\r\nidentify risks that should be tested prior to the
change/feature\r\nrelease.\r\n\r\nWhen forming the risk matrix, consider
some of the following examples\r\nand how they may potentially impact
the change:\r\n\r\n| Risk | Probability | Severity | Mitigation/Notes
|\r\n\r\n|---------------------------|-------------|----------|-------------------------|\r\n|
Multiple Spaces&mdash;unexpected behavior in non-default Kibana
Space.\r\n| Low | High | Integration tests will verify that all features
are still\r\nsupported in non-default Kibana Space and when user
switches between\r\nspaces. |\r\n| Multiple nodes&mdash;Elasticsearch
polling might have race conditions\r\nwhen multiple Kibana nodes are
polling for the same tasks. | High | Low\r\n| Tasks are idempotent, so
executing them multiple times will not result\r\nin logical error, but
will degrade performance. To test for this case we\r\nadd plenty of unit
tests around this logic and document manual testing\r\nprocedure. |\r\n|
Code should gracefully handle cases when feature X or plugin Y
are\r\ndisabled. | Medium | High | Unit tests will verify that any
feature flag\r\nor plugin combination still results in our service
operational. |\r\n| [See more potential
risk\r\nexamples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
|\r\n\r\n\r\n### For maintainers\r\n\r\n- [ ] 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-->","sha":"910188e4e0baf6b2dbfbab88f4f4466a077f87c2"}}]}]
BACKPORT-->
  • Loading branch information
eokoneyo authored Mar 13, 2024
1 parent 44df8f2 commit daffde5
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 86 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 22 additions & 8 deletions packages/home/sample_data_card/src/footer/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import React from 'react';
import React, { useCallback } from 'react';
import { SampleDataSet, InstalledStatus } from '@kbn/home-sample-data-types';
import { INSTALLED_STATUS, UNINSTALLED_STATUS } from '../constants';

Expand All @@ -28,13 +28,27 @@ export interface Props {
* Displays the appropriate Footer component based on the status of the Sample Data Set.
*/
export const Footer = ({ sampleDataSet, onAction }: Props) => {
if (sampleDataSet.status === INSTALLED_STATUS) {
return <RemoveFooter onRemove={(id) => onAction(id, UNINSTALLED_STATUS)} {...sampleDataSet} />;
}
const renderContent = useCallback(() => {
if (sampleDataSet.status === INSTALLED_STATUS) {
return (
<RemoveFooter onRemove={(id) => onAction(id, UNINSTALLED_STATUS)} {...sampleDataSet} />
);
}

if (sampleDataSet.status === UNINSTALLED_STATUS) {
return <InstallFooter onInstall={(id) => onAction(id, INSTALLED_STATUS)} {...sampleDataSet} />;
}
if (sampleDataSet.status === UNINSTALLED_STATUS) {
return (
<InstallFooter onInstall={(id) => onAction(id, INSTALLED_STATUS)} {...sampleDataSet} />
);
}

return <DisabledFooter {...sampleDataSet} />;
return <DisabledFooter {...sampleDataSet} />;
}, [onAction, sampleDataSet]);

return (
// the data-status attribute is added to solve issues with failing test,
// see https://github.com/elastic/kibana/issues/112103
<div data-status={sampleDataSet.status} style={{ display: 'contents' }}>
{renderContent()}
</div>
);
};
5 changes: 4 additions & 1 deletion test/functional/page_objects/home_page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,12 @@ export class HomePageObject extends FtrService {

async isSampleDataSetInstalled(id: string) {
const sampleDataCard = await this.testSubjects.find(`sampleDataSetCard${id}`);
const installStatus = await (
await sampleDataCard.findByCssSelector('[data-status]')
).getAttribute('data-status');
const deleteButton = await sampleDataCard.findAllByTestSubject(`removeSampleDataSet${id}`);
this.log.debug(`Sample data installed: ${deleteButton.length > 0}`);
return deleteButton.length > 0;
return installStatus === 'installed' && deleteButton.length > 0;
}

async isWelcomeInterstitialDisplayed() {
Expand Down
3 changes: 1 addition & 2 deletions x-pack/test/functional_execution_context/tests/browser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ import { assertLogContains, isExecutionContextLog, readLogFile } from '../test_u
export default function ({ getService, getPageObjects }: FtrProviderContext) {
const PageObjects = getPageObjects(['common', 'dashboard', 'header', 'home', 'timePicker']);

// FLAKY: https://github.com/elastic/kibana/issues/112103
describe.skip('Browser apps', () => {
describe('Browser apps', () => {
let logs: Ecs[];
const retry = getService('retry');

Expand Down

0 comments on commit daffde5

Please sign in to comment.