From a9fd7949e5a7c1e36025eac597c73360488180ad Mon Sep 17 00:00:00 2001 From: erickgonzalez Date: Mon, 7 Sep 2026 16:13:25 -0600 Subject: [PATCH 1/3] feat(test-plan): add a form-semantics axis, qualify dedup, define the PR set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three gaps found by review of a real run, kept separate from the two findings still under discussion. Form & state semantics — a tenth coverage axis. The existing nine are platform concerns and the UI/UX checklist is purely presentational: it asks whether a control looks right, never whether the data survives. Nothing prompted for every option of a bounded choice, numeric boundaries, derived fields, child CRUD inside a parent form, which gesture persists what, or leaving with unsaved work. On a form-heavy change the matrix simply never asked. A mocked unit spec no longer retires a manual case. The dedup rule dropped any axis a PR-added test covered, with no caveat. PR #37064 added 162 mockReturnValue calls, four of them mocking $hasUnsavedChanges, and zero e2e specs -- so the unsaved-changes guard was retired from the manual plan on the strength of a spec that builds a fake component. A mock proves component logic, not a persistence round-trip, a route change or a status transition. A run now resolves a PR set as well as an issue set. The skill was single-PR by construction: section 5 diffed one , section 2 resolved issues and never PRs. Spec-driven work lands as several PRs against the same issues, and the skill can be handed issues alone. Coverage now runs over the union of the set's diffs, the Summary names the PRs covered and any deliberately left out, and unmerged PRs are excluded and said to be excluded. Also regenerates CATALOG.md, which was already stale on main by one count line after speckit-docs-converge landed -- unrelated to this change, but skill-lint gates every PR touching .claude/skills/**. Co-Authored-By: Claude Opus 5 --- .claude/skills/CATALOG.md | 2 +- .claude/skills/dot-test-plan/README.md | 10 +++- .claude/skills/dot-test-plan/SKILL.md | 50 +++++++++++++++---- .../references/coverage-matrix.md | 25 ++++++++++ 4 files changed, 75 insertions(+), 12 deletions(-) diff --git a/.claude/skills/CATALOG.md b/.claude/skills/CATALOG.md index d52430e3ddd4..bf28890f755a 100644 --- a/.claude/skills/CATALOG.md +++ b/.claude/skills/CATALOG.md @@ -3,7 +3,7 @@ Auto-generated inventory of dotCMS skills. **Check here before creating a new skill** — if something close exists, extend it or mark yours `related`, don't fork. See [CONTRIBUTING.md](CONTRIBUTING.md). -_22 first-party · 6 external (symlinked)._ +_23 first-party · 6 external (symlinked)._ ## First-party skills (`dot-*`) 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..1400f85c3bcb 100644 --- a/.claude/skills/dot-test-plan/references/coverage-matrix.md +++ b/.claude/skills/dot-test-plan/references/coverage-matrix.md @@ -18,6 +18,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 @@ -55,6 +56,30 @@ person checks by hand: - **Theme / contrast** — light and dark both render correctly; text stays legible - **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. + --- ## Mandatory cases From 2488f01bbbeea05d1b460c6251830e46f2dae1b7 Mon Sep 17 00:00:00 2001 From: erickgonzalez Date: Tue, 8 Sep 2026 07:58:36 -0600 Subject: [PATCH 2/3] fix(test-plan): sweep coverage-matrix.md up to the contract it documents Three lines in the file this PR edits still described the state before the edit: - the header said "Nine product-surface axes" while the table below it had ten. This is an instruction file a model reads to decide which axes to walk, so an undercount risks the new axis being treated as optional -- exactly what adding it was meant to prevent - the walk instruction said "the union of everything the PR changed", singular, contradicting SKILL.md section 6's "union of the PR set's diffs" - the dedup rule said "a test the PR itself added", also singular, and restated the rule without the mocked-spec caveat added to section 5, so the two files disagreed on when an axis may be dropped The first two were raised in review; the third is the same drift one line further down and would have left the reference file contradicting SKILL.md on the substantive change in this PR. Verified consistent across all four files: ten axis rows, ten in the header, ten in the README, and both the matrix and section 6 walking the PR set. Co-Authored-By: Claude Opus 5 --- .../dot-test-plan/references/coverage-matrix.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/.claude/skills/dot-test-plan/references/coverage-matrix.md b/.claude/skills/dot-test-plan/references/coverage-matrix.md index 1400f85c3bcb..e5fda30d2c83 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 | |---|---|---| @@ -99,5 +100,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). From 5c6da1e46e63acb6cb93ecad3a460fb7ac3fe3d2 Mon Sep 17 00:00:00 2001 From: erickgonzalez Date: Tue, 8 Sep 2026 08:08:26 -0600 Subject: [PATCH 3/3] fix(test-plan): stop prompting for a light/dark mode dotCMS does not have Generated plans have been carrying a "verify light and dark mode" case for a feature that does not exist. The UI/UX checklist asked for it directly -- "Theme / contrast: light and dark both render correctly" -- so the case was prompted by the skill, not invented from thin air, and every one of them was a hallucination a reviewer had to spend attention rejecting. Verified before encoding the constraint: - core-web/libs/ui/src/lib/theme/providers.ts pins PrimeNG's darkModeSelector to `options?.darkModeSelector ?? false` - no file under core-web/libs or core-web/apps reads prefers-color-scheme - no element carries a data-theme attribute, and there is no theme switcher - the repository's only `.dark` selector is in the standalone block-editor app config, with nothing wired to activate it Split the checklist item: contrast is real and stays, extended to disabled, error and selected states. Light/dark is removed and named explicitly in a new "Does not exist in dotCMS" section, because deleting the prompt is not enough on its own -- a model generating a UI plan will reach for dark mode from general web knowledge unless told it is absent, and told why. The section is scoped so it cannot become stale in the wrong direction: if a change genuinely introduces one of these, the diff shows it and it is in scope like any other new behaviour. Co-Authored-By: Claude Opus 5 --- .../references/coverage-matrix.md | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/.claude/skills/dot-test-plan/references/coverage-matrix.md b/.claude/skills/dot-test-plan/references/coverage-matrix.md index e5fda30d2c83..f75836b688e2 100644 --- a/.claude/skills/dot-test-plan/references/coverage-matrix.md +++ b/.claude/skills/dot-test-plan/references/coverage-matrix.md @@ -54,7 +54,8 @@ 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 @@ -81,6 +82,21 @@ the *data* survives: - **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