From 9f07bd82c0f217a8959a456b73c73787d8507c40 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sun, 16 Aug 2026 11:53:28 +0100 Subject: [PATCH 1/5] Updated Shade and i18n setup documentation (#29993) Updated docs for correctness & coherence. --- apps/shade/README.md | 26 +++++++++++++++++++++----- packages/i18n/README.md | 14 ++++++++++---- 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/apps/shade/README.md b/apps/shade/README.md index 752e9f3b4fe..2ba15c08ff3 100644 --- a/apps/shade/README.md +++ b/apps/shade/README.md @@ -2,9 +2,12 @@ Ghost Design System that can be used by micro-frontends. -## Usage +## Usage in embedded Ghost Admin apps -Shade is consumed internally across Ghost apps. The package is currently private; when published, consumption will follow standard npm usage. +Ghost Admin provides Shade's CSS and application wrapper centrally. Embedded +Admin surfaces, including `apps/admin` and `apps/activitypub`, must not import +`@tryghost/shade/styles.css` or add another `ShadeApp` wrapper. Import +components from their layer-specific subpaths: Example: @@ -16,6 +19,12 @@ export function Example() { } ``` +## Usage in standalone surfaces + +The setup below applies only to a standalone surface that owns its complete +application and CSS entry points. Shade is currently a private package; when +published, consumption will follow standard npm usage. + CSS-first styling contract: ```css @@ -44,9 +53,16 @@ import {ShadeApp} from '@tryghost/shade/app'; This is a monorepo package. -Follow the instructions for the top-level repo. -1. `git clone` this repo & `cd` into it as usual -2. Run `pnpm` to install top-level dependencies. +For a fresh clone or worktree, follow the setup instructions from the repository +root: + +```bash +corepack enable pnpm +pnpm setup +``` + +After setup, run the package commands below from `apps/shade` or with +`pnpm --filter @tryghost/shade `. Local docs with Storybook: diff --git a/packages/i18n/README.md b/packages/i18n/README.md index 9b95ed1cd5a..608ebff5bf0 100644 --- a/packages/i18n/README.md +++ b/packages/i18n/README.md @@ -6,12 +6,18 @@ i18n translations for Ghost This is a monorepo package. -Follow the instructions for the top-level repo. -1. `git clone` this repo & `cd` into it as usual -2. Run `pnpm` to install top-level dependencies. +For a fresh clone or worktree, follow the setup instructions from the repository +root: + +```bash +corepack enable pnpm +pnpm setup +``` + +After setup, run package commands from `packages/i18n` or with +`pnpm --filter @tryghost/i18n `. ## Test - `pnpm lint` run just eslint - `pnpm test` run lint and tests - From a175b0ac6efbef66de6d48091e3f1b14d81f9878 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sun, 16 Aug 2026 11:57:36 +0100 Subject: [PATCH 2/5] Changed CodeRabbit to prioritise important feedback (#29994) Three months of review data showed that low-priority review-body feedback was frequently ignored. This starts a controlled, non-blocking experiment using the quiet profile to prioritise higher-value findings without adding path instructions, documentation mappings, or merge enforcement at the same time. Review details are enabled so finding volume, suppression, tool usage, severity, validity, and follow-up can be compared with the existing baseline. Renovate and Dependabot are explicitly excluded using their exact GitHub identities because their automated dependency PRs are not a useful part of this review-quality experiment. --- .coderabbit.yaml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 3bb38413bf2..94464dc19a8 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,11 +1,17 @@ # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json reviews: + profile: quiet + review_details: true high_level_summary: false collapse_walkthrough: false changed_files_summary: false sequence_diagrams: false estimate_code_review_effort: false poem: false + auto_review: + ignore_usernames: + - tryghost-renovate[bot] + - dependabot[bot] pre_merge_checks: docstrings: mode: "off" From ead70448308c5471d129b4eef2d2b54abbf99964 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sun, 16 Aug 2026 13:26:47 +0100 Subject: [PATCH 3/5] Improved internal package migration safeguards (#29944) Updates to the migration skills based on a trial run --- .../skills/migrate-internal-package/SKILL.md | 25 +++++++- .../references/history-and-merge.md | 58 +++++++++++++++---- 2 files changed, 71 insertions(+), 12 deletions(-) diff --git a/.agents/skills/migrate-internal-package/SKILL.md b/.agents/skills/migrate-internal-package/SKILL.md index 208eae9eae1..ca161ef3953 100644 --- a/.agents/skills/migrate-internal-package/SKILL.md +++ b/.agents/skills/migrate-internal-package/SKILL.md @@ -27,8 +27,16 @@ Ghost worktree from the freshly fetched `origin/main`. Run history-changing commands individually or in a fail-fast shell. A failed `git worktree add` must not be followed by `git subtree add` in whichever -checkout happens to be current. Stop if the destination branch or path already -exists and inspect that state rather than reusing it implicitly. +checkout happens to be current. Before choosing a branch or path, list existing +worktrees and matching local and remote branches. Use a migration-specific slug, +for example `codex/import--from-`, rather than a generic name. + +If the destination branch or path already exists, stop and inspect its +cleanliness, base, divergence, source split and attached worktree. Do not mutate, +delete or silently reuse it. Present the evidence and ask the user whether to +resume, preserve and supersede, or remove it when more than one choice is +reasonable. A previous attempt can contain valid unmerged history even when its +remote branch is gone. Before importing, record and compare the destination `HEAD` and `origin/main`; they must match. Recheck the first parent immediately after the subtree commit. @@ -61,6 +69,11 @@ an independently supported API. Record the evidence and confidence behind the ownership decision; stop and ask if current support expectations remain unclear. +Run registry-only `npm view` commands from a neutral temporary directory. A +repository's `devEngines` policy can reject the host Node version before npm +contacts the registry, which is unrelated to the package metadata audit. Record +that failure separately if using a neutral directory does not resolve it. + ## Produce these work products in order 1. A green Ghost import PR with reachable source history. @@ -122,6 +135,14 @@ path, package lint and tests pass through Nx, relevant consumer tests pass, the full build passes, and the Ghost archive contains the internal package. Record the exact commit IDs and commands in the handoff. +For a pilot or first use, include a structured gap report in the handoff: + +- `Observed`: the exact failure or ambiguity and the command/state that exposed it; +- `Worked around`: the safe action taken, without hiding the original gap; +- `Skill change`: the concrete instruction, preflight or script improvement; +- `Tooling change`: anything that cannot be solved within this repository; +- `Confidence`: high, medium or low, with unresolved evidence called out. + ## 2. Merge the import without rewriting history This is the exceptional PR. It must use GitHub's **Create a merge commit** diff --git a/.agents/skills/migrate-internal-package/references/history-and-merge.md b/.agents/skills/migrate-internal-package/references/history-and-merge.md index 58dfd3e6fed..0e836b3e362 100644 --- a/.agents/skills/migrate-internal-package/references/history-and-merge.md +++ b/.agents/skills/migrate-internal-package/references/history-and-merge.md @@ -4,9 +4,22 @@ Read this reference before creating the Ghost import PR. ## Create package-only source history -Work from an up-to-date, clean clone of the source repository. Use its actual -default branch and a temporary branch name that cannot be confused with a -product branch. +Work from an up-to-date, clean, full clone of the source repository. Shallow and +partial clones can complete `git subtree split` while still failing later when +Ghost fetches the split, because the local source cannot serve promised objects. +Reject them before splitting: + +```bash +set -euo pipefail + +test "$(git rev-parse --is-shallow-repository)" = "false" +test -z "$(git config --local --get extensions.partialClone || true)" +``` + +If either check fails, create a fresh full clone without `--depth`, +`--filter` or sparse/partial clone options. Use the source's actual default +branch and a temporary branch name that cannot be confused with a product +branch. `git subtree split` may inspect thousands of commits and run for several minutes. Use `--quiet` in agent or CI-style runners so its progress stream does @@ -41,14 +54,26 @@ excluding unrelated source-repository paths. ## Attach the history to Ghost -Create or enter a dedicated Ghost worktree, then verify its branch, cleanliness, -base and empty destination before attaching history: +Before creating the worktree, inspect collisions rather than discovering them +halfway through the import: + +```bash +git fetch --prune origin +git worktree list --porcelain +git branch --list 'codex/import-*' +git branch --remotes --list 'origin/codex/import-*' +``` + +If a match exists, record its worktree, cleanliness, base/divergence, imported +split and remote state. Do not delete or overwrite it without an explicit user +decision. Otherwise create or enter a dedicated Ghost worktree, then verify its +branch, cleanliness, base and empty destination before attaching history: ```bash set -euo pipefail test "$(git rev-parse --git-dir)" != "$(git rev-parse --git-common-dir)" -test "$(git branch --show-current)" = "codex/import-" +test "$(git branch --show-current)" = "codex/import--from-" test -z "$(git status --porcelain)" test "$(git rev-parse HEAD)" = "$(git rev-parse origin/main)" test ! -e "packages/" @@ -81,21 +106,34 @@ source_split_tip="" subtree_commit=$(git rev-parse HEAD) ghost_parent=$(git rev-parse HEAD^1) imported_parent=$(git rev-parse HEAD^2) +source_path="path/to/representative-file" +destination_path="packages//$source_path" test "$ghost_parent" = "$(git rev-parse origin/main)" test "$imported_parent" = "$source_split_tip" git merge-base --is-ancestor "$source_split_tip" HEAD +source_history=$(git log --full-history --format=%H "$source_split_tip" -- "$source_path") +destination_history=$(git log --full-history --format=%H -- "$destination_path") +test -n "$source_history" +test -n "$destination_history" +test "$(git rev-parse "$source_split_tip:$source_path")" = \ + "$(git rev-parse "$subtree_commit:$destination_path")" + git show --no-patch --format='%H%nparents: %P%n%B' "$subtree_commit" git log --graph --oneline --decorate --all --max-count=40 -git log --oneline -- packages//path/to/representative-file -git log --oneline "$source_split_tip" -- path/to/representative-file +git log --full-history --oneline -- "$destination_path" +git log --full-history --oneline "$source_split_tip" -- "$source_path" ``` The subtree commit must have two parents: its first parent must equal the recorded Ghost base and its second parent must equal the recorded split tip. -Checking the prefixed destination path and unprefixed split path separately is -more reliable around merge boundaries than relying only on `--follow`. +Checking the prefixed destination path with `--full-history` and the unprefixed +split path separately is more reliable around merge boundaries than relying on +ordinary path history or `--follow`, either of which may show only the subtree +merge. Requiring non-empty histories and equal blob IDs makes a missing or +mismatched representative file stop the import before integration edits obscure +the source state. The Admin API schema migration from TryGhost/SDK is a known-good example: From 4a1e047f194130cf70915c117d9d6ee9d1be1d08 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sun, 16 Aug 2026 16:49:37 +0100 Subject: [PATCH 4/5] Moved E2E guidance into codebase docs (#29997) Moves browser E2E writing guidance out of a tool-specific directory and into the canonical codebase documentation. This is part of work to bring codebase docs into the codebase, and make sure the information is consistent, correct and coherent. --- apps/ember-admin/README.md | 46 ++-- docs/README.md | 1 + .../contributing/e2e-testing.md | 97 +++++---- docs/contributing/testing.md | 14 +- e2e/AGENTS.md | 198 ++++-------------- e2e/README.md | 101 +-------- 6 files changed, 120 insertions(+), 337 deletions(-) rename e2e/.claude/E2E_TEST_WRITING_GUIDE.md => docs/contributing/e2e-testing.md (70%) diff --git a/apps/ember-admin/README.md b/apps/ember-admin/README.md index c4e415164c2..3d6102fb0af 100644 --- a/apps/ember-admin/README.md +++ b/apps/ember-admin/README.md @@ -6,53 +6,31 @@ This is the home of the Ember.js-based Admin app that ships with [Ghost](https:/ ### Running tests in the browser -Run all tests in the browser by running `pnpm dev` in the Ghost monorepo and visiting http://localhost:4200/tests. The code is hotloaded on change and you can filter which tests to run. - -[Testing public documentation](https://ghost.notion.site/Testing-Ember-560cec6700fc4d37a58b3ba9febb4b4b) - ---- +Run `pnpm dev` from the repository root, then visit +[http://localhost:4200/tests](http://localhost:4200/tests). The code reloads on +change and the browser runner can filter the tests. Tip: You can use `this.timeout(0); await this.pauseTest();` in your tests to temporarily pause the execution of browser tests. Use the browser console to inspect and debug the DOM, then resume tests by running `resumeTest()` directly in the browser console ([docs](https://guides.emberjs.com/v3.28.0/testing/testing-application/#toc_debugging-your-tests)) - ### Running tests in the CLI -To build and run tests in the CLI, you can use: - -```bash -TZ=UTC pnpm test -``` -_Note the `TZ=UTC` environment variable which is currently required to get tests working if your system timezone doesn't match UTC._ - ---- - -However, this is very slow when writing tests, as it requires the app to be rebuilt on every change. Instead, create a separate watching build with: +Run Ember Admin tests through Nx from the repository root so the required +dependencies are built first: ```bash -pnpm build --environment=test -w -o="dist-test" +pnpm nx run ghost-admin:test ``` -Then run tests with: +To run one file, pass the required parallel value before the Ember Exam +arguments: ```bash -TZ=UTC pnpm test 1 --reporter dot --path="dist-test" +pnpm nx run ghost-admin:test -- 1 \ + --file-path=tests/acceptance/editor/publish-flow-test.js ``` -The `--reporter dot` shows a dot (`.`) for every successful test, and `F` for every failed test. It renders the output of the failed tests only. - ---- - -To run a specific test file: -```bash -TZ=UTC pnpm test 1 --reporter dot --path="dist-test" -mp=tests/unit/helpers/gh-count-characters-test.js -``` - ---- - -To have a full list of the available options, run -```bash -ember exam --help -``` +For more detail, see the +[testing guide](../../docs/contributing/testing.md#run-ember-admin-tests). # Copyright & License diff --git a/docs/README.md b/docs/README.md index bab620993a5..e61aeb2796f 100644 --- a/docs/README.md +++ b/docs/README.md @@ -89,6 +89,7 @@ Practice and contributor guides explain how to make and verify changes: - [API design](practices/api-design.md) - [Database migrations](practices/database-migrations.md) +- [Browser E2E testing](contributing/e2e-testing.md) - [Email testing](contributing/testing-email.md) - [Error handling](practices/error-handling.md) - [Internationalization](practices/internationalization.md) diff --git a/e2e/.claude/E2E_TEST_WRITING_GUIDE.md b/docs/contributing/e2e-testing.md similarity index 70% rename from e2e/.claude/E2E_TEST_WRITING_GUIDE.md rename to docs/contributing/e2e-testing.md index 17db381114b..c1b16c453ab 100644 --- a/e2e/.claude/E2E_TEST_WRITING_GUIDE.md +++ b/docs/contributing/e2e-testing.md @@ -1,16 +1,19 @@ -# E2E Test Writing Guide +# Writing Browser E2E Tests -Worked examples for writing E2E tests in `/e2e/` with TypeScript and Playwright. +Ghost's browser end-to-end tests live in `/e2e/` and use TypeScript and +Playwright to verify complete journeys across Ghost Admin and the public site. +This guide explains how to write and structure them. -This guide covers *how to build the pieces* — page objects, common interaction -patterns, and selector discovery. It deliberately does not repeat what is -already documented elsewhere: +For related guidance: | For | Read | | --- | --- | -| Running tests, dev/build modes, debugging, folder layout, test isolation, fixtures | [README.md](../README.md) | -| Rules, locator priority, AAA structure, DO/DON'T, validation checklist | [AGENTS.md](../AGENTS.md) | -| Available factories and how to add one | [data-factory/README.md](../data-factory/README.md) | +| Choosing between unit, integration, acceptance, and E2E tests | [Testing Ghost](testing.md) | +| Running E2E tests, infrastructure modes, debugging, isolation, and fixtures | [E2E workspace README](../../e2e/README.md) | +| Available factories and how to add one | [Data factory README](../../e2e/data-factory/README.md) | + +Workspace command examples start from the repository root and change into +`e2e/`. ## Conventions @@ -22,8 +25,8 @@ already documented elsewhere: - Page objects: `-page.ts` — `login-page.ts`, `admin-page.ts` - Class names stay PascalCase: `login-page.ts` exports `class LoginPage` -**Import through the `@/` path aliases**, never relative paths. The aliases are -defined in `tsconfig.json`: +In tests, import shared helpers and page objects through the `@/` path aliases +defined in `e2e/tsconfig.json`: ```typescript import {expect, test} from '@/helpers/playwright'; @@ -32,6 +35,20 @@ import {createPostFactory} from '@/data-factory'; import {usePerTestIsolation} from '@/helpers/playwright/isolation'; ``` +Page-object modules use relative imports for nearby base classes and sibling +modules where appropriate. + +Use Arrange–Act–Assert as a readability heuristic: set up the scenario, +perform the behaviour under test, then verify the outcome. Make the phases +clear through structure and naming; add comments only when the boundary would +otherwise be unclear. Keep each test focused on one scenario and write its name +and flow so that someone without detailed knowledge of the implementation can +understand the behaviour being checked. + +Use factories for test data. Keep their defaults for data that does not affect +the scenario, and only override the values needed for the behaviour and +assertions in that test. + ## Page Object Pattern ### Core Principles @@ -67,7 +84,7 @@ export class FeaturePage extends AdminPage { // 2. Labels for form elements this.nameInput = page.getByLabel('Name'); - // 3. Text content (for unique text) + // 3. Text content when the text is unique this.statusMessage = page.getByText('Saved'); // 4. Stable test IDs when semantic locators are unavailable @@ -189,14 +206,8 @@ await page.keyboard.type('Hello World'); ## Ghost-Specific Patterns -### Common Selectors -- Navigation: `data-test-nav="[section]"` -- Buttons: `data-test-button="[action]"` -- Modals: `[role="dialog"]` -- Loading states: `.gh-loading-spinner` - -Ember Admin uses `data-test-*` attributes; the React Admin apps use -`data-testid`. Prefer a role or label over either where one exists. +Ember Admin commonly uses `data-test-*` attributes and the React Admin apps use +`data-testid`. Prefer a role, label, or unique visible text where one exists. ### Admin URLs - Editor: `/ghost/#/editor/post/[id]` @@ -204,38 +215,31 @@ Ember Admin uses `data-test-*` attributes; the React Admin apps use - Settings: `/ghost/#/settings` - Members: `/ghost/#/members` -## Using Playwright MCP for Page Object Discovery +## Discovering Locators + +When creating a Page Object for unfamiliar UI, preserve the test environment +after a run and inspect the rendered page with Playwright Inspector or browser +developer tools. -When creating new Page Objects or discovering selectors for unfamiliar UI: +### Preserve the test environment -### 1. Start Ghost with Preserved Environment ```bash +cd e2e + # Start Ghost and keep it running PRESERVE_ENV=true pnpm test # The test will output the Ghost instance URL (usually http://localhost:2369) ``` -### 2. Use Playwright MCP to Explore -```javascript -// Navigate to the Ghost instance -mcp__playwright__browser_navigate({url: "http://localhost:2369/ghost"}) - -// Capture the current DOM structure -mcp__playwright__browser_snapshot() +The test output provides the preserved Ghost instance URL, usually +`http://localhost:2369`. Open that URL, exercise the interaction, and inspect +the accessibility tree and relevant attributes. -// Interact with elements to discover selectors -mcp__playwright__browser_click({element: "Button description", ref: "selector-from-snapshot"}) - -// Take screenshots for reference -mcp__playwright__browser_take_screenshot({filename: "feature-state.png"}) -``` - -### 3. Extract Selectors for Page Objects -Based on your exploration, create the Page Object with discovered selectors: -- Note the element references from snapshots -- Identify the best selector strategy (role, label, text, testId) -- Test interactions before finalizing the Page Object +Choose the locator using the priority above and verify the interaction before +adding it to the Page Object. If the UI has no reliable semantic locator, add a +stable test ID to the product code rather than coupling the test to styling or +DOM position. ## Test Template @@ -258,3 +262,14 @@ test.describe('Ghost Admin - Feature', () => { }); }); ``` + +After changing E2E tests, run the focused test as well as the workspace lint +and type checks: + +```bash +cd e2e + +pnpm test tests/admin/signin.test.ts +pnpm lint +pnpm test:types +``` diff --git a/docs/contributing/testing.md b/docs/contributing/testing.md index cd721a352ef..39a489b58c6 100644 --- a/docs/contributing/testing.md +++ b/docs/contributing/testing.md @@ -35,7 +35,8 @@ Put tests as close as possible to the code and behavior under test: framework and command vary by app, so use that workspace's `test:acceptance` target. - **Browser E2E tests** use Playwright to cover complete journeys across Ghost - Admin and the public site. They live in `e2e/`. + Admin and the public site. They live in `e2e/`. See + [Writing Browser E2E Tests](e2e-testing.md) for conventions and examples. - **Ember Admin tests** cover the legacy Ember application in `apps/ember-admin/` and run through Ember Exam via Nx. @@ -112,16 +113,19 @@ pnpm dev pnpm test:e2e ``` -Run a specific file or match a test title by passing Playwright arguments: +Run a specific file or match a test title by passing Playwright arguments. For +example, the first command runs the existing Admin sign-in test: ```bash -pnpm test:e2e tests/admin/posts.spec.ts +pnpm test:e2e tests/admin/signin.test.ts pnpm test:e2e --grep "publish a post" ``` Use `pnpm test:e2e:debug` for Ghost E2E debug logs. See the -[browser E2E guide](../../e2e/README.md) for infrastructure modes, test -isolation, fixtures, selectors, and debugging. +[E2E workspace README](../../e2e/README.md) for infrastructure modes, test +isolation, fixtures, and debugging, and +[Writing Browser E2E Tests](e2e-testing.md) for test conventions, selectors, +and Page Objects. ## Run Ember Admin Tests diff --git a/e2e/AGENTS.md b/e2e/AGENTS.md index 346dffa6f50..93e5730f8e1 100644 --- a/e2e/AGENTS.md +++ b/e2e/AGENTS.md @@ -1,165 +1,37 @@ # AGENTS.md -E2E testing guidance for AI assistants (Claude, Codex, etc.) working with Ghost tests. - -**IMPORTANT**: `README.md` is the canonical human documentation for E2E testing. -When creating or modifying E2E tests, follow it first. Use -`./.claude/E2E_TEST_WRITING_GUIDE.md` for additional agent-oriented examples. - -## Critical Rules -1. **Always use pnpm**, never npm -2. **Always run after changes**: `pnpm lint` and `pnpm test:types` -3. **Prefer semantic locators**, then stable test IDs -4. **Keep reusable UI structure and interactions in Page Objects** -5. **Avoid selectors coupled to styling or DOM position** -6. **Prefer clear names and structure over explanatory comments**; add a - comment when an AAA boundary would otherwise be unclear - -## Running E2E Tests - -For normal development, start `pnpm dev` before running E2E tests. The runner -auto-detects whether the Admin dev server is reachable at -`http://127.0.0.1:5174`: when it is, tests use **dev mode**, which is the fastest -feedback loop and does not require a prebuilt Ghost E2E image. - -The suite also supports **build mode** for local CI-like testing without dev -servers. Build mode requires a prepared `ghost-e2e:local` image; follow the -commands in the canonical README's [Build Mode](./README.md#build-mode-prebuilt-image) -section. If `Build image not found: ghost-e2e:local` appears unexpectedly, either -start `pnpm dev` to use dev mode or prepare the build-mode image. - -```bash -# Terminal 1 (or background): Start dev environment from the repo root -pnpm dev - -# Wait for the admin dev server to be reachable (http://127.0.0.1:5174) - -# Terminal 2: Run e2e tests from the e2e/ directory -pnpm test # Run all tests -pnpm test tests/path/to/test.ts # Run specific test -pnpm lint # Required after writing tests -pnpm test:types # Check TypeScript errors -pnpm build # Required after factory changes -pnpm test --debug # See browser during execution, for debugging -PRESERVE_ENV=true pnpm test # Debug failed tests (keeps containers) -``` -## Test Structure - -### Naming Conventions -- **Test suites**: `Ghost Admin - Feature` or `Ghost Public - Feature` -- **Test names**: `what is tested - expected outcome` (lowercase) -- **One test = one scenario** (never mix multiple scenarios) - -### AAA Pattern -```typescript -test('action performed - expected result', async ({page}) => { - const analyticsPage = new AnalyticsGrowthPage(page); - const postFactory = createPostFactory(page.request); - const post = await postFactory.create({status: 'published'}); - - await analyticsPage.goto(); - await analyticsPage.topContent.postsButton.click(); - - await expect(analyticsPage.topContent.contentCard).toContainText('No conversions'); -}); -``` - -## Page Objects - -### Structure -```typescript -export class AnalyticsPage extends AdminPage { - // Public readonly locators only - public readonly saveButton = this.page.getByRole('button', {name: 'Save'}); - public readonly emailInput = this.page.getByLabel('Email'); - - // Semantic action methods - async saveSettings() { - await this.saveButton.click(); - } -} -``` - -### Rules -- Put reusable page and major-component behavior in `helpers/pages/` -- Direct semantic locators are acceptable for small, one-off test interactions or assertions -- Expose locators as `public readonly` when used with assertions -- Methods use semantic names (`login()` not `clickLoginButton()`) -- Use `waitFor()` for guards, never `expect()` in page objects -- Keep all assertions in test files - -## Locators (Strict Priority) - -1. **Semantic** (always prefer): - - `getByRole('button', {name: 'Save'})` - - `getByLabel('Email')` - - `getByText('Success')` - -2. **Test IDs** (when semantic unavailable): - - `getByTestId('analytics-card')` - - Suggest adding `data-testid` to Ghost codebase when needed - -3. **Structural fallback**: stable attributes when semantic locators are unavailable - -Avoid XPath, `nth-child`, styling classes, and other selectors coupled to DOM -position or presentation. Keep necessary structural selectors in Page Objects where -practical. - -### Playwright MCP Usage -- Use `mcp__playwright__browser_snapshot` to find elements -- Use `mcp__playwright__browser_click` with semantic descriptions -- If no good locator exists, suggest `data-testid` addition to Ghost - -## Test Data - -### Factory Pattern (Required) -```typescript -import {createPostFactory} from '@/data-factory'; - -const postFactory = createPostFactory(page.request); -const post = await postFactory.create({title: 'Test Post'}); -``` - -Import through the `@/` path aliases in `tsconfig.json` (`@/data-factory`, -`@/helpers/playwright`, `@/admin-pages`), never relative paths. - -## Best Practices - -### DO ✅ -- Use `usePerTestIsolation()` from `@/helpers/playwright/isolation` if a file needs per-test isolation -- Treat `config` and `labs` as environment-identity inputs: changing them should be an intentional part of test setup -- Use `resetEnvironment()` only in `beforeEach` hooks when you need a forced recycle inside per-file mode -- Keep `stripeEnabled` tests in per-test mode; the fixture forces this automatically -- Use factories for all test data -- Use Playwright's auto-waiting -- Run tests multiple times to ensure stability -- Use `test.only()` for debugging single tests - -### DON'T ❌ -- Use `test.describe.parallel(...)` or `test.describe.serial(...)` in e2e tests -- Use nested `test.describe.configure({mode: ...})` (mode toggles are root-level only) -- Call `resetEnvironment()` after resolving `baseURL`, `page`, `pageWithAuthenticatedUser`, or `ghostAccountOwner` -- Hard-coded waits (`waitForTimeout`) -- networkidle in waits (`networkidle`) -- Test dependencies (Test B needs Test A) -- Direct database manipulation -- Multiple scenarios in one test -- Assertions in page objects -- Manual login (auto-authenticated via fixture) - -## Project Structure -- `tests/admin/` - Admin area tests -- `tests/public/` - Public site tests -- `helpers/pages/` - Page objects -- `helpers/environment/` - Container management -- `data-factory/` - Test data factories - -## Validation Checklist -After writing tests, verify: -1. Test passes: `pnpm test path/to/test.ts` -2. Linting passes: `pnpm lint` -3. Types check: `pnpm test:types` -4. Follows AAA pattern with clear sections -5. Uses Page Objects for reusable UI behavior -6. Prefers semantic locators, then stable test IDs -7. Has no hard-coded waits or selectors coupled to styling/DOM position +Read the canonical human documentation before changing this workspace: + +- [Writing Browser E2E Tests](../docs/contributing/e2e-testing.md) covers test + structure, Page Objects, locator priority, waiting, and validation. +- The [E2E workspace README](./README.md) covers infrastructure modes, + fixtures, isolation, commands, and troubleshooting. +- The [data factory README](./data-factory/README.md) covers test-data helpers. + +## Required workflow + +- Always use `pnpm`, never npm or Yarn. +- Import shared test helpers through the `@/` aliases documented in the writing + guide. +- After changing E2E tests, run the focused test, `pnpm lint`, and + `pnpm test:types` from this workspace. +- After changing the data factory, also run `pnpm build`. +- Update the canonical human guide when a shared E2E convention changes. Do not + create or rely on tool-specific copies of the guidance. + +## Playwright MCP + +When discovering selectors or building a Page Object, use Playwright MCP when +it is available: + +- Run a focused test with `PRESERVE_ENV=true` and use the instance URL printed + by the test runner. +- Navigate to that instance and take an accessibility snapshot before choosing + locators. +- Exercise the interaction to verify the locator and capture a screenshot when + the rendered state is useful context. +- Follow the locator priority in the E2E writing guide; do not copy generated + selectors without checking that they are stable. + +If Playwright MCP is unavailable, use Playwright Inspector or browser developer +tools as described in the writing guide. diff --git a/e2e/README.md b/e2e/README.md index a4a7623553e..aa66185f5aa 100644 --- a/e2e/README.md +++ b/e2e/README.md @@ -87,8 +87,8 @@ pnpm --filter @tryghost/e2e preflight:build ### Running Specific Tests ```bash -# Specific test file -pnpm test specific/folder/testfile.spec.ts +# Run the Admin sign-in test +pnpm test tests/admin/signin.test.ts # Matching a pattern pnpm test --grep "homepage" @@ -99,6 +99,11 @@ pnpm test --debug ## Tests Development +See [Writing Browser E2E Tests](../docs/contributing/e2e-testing.md) for the +canonical conventions, Page Object pattern, locator priority, waiting patterns, +and worked examples. This README covers the local workspace, infrastructure, +fixtures, and commands. + The test suite is organized into separate directories for different areas/functions: ### **Current Test Suites** @@ -108,16 +113,6 @@ The test suite is organized into separate directories for different areas/functi We can decide whether to add additional sub-folders as we add more tests. -Filenames are kebab-case — `eslint.config.js` enforces this. Test files end in -`.test.ts` and are named after the behaviour under test, not the page: - -```text -tests/admin/ -├── signin.test.ts -├── two-factor-auth.test.ts -└── whats-new.test.ts -``` - Project folder structure can be seen below: ```text @@ -148,88 +143,6 @@ e2e/ └── tsconfig.json # TypeScript configuration and path aliases ``` -### Writing Tests - -Tests use [Playwright Test](https://playwright.dev/docs/writing-tests). Use -Arrange–Act–Assert (AAA) as a readability heuristic: set up the scenario, perform -the behavior under test, then verify the outcome. Keep those phases clear through -test structure and naming; comments are only useful when the boundaries would -otherwise be unclear. - -Import through the `@/` path aliases defined in `tsconfig.json`, not relative paths: - -```typescript -import {expect, test} from '@/helpers/playwright'; -import {HomePage} from '@/public-pages'; - -test.describe('Ghost Homepage', () => { - test('loads correctly', async ({page}) => { - const homePage = new HomePage(page); - - await homePage.goto(); - - await expect(homePage.title).toBeVisible(); - }); -}); -``` - -### Using Page Objects - -Page Objects are the default home for reusable knowledge about a page or major UI -component. They encapsulate locators, readiness guards, and semantic interactions -so tests can describe behavior rather than DOM structure. Assertions stay in test -files. - -Prefer an existing Page Object when a test exercises reusable UI behavior. A direct -semantic locator in a test is acceptable for a small, one-off assertion or -interaction when creating a Page Object would add indirection without reuse. -Structural selectors sometimes remain necessary for iframes, editor internals, -generated theme markup, and elements without an accessible role. Keep those inside -Page Objects where practical and prefer, in order: - -1. Accessible roles, labels, and visible text -2. Stable test IDs -3. Stable structural selectors when no semantic locator exists - -Avoid selectors coupled to visual styling, DOM position, or incidental class names. -See [Playwright's locator guidance](https://playwright.dev/docs/locators) and -[Martin Fowler's Page Object description](https://martinfowler.com/bliki/PageObject.html) -for background. - -Admin page objects extend `AdminPage`, public and portal ones extend `BasePage`. -The base class supplies `goto()`, `refresh()` and `pressKey()`, so a subclass -only sets its own `pageUrl` and locators: - -```typescript -// helpers/pages/admin/login-page.ts -import {AdminPage} from './admin-page'; -import type {Locator, Page} from '@playwright/test'; - -export class LoginPage extends AdminPage { - readonly emailAddressField: Locator; - readonly passwordField: Locator; - readonly signInButton: Locator; - - constructor(page: Page) { - super(page); - this.pageUrl = '/ghost/#/signin'; - - this.emailAddressField = page.getByRole('textbox', {name: 'Email address'}); - this.passwordField = page.getByRole('textbox', {name: 'Password'}); - this.signInButton = page.getByRole('button', {name: 'Sign in →'}); - } - - async signIn(email: string, password: string) { - await this.emailAddressField.fill(email); - await this.passwordField.fill(password); - await this.signInButton.click(); - } -} -``` - -For worked examples of modals, iframes, and discovering selectors for new page -objects, see the [test writing guide](./.claude/E2E_TEST_WRITING_GUIDE.md). - ### Global Setup and Teardown Tests use [Project Dependencies](https://playwright.dev/docs/test-global-setup-teardown#option-1-project-dependencies) to define special tests as global setup and teardown tests: From 95117e10b5ed0d04c72008b931906b6f63aa4c38 Mon Sep 17 00:00:00 2001 From: Hannah Wolfe Date: Sun, 16 Aug 2026 16:50:54 +0100 Subject: [PATCH 5/5] Updated commit message guidance (#29995) Ensures our commit message guidance is coherent and correct in line with how commits and PRs are done in 2026. Keeps the long-standing commit guidelines canonical in `.github/CONTRIBUTING.md`, with the newer workflow guide and agent guidance linking back to them. --- .agents/skills/commit/SKILL.md | 40 ++------------------- .github/CONTRIBUTING.md | 63 ++++++++++++++++++++++++---------- .github/hooks/commit-msg.bash | 23 +++++++------ AGENTS.md | 6 +++- docs/contributing/workflow.md | 29 +--------------- scripts/lib/release-notes.js | 2 +- 6 files changed, 65 insertions(+), 98 deletions(-) diff --git a/.agents/skills/commit/SKILL.md b/.agents/skills/commit/SKILL.md index 23dbc9b4730..0825b057c45 100644 --- a/.agents/skills/commit/SKILL.md +++ b/.agents/skills/commit/SKILL.md @@ -14,7 +14,8 @@ Use this skill whenever the user asks you to create a git commit for the current - `git diff` - `git log -5 --oneline` 2. Only stage files relevant to the requested change. Do not include unrelated untracked files, generated files, or likely-local artifacts. -3. Always follow Ghost's commit conventions (see below) for commit messages +3. Read and follow `.github/CONTRIBUTING.md#commit-messages`. It is the + source of truth for Ghost's commit conventions. 4. Run `git status --short` after committing and confirm the result. ## Important @@ -22,40 +23,3 @@ Use this skill whenever the user asks you to create a git commit for the current - Keep commits focused and avoid bundling unrelated changes - If there are no relevant changes, do not create an empty commit - If hooks fail, fix the issue and create a new commit. Never bypass hooks. - -## Commit message format - -We have a handful of simple standards for commit messages which help us to generate readable changelogs. Please follow this wherever possible and mention the associated issue number. - -- **1st line:** Max 80 character summary - - Written in past tense e.g. “Fixed the thing” not “Fixes the thing” - - Start with one of: Fixed, Changed, Updated, Improved, Added, Removed, Reverted, Moved, Released, Bumped, Cleaned -- **2nd line:** [Always blank] -- **3rd line:** `ref `, `fixes `, `closes ` or blank -- **4th line:** Why this change was made - the code includes the what, the commit message should describe the context of why - why this, why now, why not something else? - -If your change is **user-facing** please prepend the first line of your commit with **an emoji**. - -Because emoji commits are the release notes, it's important that anything that gets an emoji is a user-facing change that's significant and relevant for end-users to see. - -The first line of an emoji commit message should be from the perspective of the user. For example, 🐛 Fixed a race condition in the members service is technical and tells the user nothing, but 🐛 Fixed a bug causing active members to lose access to paid content tells the user reading the release notes “oh yeah, they fixed that bug I kept hitting.” - -### Main emojis we are using: - -- ✨ Feature -- 🎨 Improvement / change -- 🐛 Bug Fix -- 🌐 i18n (translation) submissions -- 💡 Anything else flagged to users or whoever is writing release notes - -### Example - -``` -✨ Added config flag for disabling page analytics - -ref https://linear.app/tryghost/issue/ENG-1234/ - -- analytics are brand new under development, therefore they need to be behind a flag -- not using the developerExperiments flag as that is already in wide use and we aren't ready to deploy this anywhere yet -- using the term `pageAnalytics` as this was discussed as best reflecting what this does -``` diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index c706d694bd0..f96ce800a6d 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -22,34 +22,59 @@ Discuss new features and substantial product or architectural changes in the ## Commit Messages -We have a handful of simple standards for commit messages which help us to generate readable changelogs. Please follow this wherever possible and mention the associated issue number. +We have a handful of simple standards for commit messages which keep the main +branch readable and generate useful release notes. They matter most for pull +request titles and squash commits; follow them for intermediate commits where +practical. -- **1st line:** Max 80 character summary - - Written in past tense e.g. “Fixed the thing” not “Fixes the thing” - - Start with one of: Fixed, Changed, Updated, Improved, Added, Removed, Reverted, Moved, Released, Bumped, Cleaned -- **2nd line:** [Always blank] -- **3rd line:** `ref `, `fixes `, `closes ` or blank -- **4th line:** Why this change was made - the code includes the what, the commit message should describe the context of why - why this, why now, why not something else? +```text + -If your change is **user-facing** please prepend the first line of your commit with **an emoji key**. If the commit is for an alpha feature, no emoji is needed. We are following [gitmoji](https://gitmoji.carloscuesta.me/). + -**Main emojis we are using:** + +``` -- ✨ Feature -- 🎨 Improvement / change -- 🐛 Bug Fix -- 🌐 i18n (translation) submissions [[See Translating Ghost docs for more detail](../docs/contributing/translating-ghost.md)] -- 💡 Anything else flagged to users or whoever is writing release notes +- Start the summary with `Fixed`, `Changed`, `Updated`, `Improved`, `Added`, + `Removed`, `Reverted`, `Moved`, `Released`, `Bumped`, or `Cleaned`. +- Keep the second line blank. +- When an issue exists, use a supported relationship followed by its URL, such + as `ref `, `fixes `, or `closes `. Use + `no ref` when it is useful to state explicitly that there is no issue, or + leave this line blank. +- Explain the context in the body: why this change, why now, and why this + approach. The diff already describes what changed. -Good commit message examples: [new feature](https://github.com/TryGhost/Ghost/commit/61db6defde3b10a4022c86efac29cf15ae60983f), [bug fix](https://github.com/TryGhost/Ghost/commit/6ef835bb5879421ae9133541ebf8c4e560a4a90e) and [translation](https://github.com/TryGhost/Ghost/commit/83904c1611ae7ab3257b3b7d55f03e50cead62d7). +The local hook warns about most deviations without blocking the commit. It +does require the common invalid forms `refs ...` and `ref: ...` to be corrected +to a supported relationship such as `ref ...`. -**Bumping @tryghost dependencies** +### Release-note emojis + +A leading release-note emoji opts the squash commit into generated release +notes. Add one only for a significant change that is relevant to users, and +write the summary from their perspective. Alpha or experimental work does not +need an emoji until it becomes user-facing. -When bumping `@tryghost/*` dependencies, the first line should follow the above format and say what has changed, not say what has been bumped. +- ✨ Feature +- 🎨 Improvement or change +- 🐛 Bug fix +- 💡 Other noteworthy user-facing change + +Use 🌐 for [translation submissions](../docs/contributing/translating-ghost.md). +Translation commits are not selected for generated release notes by that emoji +alone. -There is no need to include what modules have changed in the commit message, as this is _very_ clear from the contents of the commit. The commit should focus on surfacing the underlying changes from the dependencies - what actually changed as a result of this dependency bump? +Good final commit examples include a [new feature](https://github.com/TryGhost/Ghost/commit/61db6defde3b10a4022c86efac29cf15ae60983f), +a [bug fix](https://github.com/TryGhost/Ghost/commit/6ef835bb5879421ae9133541ebf8c4e560a4a90e), +and a [translation](https://github.com/TryGhost/Ghost/commit/83904c1611ae7ab3257b3b7d55f03e50cead62d7). + +**Bumping @tryghost dependencies** -[Good example](https://github.com/TryGhost/Ghost/commit/95751a0e5fb719bb5bca74cb97fb5f29b225094f) +When bumping `@tryghost/*` dependencies, describe the user-visible result rather +than which packages were bumped. The diff already shows the package changes; +the message should explain what changed because of them. See this +[good example](https://github.com/TryGhost/Ghost/commit/95751a0e5fb719bb5bca74cb97fb5f29b225094f). ## Changesets diff --git a/.github/hooks/commit-msg.bash b/.github/hooks/commit-msg.bash index 332e66d2219..89feb5510c8 100755 --- a/.github/hooks/commit-msg.bash +++ b/.github/hooks/commit-msg.bash @@ -79,17 +79,18 @@ if [ -z "$body" ]; then echo -e "The body should explain: why this, why now, why not something else?" fi -# Check for emoji in user-facing changes -if [[ "$subject" =~ ^[^[:space:]]*[[:space:]] ]]; then - first_word="${subject%% *}" - # Emoji are multi-byte (non-ASCII), so detect them by stripping every ASCII - # byte and seeing if anything is left. The previous check tested the first word - # against [[:punct:]], which never matches an emoji — so the warning fired on - # *every* correctly-prefixed commit (🐛, ✨, …) and passed on plain ASCII. - if [[ -z "$(printf '%s' "$first_word" | LC_ALL=C tr -d '\000-\177')" ]]; then - echo -e "${yellow}Warning: User-facing changes should start with an emoji${no_color}" - echo -e "Common emojis: ✨ (Feature), 🎨 (Improvement), 🐛 (Bug Fix), 🌐 (i18n), 💡 (User-facing)" - fi +# Give concise release-note guidance. Whether a change is user-facing requires +# human judgment, so these notices are informative rather than enforcement. +first_word="${subject%% *}" +contributor_emojis="✨ 🎨 🐛 🌐 💡" + +if [[ "$first_word" =~ ^(✨|🎨|🐛|🌐|💡|🔒)$ ]]; then + echo -e "${yellow}Notice: Keep the emoji only for a significant user-facing PR title or squash commit.${no_color}" +elif [[ -n "$(printf '%s' "$first_word" | LC_ALL=C tr -d '\000-\177')" ]]; then + echo -e "${yellow}Warning: Unsupported leading emoji. Use one of: ${contributor_emojis}${no_color}" + echo -e "Emoji selection only matters for user-facing PR titles and squash commits." +else + echo -e "${yellow}Notice: If this is a significant user-facing PR title or squash commit, add one of: ${contributor_emojis}${no_color}" fi # Check for past tense verbs in subject diff --git a/AGENTS.md b/AGENTS.md index 103b0e036e9..b91f2682d03 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -145,7 +145,11 @@ skill without duplicating it. Run `pnpm lint:agent-skills` to verify every repository skill is linked correctly; CI runs the same check. ### Commit Messages -When the user asks you to create a commit or draft a commit message, load and follow the `commit` skill from `.agents/skills/commit`. +When the user asks you to create a commit or draft a commit message, load and +follow the `commit` skill from `.agents/skills/commit`. Read the canonical +[commit message guidelines](.github/CONTRIBUTING.md#commit-messages), and apply +the subject convention carefully to PR titles and proposed squash commits. +Local hook notices on intermediate commits are best-effort guidance. ### ESLint Config Source of truth: two internal config packages — [`@internal/cfg-eslint`](configs/eslint/index.mjs) (shared rule atoms + the `nodeLibConfig` factory for Node libs) and [`@internal/cfg-eslint-react`](configs/eslint-react/index.mjs) (the `reactAppConfig` factory for every `apps/*` workspace). Both factories are synchronous and have full JSDoc with `@example`s; hover the call site in your editor. Consume them by name — declare the package as a `workspace:*` devDependency. diff --git a/docs/contributing/workflow.md b/docs/contributing/workflow.md index d874a6eaa8f..14f2397bd57 100644 --- a/docs/contributing/workflow.md +++ b/docs/contributing/workflow.md @@ -76,34 +76,7 @@ not affect a publishable package do not need one. ## Commit Messages -We have a handful of simple standards for commit messages which help us to generate readable changelogs. Please follow this wherever possible and mention the associated issue number. - -- **1st line:** Max 80 character summary - - Written in past tense e.g. “Fixed the thing” not “Fixes the thing” - - Start with one of: Fixed, Changed, Updated, Improved, Added, Removed, Reverted, Moved, Released, Bumped, Cleaned -- **2nd line:** [Always blank] -- **3rd line:** `ref `, `fixes `, `closes ` or blank -- **4th line:** Why this change was made - the code includes the what, the commit message should describe the context of why - why this, why now, why not something else? - -If your change is **user-facing** please prepend the first line of your commit with **an emoji key**. If the commit is for an alpha feature, no emoji is needed. We are following [gitmoji](https://gitmoji.carloscuesta.me/). - -**Main emojis we are using:** - -- ✨ Feature -- 🎨 Improvement / change -- 🐛 Bug Fix -- 🌐 i18n (translation) submissions [[See Translating Ghost docs for more detail](translating-ghost.md)] -- 💡 Anything else flagged to users or whoever is writing release notes - -Good commit message examples: [new feature](https://github.com/TryGhost/Ghost/commit/61db6defde3b10a4022c86efac29cf15ae60983f), [bug fix](https://github.com/TryGhost/Ghost/commit/6ef835bb5879421ae9133541ebf8c4e560a4a90e) and [translation](https://github.com/TryGhost/Ghost/commit/83904c1611ae7ab3257b3b7d55f03e50cead62d7). - -**Bumping @tryghost dependencies** - -When bumping `@tryghost/*` dependencies, the first line should follow the above format and say what has changed, not say what has been bumped. - -There is no need to include what modules have changed in the commit message, as this is _very_ clear from the contents of the commit. The commit should focus on surfacing the underlying changes from the dependencies - what actually changed as a result of this dependency bump? - -[Good example](https://github.com/TryGhost/Ghost/commit/95751a0e5fb719bb5bca74cb97fb5f29b225094f) +Follow the canonical [commit message guidelines](../../.github/CONTRIBUTING.md#commit-messages). ## Publish the branch diff --git a/scripts/lib/release-notes.js b/scripts/lib/release-notes.js index 16efd808d42..0f1977214c1 100644 --- a/scripts/lib/release-notes.js +++ b/scripts/lib/release-notes.js @@ -5,7 +5,7 @@ const ROOT = resolve(import.meta.dirname, '../..'); const REPO_URL = 'https://github.com/TryGhost/Ghost'; // Emoji priority order (lowest index = lowest priority, sorted descending) -const EMOJI_ORDER = ['💡', '🐛', '🎨', '💄', '✨', '🔒']; +const EMOJI_ORDER = ['💡', '🐛', '🎨', '✨', '🔒']; // User-facing emojis — only these are included in release notes const USER_FACING_EMOJIS = new Set(EMOJI_ORDER);