From 6074828f928b6c05d70ee2305156e7b2474ffba3 Mon Sep 17 00:00:00 2001 From: Nick Peihl Date: Mon, 23 Oct 2023 15:28:04 -0400 Subject: [PATCH] [Dashboard navigation] Fix flaky test (#167896) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #167713 and #169276 ## Summary It appears the dashboard links were not ready (still loading) when the test was trying to click on them. When they finish loading the element was stale, triggering the StaleElement error. This PR calls the RenderCompleteDispatcher on the Links embeddable when all of the child components have finished loading. In a future PR, we should consider re-factoring such that the async dashboard fetching happens in the LinksComponent and the results are passed as props to the DashboardLinkComponents. This would be more React-like. I resisted making that change in this PR so that we can fix the reporting error for v8.11. ## Flaky test runner 🤞 https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/3636 --------- Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> (cherry picked from commit 513a31f83f454fbc83dfe983877620147c982833) --- .../dashboard_link_component.test.tsx | 75 +++++++++++++++++-- .../dashboard_link_component.tsx | 21 +++++- .../external_link_component.test.tsx | 18 ++++- .../external_link/external_link_component.tsx | 7 ++ .../public/components/links_component.tsx | 24 +++++- .../public/embeddable/links_embeddable.tsx | 24 +++++- .../links/links_navigation.ts | 38 +++++++--- 7 files changed, 180 insertions(+), 27 deletions(-) diff --git a/src/plugins/links/public/components/dashboard_link/dashboard_link_component.test.tsx b/src/plugins/links/public/components/dashboard_link/dashboard_link_component.test.tsx index 90dbdd434e2eb..cb2479bfb5f51 100644 --- a/src/plugins/links/public/components/dashboard_link/dashboard_link_component.test.tsx +++ b/src/plugins/links/public/components/dashboard_link/dashboard_link_component.test.tsx @@ -54,6 +54,9 @@ describe('Dashboard link component', () => { type: 'dashboardLink' as const, }; + const onLoading = jest.fn(); + const onRender = jest.fn(); + let linksEmbeddable: LinksEmbeddable; beforeEach(async () => { window.open = jest.fn(); @@ -76,10 +79,16 @@ describe('Dashboard link component', () => { test('by default uses navigateToApp to open in same tab', async () => { render( - + ); + await waitFor(() => expect(onLoading).toHaveBeenCalledTimes(1)); await waitFor(() => expect(fetchDashboard).toHaveBeenCalledTimes(1)); expect(fetchDashboard).toHaveBeenCalledWith(defaultLinkInfo.destination); expect(getDashboardLocator).toHaveBeenCalledTimes(1); @@ -90,6 +99,7 @@ describe('Dashboard link component', () => { }, linksEmbeddable, }); + await waitFor(() => expect(onRender).toHaveBeenCalledTimes(1)); const link = await screen.findByTestId('dashboardLink--foo'); expect(link).toHaveTextContent('another dashboard'); @@ -104,10 +114,17 @@ describe('Dashboard link component', () => { test('modified click does not trigger event.preventDefault', async () => { render( - + ); + await waitFor(() => expect(onLoading).toHaveBeenCalledTimes(1)); await waitFor(() => expect(fetchDashboard).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(onRender).toHaveBeenCalledTimes(1)); const link = await screen.findByTestId('dashboardLink--foo'); const clickEvent = createEvent.click(link, { ctrlKey: true }); const preventDefault = jest.spyOn(clickEvent, 'preventDefault'); @@ -122,12 +139,19 @@ describe('Dashboard link component', () => { }; render( - + ); + await waitFor(() => expect(onLoading).toHaveBeenCalledTimes(1)); await waitFor(() => expect(fetchDashboard).toHaveBeenCalledTimes(1)); expect(fetchDashboard).toHaveBeenCalledWith(linkInfo.destination); expect(getDashboardLocator).toHaveBeenCalledWith({ link: linkInfo, linksEmbeddable }); + await waitFor(() => expect(onRender).toHaveBeenCalledTimes(1)); const link = await screen.findByTestId('dashboardLink--foo'); expect(link).toBeInTheDocument(); await userEvent.click(link); @@ -147,11 +171,18 @@ describe('Dashboard link component', () => { }; render( - + ); + await waitFor(() => expect(onLoading).toHaveBeenCalledTimes(1)); await waitFor(() => expect(fetchDashboard).toHaveBeenCalledTimes(1)); expect(getDashboardLocator).toHaveBeenCalledWith({ link: linkInfo, linksEmbeddable }); + await waitFor(() => expect(onRender).toHaveBeenCalledTimes(1)); }); test('shows an error when fetchDashboard fails', async () => { @@ -162,10 +193,17 @@ describe('Dashboard link component', () => { }; render( - + ); + await waitFor(() => expect(onLoading).toHaveBeenCalledTimes(1)); await waitFor(() => expect(fetchDashboard).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(onRender).toHaveBeenCalledTimes(1)); const link = await screen.findByTestId('dashboardLink--notfound--error'); expect(link).toHaveTextContent(DashboardLinkStrings.getDashboardErrorLabel()); }); @@ -178,10 +216,17 @@ describe('Dashboard link component', () => { }; render( - + ); + await waitFor(() => expect(onLoading).toHaveBeenCalledTimes(1)); await waitFor(() => expect(fetchDashboard).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(onRender).toHaveBeenCalledTimes(1)); const link = await screen.findByTestId('dashboardLink--bar'); expect(link).toHaveTextContent('current dashboard'); await userEvent.click(link); @@ -192,10 +237,17 @@ describe('Dashboard link component', () => { test('shows dashboard title and description in tooltip', async () => { render( - + ); + await waitFor(() => expect(onLoading).toHaveBeenCalledTimes(1)); await waitFor(() => expect(fetchDashboard).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(onRender).toHaveBeenCalledTimes(1)); const link = await screen.findByTestId('dashboardLink--foo'); await userEvent.hover(link); const tooltip = await screen.findByTestId('dashboardLink--foo--tooltip'); @@ -211,10 +263,17 @@ describe('Dashboard link component', () => { }; render( - + ); + await waitFor(() => expect(onLoading).toHaveBeenCalledTimes(1)); await waitFor(() => expect(fetchDashboard).toHaveBeenCalledTimes(1)); + await waitFor(() => expect(onRender).toHaveBeenCalledTimes(1)); const link = await screen.findByTestId('dashboardLink--foo'); expect(link).toHaveTextContent(label); await userEvent.hover(link); diff --git a/src/plugins/links/public/components/dashboard_link/dashboard_link_component.tsx b/src/plugins/links/public/components/dashboard_link/dashboard_link_component.tsx index 563cf6277c796..38097b478509a 100644 --- a/src/plugins/links/public/components/dashboard_link/dashboard_link_component.tsx +++ b/src/plugins/links/public/components/dashboard_link/dashboard_link_component.tsx @@ -8,7 +8,7 @@ import classNames from 'classnames'; import useAsync from 'react-use/lib/useAsync'; -import React, { useMemo, useState } from 'react'; +import React, { useEffect, useMemo, useState } from 'react'; import useObservable from 'react-use/lib/useObservable'; import { @@ -27,9 +27,13 @@ import { fetchDashboard, getDashboardHref, getDashboardLocator } from './dashboa export const DashboardLinkComponent = ({ link, layout, + onLoading, + onRender, }: { link: Link; layout: LinksLayoutType; + onLoading: () => void; + onRender: () => void; }) => { const linksEmbeddable = useLinks(); const [error, setError] = useState(); @@ -133,6 +137,21 @@ export const DashboardLinkComponent = ({ }; }, [link]); + useEffect(() => { + if (loadingDestinationDashboard || loadingOnClickProps) { + onLoading(); + } else { + onRender(); + } + }, [ + link, + linksEmbeddable, + loadingDestinationDashboard, + loadingOnClickProps, + onLoading, + onRender, + ]); + const id = `dashboardLink--${link.id}`; return loadingDestinationDashboard ? ( diff --git a/src/plugins/links/public/components/external_link/external_link_component.test.tsx b/src/plugins/links/public/components/external_link/external_link_component.test.tsx index 1afdc17c43563..02f984435f8ba 100644 --- a/src/plugins/links/public/components/external_link/external_link_component.test.tsx +++ b/src/plugins/links/public/components/external_link/external_link_component.test.tsx @@ -17,6 +17,8 @@ import { ExternalLinkComponent } from './external_link_component'; import { coreServices } from '../../services/kibana_services'; import { DEFAULT_URL_DRILLDOWN_OPTIONS } from '@kbn/ui-actions-enhanced-plugin/public'; +const onRender = jest.fn(); + describe('external link component', () => { const defaultLinkInfo = { destination: 'https://example.com', @@ -38,10 +40,15 @@ describe('external link component', () => { test('by default opens in new tab', async () => { render( - + ); + expect(onRender).toBeCalledTimes(1); const link = await screen.findByTestId('externalLink--foo'); expect(link).toBeInTheDocument(); await userEvent.click(link); @@ -55,9 +62,10 @@ describe('external link component', () => { }; render( - + ); + expect(onRender).toBeCalledTimes(1); const link = await screen.findByTestId('externalLink--foo'); expect(link).toHaveTextContent('https://example.com'); const clickEvent = createEvent.click(link, { ctrlKey: true }); @@ -73,9 +81,10 @@ describe('external link component', () => { }; render( - + ); + expect(onRender).toBeCalledTimes(1); const link = await screen.findByTestId('externalLink--foo'); await userEvent.click(link); expect(coreServices.application.navigateToUrl).toBeCalledTimes(1); @@ -89,9 +98,10 @@ describe('external link component', () => { }; render( - + ); + expect(onRender).toBeCalledTimes(1); const link = await screen.findByTestId('externalLink--foo--error'); expect(link).toBeDisabled(); /** diff --git a/src/plugins/links/public/components/external_link/external_link_component.tsx b/src/plugins/links/public/components/external_link/external_link_component.tsx index 9c2231fe6b711..ac409cfbac4cf 100644 --- a/src/plugins/links/public/components/external_link/external_link_component.tsx +++ b/src/plugins/links/public/components/external_link/external_link_component.tsx @@ -7,6 +7,7 @@ */ import React, { useMemo, useState } from 'react'; +import useMount from 'react-use/lib/useMount'; import { UrlDrilldownOptions, @@ -21,12 +22,18 @@ import { Link, LinksLayoutType, LINKS_VERTICAL_LAYOUT } from '../../../common/co export const ExternalLinkComponent = ({ link, layout, + onRender, }: { link: Link; layout: LinksLayoutType; + onRender: () => void; }) => { const [error, setError] = useState(); + useMount(() => { + onRender(); + }); + const linkOptions = useMemo(() => { return { ...DEFAULT_URL_DRILLDOWN_OPTIONS, diff --git a/src/plugins/links/public/components/links_component.tsx b/src/plugins/links/public/components/links_component.tsx index 9400dc9fe7308..c72c0db04fd57 100644 --- a/src/plugins/links/public/components/links_component.tsx +++ b/src/plugins/links/public/components/links_component.tsx @@ -6,7 +6,8 @@ * Side Public License, v 1. */ -import React, { useMemo } from 'react'; +import React, { useEffect, useMemo } from 'react'; +import useMap from 'react-use/lib/useMap'; import { EuiListGroup, EuiPanel } from '@elastic/eui'; import { useLinks } from '../embeddable/links_embeddable'; import { ExternalLinkComponent } from './external_link/external_link_component'; @@ -25,6 +26,22 @@ export const LinksComponent = () => { const links = linksEmbeddable.select((state) => state.componentState.links); const layout = linksEmbeddable.select((state) => state.componentState.layout); + const [linksLoading, { set: setLinkIsLoading }] = useMap( + Object.fromEntries( + (links ?? []).map((link) => { + return [link.id, true]; + }) + ) + ); + + useEffect(() => { + if (Object.values(linksLoading).includes(true)) { + linksEmbeddable.onLoading(); + } else { + linksEmbeddable.onRender(); + } + }, [linksLoading, linksEmbeddable]); + const orderedLinks = useMemo(() => { if (!links) return []; return memoizedGetOrderedLinkList(links); @@ -42,18 +59,21 @@ export const LinksComponent = () => { key={currentLink.id} link={currentLink} layout={layout ?? LINKS_VERTICAL_LAYOUT} + onLoading={() => setLinkIsLoading(currentLink.id, true)} + onRender={() => setLinkIsLoading(currentLink.id, false)} /> ) : ( setLinkIsLoading(currentLink.id, false)} /> ), }, }; }, {}); - }, [links, layout]); + }, [links, layout, setLinkIsLoading]); return ( diff --git a/test/functional/apps/dashboard_elements/links/links_navigation.ts b/test/functional/apps/dashboard_elements/links/links_navigation.ts index c4adf6e2c7042..d9f6f5b2bb6a9 100644 --- a/test/functional/apps/dashboard_elements/links/links_navigation.ts +++ b/test/functional/apps/dashboard_elements/links/links_navigation.ts @@ -18,13 +18,17 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { const kibanaServer = getService('kibanaServer'); const dashboardAddPanel = getService('dashboardAddPanel'); - const { dashboard, common, timePicker } = getPageObjects(['dashboard', 'common', 'timePicker']); + const { dashboard, common, header, timePicker } = getPageObjects([ + 'dashboard', + 'common', + 'header', + 'timePicker', + ]); const FROM_TIME = 'Oct 22, 2018 @ 00:00:00.000'; const TO_TIME = 'Dec 3, 2018 @ 00:00:00.000'; - // Failing: See https://github.com/elastic/kibana/issues/167713 - describe.skip('links panel navigation', () => { + describe('links panel navigation', () => { before(async () => { await kibanaServer.savedObjects.cleanStandardList(); await security.testUser.setRoles([ @@ -84,6 +88,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { it('should disable link if dashboard does not exist', async () => { await dashboard.loadSavedDashboard('links 001'); + await dashboard.waitForRenderComplete(); expect(await testSubjects.exists('dashboardLink--link004--error')).to.be(true); expect(await testSubjects.isEnabled('dashboardLink--link004--error')).to.be(false); }); @@ -96,10 +101,13 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { * but should not override the date range. */ await dashboard.loadSavedDashboard('links 002'); - await testSubjects.click('dashboardLink--link001'); + await dashboard.waitForRenderComplete(); + await testSubjects.clickWhenNotDisabled('dashboardLink--link001'); + await header.waitUntilLoadingHasFinished(); expect(await dashboard.getDashboardIdFromCurrentUrl()).to.equal( '0930f310-5bc2-11ee-9a85-7b86504227bc' ); + await dashboard.waitForRenderComplete(); // Should pass the filters expect(await filterBar.getFilterCount()).to.equal(2); const filterLabels = await filterBar.getFiltersLabel(); @@ -127,10 +135,14 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { * but should not pass its filters. */ await dashboard.loadSavedDashboard('links 001'); - await testSubjects.click('dashboardLink--link002'); + await dashboard.waitForRenderComplete(); + await testSubjects.clickWhenNotDisabled('dashboardLink--link002'); + await header.waitUntilLoadingHasFinished(); expect(await dashboard.getDashboardIdFromCurrentUrl()).to.equal( '24751520-5bc2-11ee-9a85-7b86504227bc' ); + + await dashboard.waitForRenderComplete(); // Should pass the date range const time = await timePicker.getTimeConfig(); expect(time.start).to.be('Oct 31, 2018 @ 00:00:00.000'); @@ -157,7 +169,9 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { * to dashboard links003. */ await dashboard.loadSavedDashboard('links 001'); - await testSubjects.click('dashboardLink--link003'); + await dashboard.waitForRenderComplete(); + await testSubjects.clickWhenNotDisabled('dashboardLink--link003'); + await header.waitUntilLoadingHasFinished(); // Should have opened another tab const windowHandlers = await browser.getAllWindowHandles(); @@ -167,6 +181,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { '27398c50-5bc2-11ee-9a85-7b86504227bc' ); + await dashboard.waitForRenderComplete(); // Should not pass any filters expect((await filterBar.getFiltersLabel()).length).to.equal(0); @@ -180,6 +195,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { describe('external links', () => { before(async () => { await dashboard.loadSavedDashboard('dashboard with external links'); + await header.waitUntilLoadingHasFinished(); }); afterEach(async () => { @@ -197,8 +213,10 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { expect(isDisabled).to.be('true'); }); - it('should create an external link when openInNewTab is enabled', async () => { - await testSubjects.click('externalLink--link999'); + // TODO We should not be using an external website for our tests. This will be flaky + // if external network connectivity issues exist. + it.skip('should create an external link when openInNewTab is enabled', async () => { + await testSubjects.clickWhenNotDisabled('externalLink--link999'); // Should have opened another tab const windowHandlers = await browser.getAllWindowHandles(); @@ -208,8 +226,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { expect(currentUrl).to.be('https://example.com/1'); }); - it('should open in same tab when openInNewTab is disabled', async () => { - await testSubjects.click('externalLink--link888'); + it.skip('should open in same tab when openInNewTab is disabled', async () => { + await testSubjects.clickWhenNotDisabled('externalLink--link888'); // Should have opened in the same tab const windowHandlers = await browser.getAllWindowHandles();