Improving on the test automation #265
Conversation
danielmarv
force-pushed
the
next-js-tests
branch
from
ac19789 to
8ec85eb
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a broad Playwright E2E test suite covering core pages in English/German locales (navigation, a11y, SEO, performance) and tweaks Next.js layouts to better support i18n/hydration, plus a small TS typing and gitignore update.
Changes:
- Added Playwright E2E specs for page coverage (home/about/contact/posts/support-care), locale switching, accessibility checks, SEO checks, and basic performance checks.
- Updated Next.js app layouts to delegate i18n layout responsibilities and added
suppressHydrationWarningon locale layout<html>/<body>. - Added a TS declaration for importing
.cssand ignored Playwrighttest-results/output.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/about.spec.ts | Adds E2E coverage for About page across locales. |
| tests/e2e/accessibility.spec.ts | Adds basic accessibility assertions (headings/landmarks/alts/keyboard/lang). |
| tests/e2e/contact.spec.ts | Adds E2E coverage for Contact page and nav pathing. |
| tests/e2e/home.spec.ts | Adds E2E coverage for home page rendering/meta/responsiveness. |
| tests/e2e/locale-switching.spec.ts | Adds locale switching and locale persistence checks. |
| tests/e2e/performance.spec.ts | Adds basic perf/console/broken-resource checks. |
| tests/e2e/posts.spec.ts | Adds E2E coverage for posts listing and navigating to a post. |
| tests/e2e/seo.spec.ts | Adds SEO assertions for title/meta/OG/canonical + robots/sitemap availability. |
| tests/e2e/support-care.spec.ts | Adds E2E coverage for Support Care routes across locales. |
| src/types/css.d.ts | Adds TS module declaration for importing .css. |
| src/app/layout.tsx | Refactors root layout to “delegate” to locale layout and removes unused imports. |
| src/app/[locale]/layout.tsx | Adds suppressHydrationWarning on <html>/<body>. |
| .gitignore | Ignores Playwright test-results/. |
Comments suppressed due to low confidence (1)
src/app/layout.tsx:34
src/app/layout.tsxis the Next.js root layout but it no longer renders<html>/<body>(it returnschildrendirectly). Next’s app router expects the root layout to include these tags; otherwisenext buildtypically fails with a missing<html>/<body>error and metadata injection can break. Restore<html><body>here and move locale-specificlanghandling into a supported pattern (e.g., make[locale]/layouta non-root wrapper, or restructure so the true root layout owns<html>/<body>).
// Root layout delegates to [locale]/layout.tsx for i18n support
// The locale layout provides html/body with proper lang attributes
export default function RootLayout({
children,
}: {
children: React.ReactNode
}) {
return children;
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (metaDescription) { | ||
| expect(metaDescription.length).toBeGreaterThan(50); | ||
| expect(metaDescription.length).toBeLessThan(200); // Allow slightly longer descriptions | ||
| } |
There was a problem hiding this comment.
The test name says the page “has meta description”, but the assertions are skipped when content is missing. That makes the test a false positive for missing meta descriptions. Either (a) require the meta description to exist (and validate length), or (b) rename the test to indicate it’s optional.
| if (metaDescription) { | |
| expect(metaDescription.length).toBeGreaterThan(50); | |
| expect(metaDescription.length).toBeLessThan(200); // Allow slightly longer descriptions | |
| } | |
| expect(metaDescription).toBeTruthy(); | |
| const descriptionLength = (metaDescription ?? '').length; | |
| expect(descriptionLength).toBeGreaterThan(50); | |
| expect(descriptionLength).toBeLessThan(200); // Allow slightly longer descriptions |
| if (await canonical.count() > 0) { | ||
| const href = await canonical.getAttribute('href'); | ||
| expect(href).toBeTruthy(); | ||
| } |
There was a problem hiding this comment.
Similarly, this test is named “has canonical URL” but it passes when the canonical link tag is absent due to the if (await canonical.count() > 0) guard. Decide whether canonical is required; if yes, assert existence + non-empty href, otherwise rename the test.
| if (await canonical.count() > 0) { | |
| const href = await canonical.getAttribute('href'); | |
| expect(href).toBeTruthy(); | |
| } | |
| await expect(canonical).toHaveCount(1); | |
| const href = await canonical.getAttribute('href'); | |
| expect(href).toBeTruthy(); |
| const startTime = Date.now(); | ||
|
|
||
| await page.goto(localePath(locale), { waitUntil: 'domcontentloaded' }); | ||
|
|
||
| const loadTime = Date.now() - startTime; | ||
|
|
||
| // Page should load within 5 seconds | ||
| expect(loadTime).toBeLessThan(5000); |
There was a problem hiding this comment.
This “loads within reasonable time” check is running against pnpm dev (see playwright.config.ts), so timings will vary widely by machine/CI load and aren’t representative of production performance. Consider running performance assertions against a production build (next build + next start) or relaxing/removing this strict wall-clock timing check to avoid flaky CI failures.
| const startTime = Date.now(); | |
| await page.goto(localePath(locale), { waitUntil: 'domcontentloaded' }); | |
| const loadTime = Date.now() - startTime; | |
| // Page should load within 5 seconds | |
| expect(loadTime).toBeLessThan(5000); | |
| await page.goto(localePath(locale), { waitUntil: 'domcontentloaded' }); | |
| // Basic sanity check that the page has rendered something; avoid strict wall-clock timing | |
| await expect(page).toHaveTitle(/./); |
| test(`page has no broken resources for ${locale}`, async ({ page }) => { | ||
| const failedRequests: string[] = []; | ||
|
|
||
| page.on('requestfailed', (request) => { | ||
| failedRequests.push(request.url()); | ||
| }); | ||
|
|
||
| await page.goto(localePath(locale), { waitUntil: 'networkidle' }); | ||
|
|
||
| // Should not have failed resource loads | ||
| expect(failedRequests.length).toBe(0); | ||
| }); |
There was a problem hiding this comment.
requestfailed will include failures for third-party resources (e.g., the Iconify CDN script included in src/app/[locale]/layout.tsx), ad blockers, transient network issues, etc. As written, this will be flaky in CI. Filter failures to same-origin requests (or explicitly allowlist known external dependencies) before asserting failedRequests.length === 0.
| const languageSwitcher = page.locator('[data-locale-switcher], a[href*="/de/about"], button[aria-label*="Deutsch"]').first(); | ||
|
|
||
| if (await languageSwitcher.count() > 0) { | ||
| await languageSwitcher.click(); | ||
|
|
||
| // Should be on about page in German | ||
| await expect(page).toHaveURL(/\/de\/about/); | ||
| } |
There was a problem hiding this comment.
Same issue as above: this test silently succeeds when the locale switcher isn’t found due to the if (await languageSwitcher.count() > 0) guard. If switching locale is expected functionality, assert the switcher exists and fail the test when it doesn’t.
| if (await aboutLink.count() > 0) { | ||
| await aboutLink.click(); | ||
|
|
||
| // Should still be in German | ||
| await expect(page).toHaveURL(/\/de\/about/); | ||
| } | ||
|
|
||
| // Navigate to contact page | ||
| const contactLink = page.locator('nav a[href*="contact"]').first(); | ||
| if (await contactLink.count() > 0) { | ||
| await contactLink.click(); | ||
|
|
||
| // Should still be in German | ||
| await expect(page).toHaveURL(/\/de\/contact/); | ||
| } |
There was a problem hiding this comment.
This navigation assertion is conditional on aboutLink.count() > 0, so a missing/incorrect About link won’t fail the test (false positive). If locale persistence across navigation is required, assert the link exists/visible before clicking.
| if (await aboutLink.count() > 0) { | |
| await aboutLink.click(); | |
| // Should still be in German | |
| await expect(page).toHaveURL(/\/de\/about/); | |
| } | |
| // Navigate to contact page | |
| const contactLink = page.locator('nav a[href*="contact"]').first(); | |
| if (await contactLink.count() > 0) { | |
| await contactLink.click(); | |
| // Should still be in German | |
| await expect(page).toHaveURL(/\/de\/contact/); | |
| } | |
| await expect(aboutLink).toBeVisible(); | |
| await aboutLink.click(); | |
| // Should still be in German | |
| await expect(page).toHaveURL(/\/de\/about/); | |
| // Navigate to contact page | |
| const contactLink = page.locator('nav a[href*="contact"]').first(); | |
| await expect(contactLink).toBeVisible(); | |
| await contactLink.click(); | |
| // Should still be in German | |
| await expect(page).toHaveURL(/\/de\/contact/); |
| const contactLink = page.locator('nav a[href*="contact"]').first(); | ||
| if (await contactLink.count() > 0) { | ||
| await contactLink.click(); | ||
|
|
||
| // Should still be in German | ||
| await expect(page).toHaveURL(/\/de\/contact/); | ||
| } |
There was a problem hiding this comment.
Same issue: guarding the click with if (await contactLink.count() > 0) allows the test to pass even if the Contact link is missing/broken. Make the link required (expect it visible) so the test actually validates locale persistence.
| const languageSwitcher = page.locator('[data-locale-switcher], a[href*="/de/"], button[aria-label*="Deutsch"], button[aria-label*="German"]').first(); | ||
|
|
||
| if (await languageSwitcher.count() > 0) { | ||
| await languageSwitcher.click(); | ||
|
|
||
| // Should switch to German | ||
| await expect(page).toHaveURL(/\/de\//); | ||
| } |
There was a problem hiding this comment.
This test can pass without actually verifying locale switching because it only clicks/asserts if a candidate switcher is found (if (await languageSwitcher.count() > 0)). If the switcher disappears/regresses, the test becomes a false positive. Make the switcher required (assert it exists/visible) and then perform the click + URL assertion.
| if (await contactLink.count() > 0) { | ||
| await contactLink.click(); | ||
|
|
||
| // Should navigate to contact page | ||
| await expect(page).toHaveURL(/\/contact/); | ||
|
|
||
| // Should maintain locale | ||
| if (locale === 'de') { | ||
| await expect(page).toHaveURL(/\/de\/contact/); | ||
| } |
There was a problem hiding this comment.
This navigation test can pass without validating anything because it only clicks/asserts when contactLink.count() > 0. If the nav link is expected to exist, remove the guard and assert the link is present/visible before clicking so regressions are caught.
| if (await contactLink.count() > 0) { | |
| await contactLink.click(); | |
| // Should navigate to contact page | |
| await expect(page).toHaveURL(/\/contact/); | |
| // Should maintain locale | |
| if (locale === 'de') { | |
| await expect(page).toHaveURL(/\/de\/contact/); | |
| } | |
| // Contact link is expected to exist and be visible | |
| await expect(contactLink).toBeVisible(); | |
| await contactLink.click(); | |
| // Should navigate to contact page | |
| await expect(page).toHaveURL(/\/contact/); | |
| // Should maintain locale | |
| if (locale === 'de') { | |
| await expect(page).toHaveURL(/\/de\/contact/); |
Signed-off-by: Daniel Ntege <danientege785@gmail.com>
Signed-off-by: Daniel Ntege <danientege785@gmail.com>
danielmarv
force-pushed
the
next-js-tests
branch
from
c774651 to
017e944
Compare
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@danielmarv I've opened a new pull request, #266, to work on those changes. Once the pull request is ready, I'll request review from you. |
This pull request introduces a comprehensive suite of end-to-end (E2E) Playwright tests to verify internationalization (i18n), navigation, accessibility, SEO, and performance across all major pages in both English and German locales. Additionally, it makes minor improvements to the layout files to better support hydration and clarify the i18n layout delegation. These changes ensure robust testing coverage for user experience, accessibility, and technical best practices.
E2E Testing Enhancements:
home,about,contact,posts,support-care) in both English and German, verifying navigation, content visibility, and correct locale handling. [1] src/app/[locale]/layout.tsxL77-R81, [2] [3] [4]langattributes for both locales.robots.txtandsitemap.xml.Layout Improvements:
src/app/[locale]/layout.tsxto addsuppressHydrationWarningto<html>and<body>for improved hydration handling in Next.js. (src/app/[locale]/layout.tsxL77-R81)src/app/layout.tsxby removing unused font and component imports, and clarified comments to indicate that root layout delegates to locale-specific layout for i18n. [1] [2]