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

App renaming integration test #820

Merged
merged 11 commits into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ jobs:
-w /tests \
-e PLAYWRIGHT_TEST_BASE_URL=http://172.17.0.1:3000/ \
mcr.microsoft.com/playwright:v1.23.1-focal \
yarn e2e:smoke
yarn test:integration
docker-compose -f ./docker/compose/docker-compose.yml down

# push the image
Expand Down
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"release:docker": "dotenv -- node ./scripts/releaseDocker.mjs",
"release:changelog": "dotenv -- node ./scripts/releaseChangelog.js --repo mui-toolpad",
"test:build": "lerna run build --scope @mui/toolpad-core --scope @mui/toolpad-components --stream",
"test:integration": "playwright test --config ./test/integration/playwright.config.ts",
"e2e:smoke": "playwright test --config ./test/e2e-smoke/playwright.config.ts",
"test": "yarn test:build && jest"
},
Expand Down
147 changes: 37 additions & 110 deletions packages/toolpad-app/src/toolpad/Home.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import ToolpadShell from './ToolpadShell';
import getReadableDuration from '../utils/readableDuration';
import EditableText from '../components/EditableText';
import type { AppMeta } from '../server/data';
import useMenu from '../utils/useMenu';

export interface CreateAppDialogProps {
open: boolean;
Expand Down Expand Up @@ -264,64 +265,43 @@ function AppOpenButton({ app, activeDeployment }: AppOpenButtonProps) {
}

interface AppOptionsProps {
menuOpen: boolean;
onClick: (event: React.MouseEvent<HTMLButtonElement>) => void;
onRename: () => void;
onDelete?: () => void;
}

function AppOptions({ menuOpen, onClick }: AppOptionsProps) {
return (
<IconButton
aria-label="settings"
aria-controls={menuOpen ? 'basic-menu' : undefined}
aria-haspopup="true"
aria-expanded={menuOpen ? 'true' : undefined}
onClick={onClick}
>
<MoreVertIcon />
</IconButton>
);
}
function AppOptions({ onRename, onDelete }: AppOptionsProps) {
const { buttonProps, menuProps, onMenuClose } = useMenu();

interface AppMenuProps {
menuAnchorEl: HTMLElement | null;
menuOpen: boolean;
handleMenuClose: () => void;
handleRenameClick: () => void;
handleDeleteClick: () => void;
}
const handleRenameClick = React.useCallback(() => {
onMenuClose();
onRename();
}, [onMenuClose, onRename]);

const handleDeleteClick = React.useCallback(() => {
onMenuClose();
onDelete?.();
}, [onDelete, onMenuClose]);

function AppMenu({
menuAnchorEl,
menuOpen,
handleMenuClose,
handleRenameClick,
handleDeleteClick,
}: AppMenuProps) {
return (
<Menu
id="basic-menu"
anchorEl={menuAnchorEl}
open={menuOpen}
onClose={handleMenuClose}
MenuListProps={{
'aria-labelledby': 'basic-button',
dense: true,
}}
>
{/* Using an onClick on a MenuItem causes accessibility issues, see: https://github.com/mui/material-ui/pull/30145 */}
<MenuItem onClick={handleRenameClick}>
<ListItemIcon>
<DriveFileRenameOutlineIcon />
</ListItemIcon>
<ListItemText>Rename</ListItemText>
</MenuItem>
<MenuItem onClick={handleDeleteClick}>
<ListItemIcon>
<DeleteIcon />
</ListItemIcon>
<ListItemText>Delete</ListItemText>
</MenuItem>
</Menu>
<React.Fragment>
<IconButton {...buttonProps} aria-label="Application menu">
<MoreVertIcon />
</IconButton>
<Menu {...menuProps}>
<MenuItem onClick={handleRenameClick}>
<ListItemIcon>
<DriveFileRenameOutlineIcon />
</ListItemIcon>
<ListItemText>Rename</ListItemText>
</MenuItem>
Copy link
Member

Choose a reason for hiding this comment

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

Should we leave the accessibility issue link in there (assuming the issue persists of course)?

Copy link
Member Author

@Janpot Janpot Aug 18, 2022

Choose a reason for hiding this comment

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

I don't understand the comment. Isn't this how MenuItem is supposed to be used? If it's forcing us to do a workaround in another part of the code then I wouldn't the comment rather belong at that location?

Copy link
Member

Choose a reason for hiding this comment

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

This was intended to be a reference to the bug report which talks about how MenuItem doesn't work as a button when used with a keyboard; we can remove it since we aren't doing anything specifically to tackle it

<MenuItem onClick={handleDeleteClick}>
<ListItemIcon>
<DeleteIcon />
</ListItemIcon>
<ListItemText>Delete</ListItemText>
</MenuItem>
</Menu>
</React.Fragment>
);
}

Expand All @@ -332,31 +312,12 @@ interface AppCardProps {
}

function AppCard({ app, activeDeployment, onDelete }: AppCardProps) {
const [menuAnchorEl, setMenuAnchorEl] = React.useState<null | HTMLElement>(null);
const [editingName, setEditingName] = React.useState<boolean>(false);

const menuOpen = Boolean(menuAnchorEl);

const handleOptionsClick = (event: React.MouseEvent<HTMLButtonElement>) => {
setMenuAnchorEl(event.currentTarget);
};

const handleMenuClose = React.useCallback(() => {
setMenuAnchorEl(null);
}, []);

const handleRenameClick = React.useCallback(() => {
setMenuAnchorEl(null);
const handleRename = React.useCallback(() => {
setEditingName(true);
}, []);

const handleDeleteClick = React.useCallback(() => {
setMenuAnchorEl(null);
if (onDelete) {
onDelete();
}
}, [onDelete]);

return (
<React.Fragment>
<Card
Expand All @@ -369,7 +330,7 @@ function AppCard({ app, activeDeployment, onDelete }: AppCardProps) {
}}
>
<CardHeader
action={<AppOptions menuOpen={menuOpen} onClick={handleOptionsClick} />}
action={<AppOptions onRename={handleRename} onDelete={onDelete} />}
disableTypography
subheader={
<Typography variant="body2" color="text.secondary">
Expand All @@ -396,13 +357,6 @@ function AppCard({ app, activeDeployment, onDelete }: AppCardProps) {
<AppOpenButton app={app} activeDeployment={activeDeployment} />
</CardActions>
</Card>
<AppMenu
menuAnchorEl={menuAnchorEl}
menuOpen={menuOpen}
handleMenuClose={handleMenuClose}
handleRenameClick={handleRenameClick}
handleDeleteClick={handleDeleteClick}
/>
</React.Fragment>
);
}
Expand All @@ -414,32 +368,12 @@ interface AppRowProps {
}

function AppRow({ app, activeDeployment, onDelete }: AppRowProps) {
const [menuAnchorEl, setMenuAnchorEl] = React.useState<null | HTMLElement>(null);

const menuOpen = Boolean(menuAnchorEl);

const [editingName, setEditingName] = React.useState<boolean>(false);

const handleOptionsClick = (event: React.MouseEvent<HTMLButtonElement>) => {
setMenuAnchorEl(event.currentTarget);
};

const handleMenuClose = React.useCallback(() => {
setMenuAnchorEl(null);
}, []);

const handleRenameClick = React.useCallback(() => {
setMenuAnchorEl(null);
const handleRename = React.useCallback(() => {
setEditingName(true);
}, []);

const handleDeleteClick = React.useCallback(() => {
setMenuAnchorEl(null);
if (onDelete) {
onDelete();
}
}, [onDelete]);

return (
<React.Fragment>
<TableRow hover role="row">
Expand All @@ -458,17 +392,10 @@ function AppRow({ app, activeDeployment, onDelete }: AppRowProps) {
<Stack direction="row" spacing={1} justifyContent={'flex-end'}>
<AppEditButton app={app} />
<AppOpenButton app={app} activeDeployment={activeDeployment} />
<AppOptions menuOpen={menuOpen} onClick={handleOptionsClick} />
<AppOptions onRename={handleRename} onDelete={onDelete} />
</Stack>
</TableCell>
</TableRow>
<AppMenu
menuAnchorEl={menuAnchorEl}
menuOpen={menuOpen}
handleMenuClose={handleMenuClose}
handleRenameClick={handleRenameClick}
handleDeleteClick={handleDeleteClick}
/>
</React.Fragment>
);
}
Expand Down
40 changes: 40 additions & 0 deletions test/integration/appRename.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { test, expect, Request, Page } from '@playwright/test';
import generateId from '../utils/generateId';
import * as locators from '../utils/locators';

async function createApp(page: Page, name: string) {
await page.locator('button:has-text("create new")').click();

await page.fill('[role="dialog"] label:has-text("name")', name);

await page.click('[role="dialog"] button:has-text("create")');

await page.waitForNavigation({ url: /\/_toolpad\/app\/[^/]+\/editor\/pages\/[^/]+/ });
}

test('app create/rename flow', async ({ page }) => {
const appName1 = `App ${generateId()}`;
const appName2 = `App ${generateId()}`;
const appName3 = `App ${generateId()}`;

await page.goto('/');
await createApp(page, appName1);

await page.goto('/');
await createApp(page, appName2);

await page.goto('/');

await page.click(`${locators.toolpadHomeAppRow(appName1)} >> [aria-label="Application menu"]`);

await page.click('[role="menuitem"]:has-text("Rename"):visible');
Copy link
Member

Choose a reason for hiding this comment

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

Should all the locators used in this test also be present in locators.ts?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, just the hacky ones that we want to reuse


await page.keyboard.type(appName2);
await page.keyboard.press('Enter');

await expect(page.locator(`text=An app named "${appName2}" already exists`)).toBeVisible();

await page.keyboard.type(appName3);

await expect(page.locator(locators.toolpadHomeAppRow(appName3))).toBeVisible();
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ const config: PlaywrightTestConfig = {
trace: 'on-first-retry',
baseURL: process.env.PLAYWRIGHT_TEST_BASE_URL || 'http://localhost:3000/',
},
globalSetup: '../playwright/global-setup',
globalTeardown: '../playwright/global-teardown',
projects: [
{
name: 'chromium',
Expand Down
10 changes: 5 additions & 5 deletions test/e2e-smoke/basic.spec.ts → test/integration/smoke.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { test, expect, Request } from '@playwright/test';
import generateId from '../utils/generateId';
import * as locators from '../utils/locators';

test('basic app creation flow', async ({ page }) => {
const appName = `App ${String(Math.random()).slice(2)}`;
const appName = `App ${generateId()}`;

await page.goto('/');
const brand = page.locator('data-test-id=brand');
Expand All @@ -19,9 +21,7 @@ test('basic app creation flow', async ({ page }) => {

await page.click('[aria-label="Home"]');

const appRowSelector = `[role="row"] >> has="input[value='${appName}']"`;

await page.click(`${appRowSelector} >> [aria-label="settings"]`);
await page.click(`${locators.toolpadHomeAppRow(appName)} >> [aria-label="Application menu"]`);

await page.click('[role="menuitem"]:has-text("Delete"):visible');

Expand All @@ -37,7 +37,7 @@ test('basic app creation flow', async ({ page }) => {
`[role="dialog"]:has-text('Are you sure you want to delete application "${appName}"') >> button:has-text("delete")`,
);

await page.waitForSelector(appRowSelector, { state: 'detached' });
await page.waitForSelector(locators.toolpadHomeAppRow(appName), { state: 'detached' });

await page.off('request', handleRequest);

Expand Down
18 changes: 18 additions & 0 deletions test/playwright/global-setup.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import * as devDb from '../utils/devDb';

async function globalSetup() {
// eslint-disable-next-line no-underscore-dangle
(globalThis as any).__toolpadTestDbDump = null;

if (!(await devDb.isRunning())) {
return;
}

// eslint-disable-next-line no-console
console.log('Creating a backup of the dev database');

// eslint-disable-next-line no-underscore-dangle
(globalThis as any).__toolpadTestDbDump = await devDb.dump();
}

export default globalSetup;
17 changes: 17 additions & 0 deletions test/playwright/global-teardown.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import * as devDb from '../utils/devDb';

async function globalTeardown() {
// eslint-disable-next-line no-underscore-dangle
const pgDump = (globalThis as any).__toolpadTestDbDump;

if (pgDump) {
// eslint-disable-next-line no-console
console.log('Restoring the dev database');

await devDb.restore(pgDump);
} else if (pgDump !== null) {
throw new Error(`global-setup didn't run correctly`);
}
}

export default globalTeardown;
Loading