Skip to content

Migrate MostRead e2es to Next.js app#13855

Merged
amoore108 merged 7 commits intolatestfrom
WS-203-most-read-e2e-migration-to-nextjs
Apr 1, 2026
Merged

Migrate MostRead e2es to Next.js app#13855
amoore108 merged 7 commits intolatestfrom
WS-203-most-read-e2e-migration-to-nextjs

Conversation

@amoore108
Copy link
Copy Markdown
Contributor

@amoore108 amoore108 commented Mar 31, 2026

Part of JIRA: https://bbc.atlassian.net/browse/WS-203

Summary

Migrate MostRead e2es from Express app to Next.js app

Deletes the errorPage404 tests from the Express app as the Next.js app already has these tests

Code changes

  • Moves Cypress e2es from Express app to Next.js app for Most Read pages
  • Uplifts test to TS
  • Removes AMP related tests as AMP is no longer support for Most Read pages as they render as Topic Pages.
  • Removes Express errorPage404 tests

Testing

  1. List the steps required to test this PR.

Useful Links

@amoore108 amoore108 self-assigned this Mar 31, 2026
@amoore108 amoore108 marked this pull request as ready for review March 31, 2026 13:15
Copilot AI review requested due to automatic review settings March 31, 2026 13:15
Comment on lines +104 to +107
atiAnalytics: {
...data.pageData.metadata.atiAnalytics,
pageTitle: data.pageData.title || null,
},
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm going to add this logic in the API. This is just a temporary measure.

path: '/arabic/popular/read',
service: 'arabic',
runforEnv: ['local', 'test', 'live'],
runforEnv: ['test', 'live'],
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is there a benefit to having at least one test on local? It looks like we've removed them all.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Pidgin still has a local test for the page and ATI data:

runforEnv: ['local', 'test', 'live'],

runforEnv: ['local', 'test', 'live'],

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR migrates Most Read Cypress E2E coverage into the ws-nextjs-app (Next.js) test setup and aligns Most Read SSR metadata so ATI analytics has a pageTitle for the Most Read route (which is implemented as a Topic page under the hood).

Changes:

  • Updates Most Read Next.js SSR props to populate metadata.atiAnalytics.pageTitle from the page title.
  • Moves/rewires Most Read Cypress suites to use ws-nextjs-app/cypress helpers and updated assertion exports.
  • Removes AMP Most Read E2E coverage and limits suites to canonical + lite where applicable.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
ws-nextjs-app/pages/[service]/popular/[read]/[[...variant]].page.tsx Adds atiAnalytics.pageTitle to Most Read SSR pageData.metadata.
ws-nextjs-app/cypress/e2e/mostReadPage/testsForCanonicalOnly.ts Fixes helper import path for Chartbeat tests after migration.
ws-nextjs-app/cypress/e2e/mostReadPage/tests.ts Switches to default export for Most Read assertions.
ws-nextjs-app/cypress/e2e/mostReadPage/mostReadAssertions.ts Simplifies assertions to canonical-only (removes AMP-only branch) and exports default.
ws-nextjs-app/cypress/e2e/mostReadPage/index.cy.ts Repoints imports to Next.js app Cypress helpers, removes AMP suites, updates ATI suite config and pageType constant.
Comments suppressed due to low confidence (1)

ws-nextjs-app/cypress/e2e/mostReadPage/index.cy.ts:72

  • atiAnalyticsTestSuites is passed into runTestsForPage without being typed/cast to TestDataType[], but it contains siteId values as numbers (e.g. 70). runTestsForPage's TestDataType currently defines siteId?: string, so this will fail TypeScript checking unless you either (a) cast like other e2e entrypoints do (e.g. topicPage/index.cy.ts uses ] as unknown as TestDataType[];) or (b) fix the underlying types to accept number for siteId (which also matches how the ATI assertions compare it).

@LukasFrm LukasFrm self-requested a review April 1, 2026 08:02
@amoore108 amoore108 merged commit f5b19c2 into latest Apr 1, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants