Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

unskips application leave confirm & application deep links tests #168741

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
0feeb53
unskips tests failing in CI
TinaHeiligers Oct 12, 2023
a49010c
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 12, 2023
5fc9a09
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 13, 2023
c61ef3d
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 14, 2023
4713ed5
Use consistent nav link title, name
TinaHeiligers Oct 15, 2023
15096a6
adds debug log for current URL
TinaHeiligers Oct 15, 2023
4c080ea
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Oct 16, 2023
59995c8
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 16, 2023
cc6f57f
Improves tests
TinaHeiligers Oct 16, 2023
6f2f392
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 16, 2023
41e0dcb
Merge branch 'main' into skipped-tests-app-leave-deep-links
TinaHeiligers Oct 16, 2023
06e050d
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 16, 2023
eb25bbc
Ensure app navigates to app home url
TinaHeiligers Oct 17, 2023
d0ae7d2
Swap test order to force browser response
TinaHeiligers Oct 17, 2023
16bb946
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 17, 2023
cd2f7a5
updates test plugin and page objects methods to accomodate latest beh…
TinaHeiligers Oct 17, 2023
77ceb45
Merge branch 'skipped-tests-app-leave-deep-links' of github.com:TinaH…
TinaHeiligers Oct 17, 2023
ca94111
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 17, 2023
18abd6b
check exists for the correct modal test subject
TinaHeiligers Oct 17, 2023
fac7c79
Merge branch 'skipped-tests-app-leave-deep-links' of github.com:TinaH…
TinaHeiligers Oct 17, 2023
1e08998
Refactors tests, helpers and plugin to take delayed rendering into ac…
TinaHeiligers Oct 18, 2023
473c1fd
Increase waitFor timeout more
TinaHeiligers Oct 18, 2023
d725990
Change data test subject
TinaHeiligers Oct 18, 2023
81579c4
increase timeout even more
TinaHeiligers Oct 18, 2023
8d1c8a2
Move nav click event into retry
TinaHeiligers Oct 18, 2023
0f5277f
Merge branch 'main' into skipped-tests-app-leave-deep-links
kibanamachine Oct 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions test/functional/page_objects/common_page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -411,9 +411,18 @@ export class CommonPageObject extends FtrService {
* Clicks cancel button on modal
* @param overlayWillStay pass in true if your test will show multiple modals in succession
*/
async clickCancelOnModal(overlayWillStay = true) {
async clickCancelOnModal(overlayWillStay = true, ignorePageLeaveWarning = false) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted to allow re-calling similar test flow for both navigation options (cancel | confirm)

this.log.debug('Clicking modal cancel');
await this.testSubjects.click('confirmModalCancelButton');
await this.testSubjects.exists('confirmModalTitleText');

await this.retry.try(async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have to wait for the modal to fade in, which takes time, likely even longer in CI

const warning = await this.testSubjects.exists('confirmModalTitleText');
if (warning) {
await this.testSubjects.click(
ignorePageLeaveWarning ? 'confirmModalConfirmButton' : 'confirmModalCancelButton'
);
}
});
if (!overlayWillStay) {
await this.ensureModalOverlayHidden();
}
Expand Down
1 change: 1 addition & 0 deletions test/functional/services/apps_menu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export class AppsMenuService extends FtrService {
if (!(await this.testSubjects.exists('collapsibleNav'))) {
await this.testSubjects.click('toggleNavButton');
}
await this.testSubjects.exists('collapsibleNav');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This helper wasn't confirming a 'happy-path', we should assert that the nav flyout does appear before progressing to use any of the nav methods (clicking an app title for example)

}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { Plugin, CoreSetup } from '@kbn/core/public';
import { Plugin, CoreSetup, DEFAULT_APP_CATEGORIES } from '@kbn/core/public';

export class CoreAppLeavePlugin
implements Plugin<CoreAppLeavePluginSetup, CoreAppLeavePluginStart>
Expand All @@ -15,6 +15,8 @@ export class CoreAppLeavePlugin
core.application.register({
id: 'appleave1',
title: 'AppLeave 1',
appRoute: '/app/appleave1',
category: DEFAULT_APP_CATEGORIES.kibana,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cheated here a little, adding the test plugins to the Analytics section to find them without having to scroll through all the other plugins in the nav flyout.

async mount(params) {
const { renderApp } = await import('./application');
params.onAppLeave((actions) => actions.confirm('confirm-message', 'confirm-title'));
Expand All @@ -24,9 +26,11 @@ export class CoreAppLeavePlugin
core.application.register({
id: 'appleave2',
title: 'AppLeave 2',
appRoute: '/app/appleave2',
category: DEFAULT_APP_CATEGORIES.kibana,
async mount(params) {
const { renderApp } = await import('./application');
params.onAppLeave((actions) => actions.default());
params.onAppLeave((actions) => actions.confirm('confirm-message', 'confirm-title'));
return renderApp('AppLeave 2', params);
},
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ const PageA = () => (
<EuiPageHeader>
<EuiPageHeaderSection>
<EuiTitle size="l">
<h1>DL Page A</h1>
<h1>DL page A</h1>
</EuiTitle>
</EuiPageHeaderSection>
</EuiPageHeader>
Expand All @@ -70,7 +70,7 @@ const PageB = () => (
<EuiPageHeader>
<EuiPageHeaderSection>
<EuiTitle size="l">
<h1>DL Page B</h1>
<h1>DL page B</h1>
</EuiTitle>
</EuiPageHeaderSection>
</EuiPageHeader>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export class CorePluginDeepLinksPlugin
},
{
id: 'pageA',
title: 'DL Page A',
title: 'DL page A',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test plugins and test suites themselves weren't using consistent case, making it hard to debug if the tests fail. Inconsistent case can also easily be missed in future refactors so keep it simple: Use consistent case.

path: '/page-a',
navLinkStatus: AppNavLinkStatus.visible,
},
Expand All @@ -39,15 +39,15 @@ export class CorePluginDeepLinksPlugin
deepLinks: [
{
id: 'pageB',
title: 'DL Page B',
title: 'DL page B',
path: '/page-b',
navLinkStatus: AppNavLinkStatus.visible,
},
],
},
{
id: 'pageC',
title: 'DL Page C',
title: 'DL page C',
path: '/page-c',
// navLinkStatus hidden by default
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
const testSubjects = getService('testSubjects');
const retry = getService('retry');
const esArchiver = getService('esArchiver');
const log = getService('log');

const loadingScreenNotShown = async () =>
expect(await testSubjects.exists('kbnLoadingMessage')).to.be(false);
Expand All @@ -35,12 +36,20 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
const waitForUrlToBe = (pathname?: string, search?: string) => {
const expectedUrl = getKibanaUrl(pathname, search);
return retry.waitFor(`Url to be ${expectedUrl}`, async () => {
return (await browser.getCurrentUrl()) === expectedUrl;
const currentUrl = await browser.getCurrentUrl();
log?.debug(`waiting for currentUrl ${currentUrl} to be expectedUrl ${expectedUrl}`);
return currentUrl === expectedUrl;
});
};

// Failing: See https://github.com/elastic/kibana/issues/166893
describe.skip('application deep links navigation', function describeDeepLinksTests() {
const navigateToAppLinks = async (subject: string) => {
if (!(await testSubjects.exists(subject))) {
log.debug(`side nav in app not in DOM`);
}
await testSubjects.click(subject);
};

describe('application deep links navigation', function describeDeepLinksTests() {
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Oct 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elastic/appex-sharedux I could use a hand in figuring out why tests using appMenu.clickLink fail in CI and pass locally. The most recent changes to relevant components seems to be with CollapsibleNav but that was 4 months ago.

The appLeave test failed because the modal wasn't visible.

before(async () => {
await esArchiver.emptyKibanaIndex();
await PageObjects.common.navigateToApp('dl');
Expand All @@ -51,28 +60,29 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
});

it('should navigate to page A when navlink is clicked', async () => {
await appsMenu.clickLink('DL Page A');
await navigateToAppLinks('dlNavPageA');
await waitForUrlToBe('/app/dl/page-a');
await loadingScreenNotShown();
await testSubjects.existOrFail('dlAppPageA');
await testSubjects.existOrFail('dlNavPageA');
});

it('should be able to use the back button to navigate back to previous deep link', async () => {
await browser.goBack();
await waitForUrlToBe('/app/dl/home');
await loadingScreenNotShown();
await testSubjects.existOrFail('dlAppHome');
await testSubjects.existOrFail('dlNavHome');
});

it('should navigate to nested page B when navlink is clicked', async () => {
await appsMenu.clickLink('DL Page B');
await navigateToAppLinks('dlNavDeepPageB');
await waitForUrlToBe('/app/dl/page-b');
await loadingScreenNotShown();
await testSubjects.existOrFail('dlAppPageB');
await testSubjects.existOrFail('dlNavDeepPageB');
});

it('should navigate to Home when navlink is clicked inside the defined category group', async () => {
await appsMenu.clickLink('DL Home', { category: 'securitySolution' });
await navigateToAppLinks('dlAppHome');
await waitForUrlToBe('/app/dl/home');
await loadingScreenNotShown();
await testSubjects.existOrFail('dlAppHome');
Expand All @@ -82,14 +92,14 @@ export default function ({ getService, getPageObjects }: PluginFunctionalProvide
await testSubjects.click('dlNavDeepPageB');
await waitForUrlToBe('/app/dl/page-b');
await loadingScreenNotShown();
await testSubjects.existOrFail('dlAppPageB');
await testSubjects.existOrFail('dlNavDeepPageB');
});

it('should navigate to nested page A using navigateToApp deepLinkId', async () => {
await testSubjects.click('dlNavDeepPageAById');
await waitForUrlToBe('/app/dl/page-a');
await loadingScreenNotShown();
await testSubjects.existOrFail('dlAppPageA');
await testSubjects.existOrFail('dlNavPageA');
});

it('should not display hidden deep links', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
* Side Public License, v 1.
*/

import url from 'url';
import expect from '@kbn/expect';
import url from 'url';
import { PluginFunctionalProviderContext } from '../../services';

const getKibanaUrl = (pathname?: string, search?: string) =>
Expand All @@ -20,30 +20,96 @@ const getKibanaUrl = (pathname?: string, search?: string) =>
});

export default function ({ getService, getPageObjects }: PluginFunctionalProviderContext) {
const PageObjects = getPageObjects(['common']);
const PageObjects = getPageObjects(['common', 'header']);
const browser = getService('browser');
const appsMenu = getService('appsMenu');
const log = getService('log');
const retry = getService('retry');
const testSubjects = getService('testSubjects');
const config = getService('config');

const waitForUrlToBe = async (pathname?: string, search?: string) => {
const expectedUrl = getKibanaUrl(pathname, search);
return await retry.waitFor(`Url to be ${expectedUrl}`, async () => {
const currentUrl = await browser.getCurrentUrl();
log.debug(`waiting for currentUrl ${currentUrl} to be expectedUrl ${expectedUrl}`);
return currentUrl === expectedUrl;
});
};

const ensureModalOpen = async (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test helper to make it easier to recall

defaultTryTimeout: number,
attempts: number,
timeMultiplier: number,
action: 'cancel' | 'confirm',
linkText: string = 'home'
): Promise<void> => {
let isConfirmCancelModalOpenState = false;

// Failing: See https://github.com/elastic/kibana/issues/75963
// Failing: See https://github.com/elastic/kibana/issues/166838
describe.skip('application using leave confirmation', () => {
await retry.tryForTime(defaultTryTimeout * timeMultiplier, async () => {
await appsMenu.clickLink(linkText);
isConfirmCancelModalOpenState = await testSubjects.exists('confirmModalTitleText', {
allowHidden: true,
timeout: defaultTryTimeout * timeMultiplier,
});
});
if (isConfirmCancelModalOpenState) {
log.debug(`defaultTryTimeout * ${timeMultiplier} is long enough`);
return action === 'cancel'
? await PageObjects.common.clickCancelOnModal(true, false)
: await PageObjects.common.clickConfirmOnModal();
} else {
log.debug(`defaultTryTimeout * ${timeMultiplier} is not long enough`);
return await ensureModalOpen(
defaultTryTimeout,
(attempts = attempts > 0 ? attempts - 1 : 0),
(timeMultiplier = timeMultiplier < 10 ? timeMultiplier + 1 : 10),
action,
linkText
);
}
};

describe('application using leave confirmation', () => {
Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Oct 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test suite needed a complete overhaul to account for increased delays that various background tasks perform (ebt data collection, fleet setup etc).

const defaultTryTimeout = config.get('timeouts.try');
const attempts = 5;
describe('when navigating to another app', () => {
const timeMultiplier = 10;
beforeEach(async () => {
await PageObjects.common.navigateToApp('home');
});
it('prevents navigation if user click cancel on the confirmation dialog', async () => {
await PageObjects.common.navigateToApp('appleave1');
await appsMenu.clickLink('AppLeave 2');
await PageObjects.header.waitUntilLoadingHasFinished();
await waitForUrlToBe('/app/appleave1');

await testSubjects.existOrFail('appLeaveConfirmModal');
await PageObjects.common.clickCancelOnModal(false);
expect(await browser.getCurrentUrl()).to.eql(getKibanaUrl('/app/appleave1'));
await ensureModalOpen(defaultTryTimeout, attempts, timeMultiplier, 'cancel', 'AppLeave 2');
await PageObjects.header.waitUntilLoadingHasFinished();
await retry.waitFor('navigate to appleave1', async () => {
const currentUrl = await browser.getCurrentUrl();
log.debug(`currentUrl ${currentUrl}`);
return currentUrl.includes('appleave1');
});
const currentUrl = await browser.getCurrentUrl();
expect(currentUrl).to.contain('appleave1');
await PageObjects.common.navigateToApp('home');
});

it('allows navigation if user click confirm on the confirmation dialog', async () => {
await PageObjects.common.navigateToApp('appleave1');
await appsMenu.clickLink('AppLeave 2');
await PageObjects.header.waitUntilLoadingHasFinished();
await waitForUrlToBe('/app/appleave1');

await testSubjects.existOrFail('appLeaveConfirmModal');
await PageObjects.common.clickConfirmOnModal();
expect(await browser.getCurrentUrl()).to.eql(getKibanaUrl('/app/appleave2'));
await ensureModalOpen(defaultTryTimeout, attempts, timeMultiplier, 'confirm', 'AppLeave 2');
await PageObjects.header.waitUntilLoadingHasFinished();
await retry.waitFor('navigate to appleave1', async () => {
const currentUrl = await browser.getCurrentUrl();
log.debug(`currentUrl ${currentUrl}`);
return currentUrl.includes('appleave2');
});
const currentUrl = await browser.getCurrentUrl();
expect(currentUrl).to.contain('appleave2');
await PageObjects.common.navigateToApp('home');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Navigating away completely is the only way to ensure the browser doesn't get "stuck" in a rendering frenzy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yikes 😄 , do you know what the source is of the rendering frenzy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the side nav, the react hooks and the multiple ways navigation can occur. Mostly, though, it's the browser + delays in fading in and out.

});
});
});
Expand Down
Loading