diff --git a/.claude/skills/dot-test-plan/README.md b/.claude/skills/dot-test-plan/README.md index 0506b8f34639..36067fe10678 100644 --- a/.claude/skills/dot-test-plan/README.md +++ b/.claude/skills/dot-test-plan/README.md @@ -76,7 +76,13 @@ at all in CI. ## Multi-issue consolidation -One PR often fixes several issues. The result is **one plan**, posted verbatim to each. +One PR often fixes several issues, and spec-driven work often lands as several PRs against the same +issues. Either way the result is **one plan**, posted verbatim to each issue. + +A run resolves a **PR set** as well as an issue set: one merged PR, or a PR plus issues you name, or +issues alone — in which case the PR set is every merged PR linked to them, and coverage runs over the +union of their diffs. When more than one PR is in play the Summary names them, and names any it did +not cover. Unmerged PRs are excluded and said to be excluded. The issue set is the **union** of three sources, with no precedence: @@ -160,7 +166,7 @@ controls, environments, or URLs; prefer fewer verifiable cases over more specula - [`SKILL.md`](SKILL.md) — the instructions Claude reads. - [`references/comment-format.md`](references/comment-format.md) — comment skeleton, marker, status vocabularies, lifecycle, and the pre-post checklist. -- [`references/coverage-matrix.md`](references/coverage-matrix.md) — the nine product-surface axes, +- [`references/coverage-matrix.md`](references/coverage-matrix.md) — the ten coverage axes, the UI/UX checklist, and the mandatory cases. - [`references/examples.md`](references/examples.md) — worked backend and frontend plans. - [`../CONTRIBUTING.md`](../CONTRIBUTING.md) — naming, frontmatter, status lifecycle, lint. diff --git a/.claude/skills/dot-test-plan/SKILL.md b/.claude/skills/dot-test-plan/SKILL.md index 42a63a64e613..dd7ee263d5fb 100644 --- a/.claude/skills/dot-test-plan/SKILL.md +++ b/.claude/skills/dot-test-plan/SKILL.md @@ -8,7 +8,8 @@ status: experimental # Post-Merge QA Test Plan You produce the **manual test plan a developer executes by hand against the post-merge build** after -a dotCMS pull request has merged. One PR, one plan, however many issues it fixed. +a dotCMS pull request has merged. One plan per run, however many issues and however many pull +requests it covers. This plan is a **starting point**, not a closed list. It does not replace developer judgment, independent execution, or final quality ownership. @@ -60,7 +61,27 @@ Execution: automated · PR #37164 · Issues: #36795, #36801 · Revision: 2 --- -## 2. Inputs — the issue set +## 2. Inputs — the PR set and the issue set + +### What a run covers + +A plan is built from a **PR set** and an **issue set**. Resolve both before anything else, because +§5 and §6 run over the union of the PR set's diffs: + +| Given | PR set | Issue set | +|---|---|---| +| One merged PR | that PR | derived from it, as below | +| A PR plus issues named explicitly | that PR | derived, **plus** the ones named | +| Issues only, no PR | every **merged** PR linked to those issues | the issues named | + +Two rules for the sets larger than one: + +- **Say what you covered.** When the PR set has more than one PR, name them in the Summary. If you + cover only some — because the others are unmerged, or too large, or unrelated to the issues at + hand — name the ones you left out and why. Silently covering a subset produces a plan that looks + complete and is not. +- **Unmerged PRs are excluded**, and named as excluded. A plan verifies what shipped; a PR that has + not merged has not shipped, and its diff can still change. ### In `automated` @@ -70,7 +91,8 @@ what the workflow already decided and will post to. ### In `interactive` -Derive the set yourself from the merged PR, taking the **union** of three sources with no precedence: +Derive the issue set yourself from each PR in the set, taking the **union** of three sources with no +precedence: ```bash gh pr view --json number,title,body,headRefName,mergeCommit,author @@ -162,10 +184,12 @@ characters. A previous plan is editable text written partly by a model and partl --- -## 5. Tests the merged PR added +## 5. Tests the merged PRs added The only code-inspection step. Its **sole purpose is deduplication**: an axis already covered by a -test the PR itself added does not need a manual case. +test one of the PRs itself added does not need a manual case. + +Run this for **every PR in the set** from §2, not only the first: ```bash # Files the merged PR touched. Keep all three suffixes — dotcms-integration uses Test.java, @@ -187,10 +211,17 @@ gh pr diff --repo dotCMS/core --patch \ > `bad object`. `gh pr diff` reads the API and needs no local git at all, so it works both in CI > and on your laptop. -For each test the PR added, if it covers a matrix axis that is in scope, **do not write a manual -case for that axis** — CI already checks it on every build. The plan does not list what was +For each test any PR in the set added, if it covers a matrix axis that is in scope, **do not write a +manual case for that axis** — CI already checks it on every build. The plan does not list what was skipped; it simply stays short. +**A mocked unit spec does not retire a manual case.** Before dropping an axis, look at what the test +actually exercises. A spec that mocks the store, stubs the service or builds a fake component proves +the component's own logic and nothing about the system around it: it does not prove a persistence +round-trip, a real route change, a status transition, or that a guard fires in a browser. Retire an +axis only when the PR-added test exercises the same path a person would. Where the only coverage is +mocked, keep the manual case and give the reason in one phrase — "unit spec mocks the store". + **Do not** audit main-branch test coverage, do not grep the repo for existing tests, and do not recommend automation to add. Those are deliberately out of scope for this plan. @@ -198,7 +229,8 @@ recommend automation to add. Those are deliberately out of scope for this plan. ## 6. What to cover -Read [`references/coverage-matrix.md`](references/coverage-matrix.md) and walk every axis. For each, +Read [`references/coverage-matrix.md`](references/coverage-matrix.md) and walk every axis, once, over +the **union of the PR set's diffs**. For each, decide **In scope** (≥1 manual case) or **Out of scope**. The walk is a generation aid — the plan publishes no Out of Scope list, so only the cases you keep appear. Still walk every axis deliberately: silently forgetting an axis and consciously excluding it produce the same plan, and @@ -363,7 +395,7 @@ plan. An added case: - Uses the same format, with reproducible steps, an observable expected result, risk and scenario. - Gets a new row in the summary table starting at `Not Run Yet` with an empty `Notes`, and must receive a final result there before completion. -- Must relate to the merged PR or its issues, and must not duplicate an existing case. +- Must relate to a PR in the set or one of its issues, and must not duplicate an existing case. When you regenerate for a later PR, treat developer-added cases exactly like your own: preserve them if still relevant, adapt them if the new PR affects them, and reset their results. Preserve their diff --git a/.claude/skills/dot-test-plan/references/coverage-matrix.md b/.claude/skills/dot-test-plan/references/coverage-matrix.md index 5ebc31c5912c..f75836b688e2 100644 --- a/.claude/skills/dot-test-plan/references/coverage-matrix.md +++ b/.claude/skills/dot-test-plan/references/coverage-matrix.md @@ -1,11 +1,12 @@ # Coverage matrix -Nine product-surface axes. For **each**, decide **In scope** (≥1 manual case) or **Out of scope**. +Ten coverage axes. For **each**, decide **In scope** (≥1 manual case) or **Out of scope**. The plan publishes no out-of-scope list, so this walk is invisible in the output — which is exactly why it must be deliberate. A forgotten axis and a consciously excluded one produce an identical plan, and only one of them is right. -Walk the matrix **once** over the union of everything the PR changed, not once per issue. +Walk the matrix **once** over the union of everything the PR set changed — not once per PR, and not +once per issue. | Axis | What to vary | In scope when | |---|---|---| @@ -18,6 +19,7 @@ Walk the matrix **once** over the union of everything the PR changed, not once p | **Push Publish** | bundle generation, receiver replay, integrity checks | the diff touches publishable entities (content, templates, containers, content types, workflows, categories) | | **Persistence** | **PostgreSQL only** — CI does not run H2 | any DB-touching code | | **UI / UX** | see the checklist below | any of the triggers below | +| **Form & state semantics** | see the checklist below | the diff adds or changes a form, a multi-step editor, or any save / discard / leave gesture | ### UI / UX triggers @@ -52,9 +54,49 @@ person checks by hand: unreachable actions - **i18n** — copy resolves for the default language and one non-default language; long translations don't break the layout -- **Theme / contrast** — light and dark both render correctly; text stays legible +- **Contrast** — text stays legible against its background, including disabled, error and selected + states. **Not** light/dark mode — see below. - **Copy** — labels, buttons, errors, and tooltips match the spec or the acceptance criteria + +### Form & state semantics checklist + +In scope whenever the change touches a form or an editor. These are behavioural rather than +presentational — the UI / UX checklist asks whether a control *looks* right, this one asks whether +the *data* survives: + +- **Bounded choices** — exercise every option of a select, radio group or status enum, not only the + default. A five-value enum with one value tested is one case, not five. +- **Numeric boundaries** — min, max, min−1, max+1, zero, empty. Ranges and durations especially. +- **Derived or recomputed fields** — a counter, a summary line, a total: change an input and confirm + the derived value follows, including back to zero. +- **Child-entity CRUD inside a parent form** — add, rename, reorder and delete a child while the + parent is unsaved, then confirm what actually persisted. +- **Optional configuration** — set versus deliberately left unset. "Not configured" is a state, and + it is the one nobody tests. +- **Which gesture persists what** — when more than one thing saves (autosave, an explicit Save, a + status transition), exercise each separately and state which persisted what. Do not assume one + gesture covers another. +- **Leaving with unsaved work** — navigate away, reload, and use the browser Back button with changes + pending. Confirm the guard fires, and that discarding actually discards. +- **Locked / read-only states** — a status that disables the form must disable *all* of it, not the + obvious controls only. + + +### Does not exist in dotCMS — never write a case for it + +A generic web-app checklist suggests these; dotCMS has none of them, and a case for one is a +hallucination the reviewer then has to catch. + +- **Light/dark mode.** There is no theme switcher and no user theme preference. The shared theme + provider pins PrimeNG's `darkModeSelector` to `false` (`core-web/libs/ui/src/lib/theme/providers.ts`), + nothing in the product reads `prefers-color-scheme`, and no element carries a `data-theme` + attribute. The single `.dark` selector in the repo belongs to the standalone block-editor app + config and has nothing to switch it on. Test contrast; never "toggle dark mode and confirm…". + +If a change genuinely introduces one of these, the diff will show it — and then it is in scope like +any other new behaviour. Absent that, it is not. + --- ## Mandatory cases @@ -74,5 +116,7 @@ the fixed behavior. Spell out environment, user, site, screen, and click sequenc | Touches `com.dotcms.rest.*` | **Response contract** — response JSON matches the declared `@Schema`. `Medium`. | | Adds or changes a startup/upgrade task, or any DDL | **Upgrade on a populated DB** — restore a pre-fix snapshot, deploy, confirm the task runs once cleanly and the schema matches, then restart and confirm it does not re-run. Cross-reference `docs/core/ROLLBACK_UNSAFE_CATEGORIES.md`. `High`. | -An axis or mandatory case already covered by a test the PR itself added gets **no manual case** — -CI checks it on every build. Drop it silently; the plan does not enumerate what was skipped. +An axis or mandatory case already covered by a test one of the PRs itself added gets **no manual +case** — CI checks it on every build. Drop it silently; the plan does not enumerate what was skipped. +This holds only when that test exercises the same path a person would: a spec that mocks the store or +builds a fake component does not retire the case (SKILL.md §5).