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

tests(e2e): improve login util stability #20223

Closed
wants to merge 13 commits into from
Closed
4 changes: 2 additions & 2 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ jobs:
uses: ./.github/actions/yarn-nm-install

- name: Install Playwright Browsers
run: npx playwright@1.42.1 install --with-deps
run: npx playwright@1.43.1 install --with-deps

- name: Monorepo build
uses: ./.github/actions/run-build
Expand Down Expand Up @@ -216,7 +216,7 @@ jobs:
uses: ./.github/actions/yarn-nm-install

- name: Install Playwright Browsers
run: npx playwright@1.42.1 install --with-deps
run: npx playwright@1.43.1 install --with-deps

- name: Monorepo build
uses: ./.github/actions/run-build
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
"@commitlint/cli": "19.2.0",
"@commitlint/config-conventional": "19.1.0",
"@commitlint/prompt-cli": "19.2.0",
"@playwright/test": "1.42.1",
"@playwright/test": "1.43.1",
"@strapi/admin-test-utils": "workspace:*",
"@strapi/eslint-config": "0.2.0",
"@swc/cli": "0.3.10",
Expand Down
10 changes: 10 additions & 0 deletions tests/e2e/constants.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
'use strict';

const { CUSTOM_TRANSFER_TOKEN_ACCESS_KEY } = require('./app-template/template/src/constants');

// NOTE: anything included here needs to be included in all test datasets exports
Expand Down Expand Up @@ -41,9 +43,17 @@ const ALLOWED_CONTENT_TYPES = [
const ADMIN_EMAIL_ADDRESS = '[email protected]';
const ADMIN_PASSWORD = 'Testing123!';

const TITLE_LOGIN = 'Strapi Admin';
const TITLE_HOME = 'Homepage | Strapi';
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 is so we stop having so many conflicts merging develop <-> v5/main


const URL_ROOT = '/admin';

module.exports = {
ADMIN_EMAIL_ADDRESS,
ADMIN_PASSWORD,
ALLOWED_CONTENT_TYPES,
CUSTOM_TRANSFER_TOKEN_ACCESS_KEY,
TITLE_LOGIN,
TITLE_HOME,
URL_ROOT,
};
30 changes: 23 additions & 7 deletions tests/e2e/tests/admin/api-tokens.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { test, expect } from '@playwright/test';
import { test, expect, Page } from '@playwright/test';
import { login } from '../../utils/login';
import { navToHeader } from '../../utils/shared';
import { resetDatabaseAndImportDataFromPath } from '../../utils/dts-import';
Expand All @@ -22,12 +22,19 @@ const createAPIToken = async (page, tokenName, duration, type) => {
};

test.describe('API Tokens', () => {
test.beforeEach(async ({ page }) => {
let page: Page;

// For some reason, logging in after each token is extremely flaky, so we'll just do it once
test.beforeAll(async ({ browser }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
page = await browser.newPage();
await login({ page });
});

test.afterAll(async ({ browser }) => {
page.close();
});

// Test token creation
const testCases = [
['30-day Read-only token', '30 days', 'Read-only'],
Expand All @@ -37,16 +44,25 @@ test.describe('API Tokens', () => {
['unlimited token', 'Unlimited', 'Full access'],
];
for (const [name, duration, type] of testCases) {
test(`A user should be able to create a ${name}`, async ({ page }) => {
test(`A user should be able to create a ${name}`, async ({}) => {
await createAPIToken(page, name, duration, type);
});
}

test('Created tokens list page should be correct', async ({ page }) => {
test('Created tokens list page should be correct', async ({}) => {
await createAPIToken(page, 'my test token', 'unlimited', 'Full access');

// if we don't wait until createdAt is at least 1s, we see "NaN" for the timestamp
// TODO: fix the bug and remove this
await page.waitForTimeout(1100);

await navToHeader(page, ['Settings', 'API Tokens'], 'API Tokens');

const row = page.getByRole('gridcell', { name: 'my test token', exact: true });
await expect(row).toBeVisible();
const nameCell = page.getByRole('gridcell', { name: 'my test token', exact: true });
await expect(nameCell).toBeVisible();

// Locate the parent of nameCell and then search within it for the timestamp
const parentRow = nameCell.locator('xpath=..');
await expect(parentRow).toContainText(/\d+ (second|minute)s? ago/);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

adds check present on transfer tokens that was missing from api tokens

});
});
35 changes: 21 additions & 14 deletions tests/e2e/tests/admin/login.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { test, expect } from '@playwright/test';
import { resetDatabaseAndImportDataFromPath } from '../../utils/dts-import';
import { toggleRateLimiting } from '../../utils/rate-limit';
import { ADMIN_EMAIL_ADDRESS, ADMIN_PASSWORD } from '../../constants';
import { ADMIN_EMAIL_ADDRESS, ADMIN_PASSWORD, TITLE_HOME, TITLE_LOGIN } from '../../constants';
import { login } from '../../utils/login';

test.describe('Login', () => {
Expand All @@ -14,26 +14,33 @@ test.describe('Login', () => {
test('A user should be able to log in with or without making their authentication persistent', async ({
page,
context,
browser,
}) => {
// Test without making user authentication persistent
await login({ page });
await expect(page).toHaveTitle('Homepage | Strapi');
await expect(page).toHaveTitle(TITLE_HOME);

// Close the page and open a new one
await page.close();

page = await context.newPage();
await page.goto('/admin');
await expect(page).toHaveTitle('Strapi Admin');
const nonPersistentPage = await context.newPage();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-using the page object was rarely but sometimes causing a "page already closed" when attempting to navigate after

await Promise.all([
nonPersistentPage.waitForLoadState('networkidle'),
nonPersistentPage.goto('/admin'),
]);
await expect(nonPersistentPage).toHaveTitle(TITLE_LOGIN);

// Test with making user authentication persistent
await login({ page, rememberMe: true });
await expect(page).toHaveTitle('Homepage | Strapi');

await page.close();

page = await context.newPage();
await page.goto('/admin');
await expect(page).toHaveTitle('Homepage | Strapi');
await login({ page: nonPersistentPage, rememberMe: true });
await expect(nonPersistentPage).toHaveTitle(TITLE_HOME);

// Close the page and open a new one
await nonPersistentPage.close();
const persistentPage = await context.newPage();
await Promise.all([
persistentPage.waitForLoadState('networkidle'),
persistentPage.goto('/admin'),
]);
await expect(persistentPage).toHaveTitle(TITLE_HOME);
});
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/admin/logout.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { login } from '../../utils/login';
test.describe('Log Out', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
11 changes: 7 additions & 4 deletions tests/e2e/tests/admin/signup.spec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { test, expect } from '@playwright/test';

import { resetDatabaseAndImportDataFromPath } from '../../utils/dts-import';
import { ADMIN_EMAIL_ADDRESS, ADMIN_PASSWORD } from '../../constants';
import { ADMIN_EMAIL_ADDRESS, ADMIN_PASSWORD, TITLE_HOME } from '../../constants';

/**
* Fill in the sign up form with valid values
Expand All @@ -28,7 +28,7 @@ test.describe('Sign Up', () => {
await resetDatabaseAndImportDataFromPath('without-admin.tar', (cts) =>
cts.filter((ct) => ct !== 'plugin::i18n.locale')
);
await page.goto('/admin');
await page.goto('/admin', { waitUntil: 'networkidle' });
await fillValidSignUpForm({ page });
});

Expand Down Expand Up @@ -96,8 +96,11 @@ test.describe('Sign Up', () => {
test('a user should be able to signup when the strapi instance starts fresh', async ({
page,
}) => {
await page.getByRole('button', { name: "Let's start" }).click();
await Promise.all([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using Promise.all with waitForLoadState and a click action works better to give clicks the time necessary to load the next page

page.waitForLoadState('networkidle'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should make networkidle a const somewhere, it gets used quite a bit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's a typed value for waitForLoadState I'll leave it for now, but I actually think the next time I'm in here messing with things I'm going to add a util for navigating + waiting so you can just call one line instead of 2+, nobody should have to remember to do this to write a stable test

page.getByRole('button', { name: "Let's start" }).click(),
]);

await expect(page).toHaveTitle('Homepage | Strapi');
await expect(page).toHaveTitle(TITLE_HOME);
});
});
27 changes: 19 additions & 8 deletions tests/e2e/tests/admin/transfer-tokens.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { test, expect } from '@playwright/test';
import { test, expect, Page } from '@playwright/test';
import { login } from '../../utils/login';
import { resetDatabaseAndImportDataFromPath } from '../../utils/dts-import';
import { navToHeader } from '../../utils/shared';
Expand Down Expand Up @@ -26,12 +26,19 @@ const createTransferToken = async (page, tokenName, duration, type) => {
};

test.describe('Transfer Tokens', () => {
test.beforeEach(async ({ page }) => {
let page: Page;

Copy link
Member

Choose a reason for hiding this comment

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

Is this a pattern we want to promote? you effectively make the last test dependent on the previous because you're not starting with a "fresh" slate every time.

I'd be cautious about promoting this pattern as I don't think it's something we want to get into the habit of doing in the long term?

Copy link
Member

Choose a reason for hiding this comment

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

Since we aren't clearing the db after each run anymore and have multiple tokens, we need to be more specific with what we're looking for

My point exactly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I think the bad practice is the core of the fix here. I will take a look and see if there's an alternative I can find, but otherwise I may just add a warning not to repeat that pattern in other tests.

// For some reason, logging in after each token is extremely flaky, so we'll just do it once
test.beforeAll(async ({ browser }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
page = await browser.newPage();
await login({ page });
});

test.afterAll(async ({ browser }) => {
page.close();
});

// Test token creation
const testCases = [
['30-day push token', '30 days', 'Push'],
Expand All @@ -43,12 +50,12 @@ test.describe('Transfer Tokens', () => {
['unlimited token', 'Unlimited', 'Full access'],
];
for (const [name, duration, type] of testCases) {
test(`A user should be able to create a ${name}`, async ({ page }) => {
test(`A user should be able to create a ${name}`, async ({}) => {
await createTransferToken(page, name, duration, type);
});
}

test('Created tokens list page should be correct', async ({ page }) => {
test('Created tokens list page should be correct', async ({}) => {
await createTransferToken(page, 'my test token', 'unlimited', 'Full access');

// if we don't wait until createdAt is at least 1s, we see "NaN" for the timestamp
Expand All @@ -57,9 +64,13 @@ test.describe('Transfer Tokens', () => {

await navToHeader(page, ['Settings', 'Transfer Tokens'], 'Transfer Tokens');

const row = page.getByRole('gridcell', { name: 'my test token', exact: true });
await expect(row).toBeVisible();
await expect(page.getByText(/\d+ (second|minute)s? ago/)).toBeVisible();
const nameCell = page.getByRole('gridcell', { name: 'my test token', exact: true });
await expect(nameCell).toBeVisible();

// Locate the parent of nameCell and then search within it for the timestamp
const parentRow = nameCell.locator('xpath=..');
await expect(parentRow).toContainText(/\d+ (second|minute)s? ago/);

Comment on lines +70 to +72
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we aren't clearing the db after each run anymore and have multiple tokens, we need to be more specific with what we're looking for

// TODO: expand on this test, it could check edit and delete icons
});
});
1 change: 0 additions & 1 deletion tests/e2e/tests/content-manager/cloning.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { findAndClose } from '../../utils/shared';
test.describe('Cloning', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/content-manager/editview.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { findAndClose } from '../../utils/shared';
test.describe('Edit View', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/content-manager/listview.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { resetDatabaseAndImportDataFromPath } from '../../utils/dts-import';
test.describe('List View', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
2 changes: 0 additions & 2 deletions tests/e2e/tests/content-manager/uniqueness.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ test.describe('Uniqueness', () => {
test.beforeEach(async ({ page }) => {
// Reset the DB and also specify that we are wiping all entries of the unique content type each time
await resetDatabaseAndImportDataFromPath('with-admin.tar');

await page.goto('/admin');
await login({ page });

await page.getByRole('link', { name: 'Content Manager' }).click();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ const addEntryToRelease = async ({ page, releaseName }: { page: Page; releaseNam
describeOnCondition(/*edition === 'EE'*/ false)('Release page', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });

await page.getByRole('link', { name: 'Releases' }).click();
Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/content-releases/releases-page.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const edition = process.env.STRAPI_DISABLE_EE === 'true' ? 'CE' : 'EE';
describeOnCondition(edition === 'EE')('Releases page', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ test.describe('Create collection type', () => {
test.beforeEach(async ({ page }) => {
await resetFiles();
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });

await page.getByRole('link', { name: 'Content-Type Builder' }).click();
Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/content-type-builder/ctb-edit-view.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { resetDatabaseAndImportDataFromPath } from '../../utils/dts-import';
test.describe('Edit View CTB', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/content-type-builder/tutorial.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { resetDatabaseAndImportDataFromPath } from '../../utils/dts-import';
test.describe('Tutorial', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/i18n/editview.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { findAndClose } from '../../utils/shared';
test.describe('Edit view', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/i18n/listview.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { login } from '../../utils/login';
test.describe('List view', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/i18n/settings.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ test.describe('Settings', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await prunePermissions(page);
await page.goto('/admin');
await login({ page });
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/review-workflows/content-manager.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const edition = process.env.STRAPI_DISABLE_EE === 'true' ? 'CE' : 'EE';
describeOnCondition(edition === 'EE')('content-manager', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down
1 change: 0 additions & 1 deletion tests/e2e/tests/review-workflows/settings.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const edition = process.env.STRAPI_DISABLE_EE === 'true' ? 'CE' : 'EE';
describeOnCondition(edition === 'EE')('settings', () => {
test.beforeEach(async ({ page }) => {
await resetDatabaseAndImportDataFromPath('with-admin.tar');
await page.goto('/admin');
await login({ page });
});

Expand Down