Skip to content

Commit b085d3f

Browse files
Kseolisclaude
andcommitted
refactor(tests): clean-code rewrite — verb POM, support layer, comment-free
Apply GRASP/KISS/DRY/YAGNI to the suite while keeping the same scenarios: - Page objects expose verb-only actions over private locators; remove all noun-method locator builders. Shared low-level interaction lives in BasePage (fillReactively, toggleHiddenControl) — kills the 5x evaluate/dispatchEvent duplication. Names carry intent; zero comments in tests/** and fixtures. - New tests/support layer (Dependency Inversion): purchaseVpn flow over OfferLandingPage/PaymentStep interfaces dedupes the RU/EN twin; specs are declarative. Pure reachedPaymentProvider oracle replaces the fake PaymentRedirectPage (which masqueraded as a page) and rejects */failed pages. - acceptTerms scoped to the payment form's checkboxes via the shared helper (was a document-wide querySelectorAll sweep). - YAGNI: delete unused payment-draft factory, user-factory transients, dead LoginPage/SignupPage locators, BasePage.currentUrl. - Relocate load-bearing 'why' (PrimeVue events, hidden controls, oracle, known gaps) to docs/ARCHITECTURE.md §5. Wire @support alias + layer. Suite shrinks ~1073→877 LOC. Validated: typecheck, lint, validate, format green; RU-stripe + EN paths pass live; RU card_ru is the documented live-site flake (retries handle it in CI). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent e821966 commit b085d3f

23 files changed

Lines changed: 316 additions & 483 deletions

‎README.md‎

Lines changed: 27 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -86,14 +86,16 @@ What's intentionally **not** used:
8686

8787
## Architecture
8888

89-
The repo is built around a 5-layer convention pinned in [`tests-config.json`](tests-config.json) and mechanically enforced by validators under [`tools/`](tools/). Imports flow one direction only — reverse imports across layers are forbidden. A deeper walkthrough lives in [`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md).
89+
The repo is built around a 6-layer convention pinned in [`tests-config.json`](tests-config.json) and mechanically enforced by validators under [`tools/`](tools/). Imports flow one direction only — reverse imports across layers are forbidden. A deeper walkthrough lives in [`docs/ARCHITECTURE.md`](docs/ARCHITECTURE.md).
9090

9191
```mermaid
9292
flowchart TD
9393
specs["specs/<br/>the actual tests"] --> fixtures["fixtures/<br/>mergeTests composition"]
94-
fixtures --> pages["pages/<br/>page objects"]
94+
specs --> support["support/<br/>purchase flow + boundary oracle"]
95+
fixtures --> pages["pages/<br/>verb actions over private locators"]
9596
fixtures --> factories["factories/<br/>Fishery test data"]
9697
fixtures --> infra["infra/<br/>env loader + helpers"]
98+
pages --> support
9799
pages --> infra
98100
factories --> infra
99101
@@ -113,14 +115,15 @@ flowchart TD
113115

114116
### Why layers?
115117

116-
| Layer | Purpose | Boundary rule |
117-
| ------------ | --------------------------------------------- | ------------------------------------------------------- |
118-
| `infra/` | env loader, low-level helpers | imported by anything; imports nothing from above |
119-
| `pages/` | page objects (locator builders + actions) | no `expect`, no specs imports |
120-
| `factories/` | Fishery test-data factories | no network, no `Date.now`, no `Math.random` |
121-
| `fixtures/` | Playwright fixture composition (`mergeTests`) | no business logic |
122-
| `specs/` | the actual tests (AAA structure) | no inline locators, no raw `fetch`, no `waitForTimeout` |
123-
| `tools/` | static-analysis validators | run as pre-commit checks and in CI |
118+
| Layer | Purpose | Boundary rule |
119+
| ------------ | ------------------------------------------------- | ------------------------------------------------------- |
120+
| `infra/` | env loader, low-level helpers | imported by anything; imports nothing from above |
121+
| `support/` | purchase flow + payment-boundary oracle (pure) | import concrete page objects; contain `expect` |
122+
| `pages/` | page objects — verb actions over private locators | no `expect`, no specs imports |
123+
| `factories/` | Fishery test-data factories | no network, no `Date.now`, no `Math.random` |
124+
| `fixtures/` | Playwright fixture composition (`mergeTests`) | no business logic |
125+
| `specs/` | the actual tests (AAA structure) | no inline locators, no raw `fetch`, no `waitForTimeout` |
126+
| `tools/` | static-analysis validators | run as pre-commit checks and in CI |
124127

125128
The validators (`tools/{validate-layout.sh, factory-rules.ts, fixture-rules.ts, lint-ui-spec.ts}`) catch boundary violations at lint time so they don't slip into a PR. See [`tools/README.md`](tools/README.md) for exactly what each one guards.
126129

@@ -154,8 +157,9 @@ The validators (`tools/{validate-layout.sh, factory-rules.ts, fixture-rules.ts,
154157
│ └── ux-findings.md # UX & functional defects discovered during automation
155158
├── tests/
156159
│ ├── infra/env.ts # zod-validated env access
157-
│ ├── factories/ # user, payment-draft, deterministic seed (_seed.ts)
158-
│ ├── pages/ # 7 page objects + BasePage (8 files)
160+
│ ├── support/ # payment-boundary oracle + shared purchase flow (pure)
161+
│ ├── factories/ # user factory + deterministic seed (_seed.ts)
162+
│ ├── pages/ # 6 page objects + BasePage (7 files)
159163
│ ├── fixtures/ # mergeTests of page-object fixtures
160164
│ └── specs/
161165
│ ├── signup/ # Scenario A
@@ -206,16 +210,16 @@ Without Make, the equivalent `npx playwright test ...` commands are documented i
206210

207211
| Sub-scenario | Spec | Browsers | Notes |
208212
| ------------------------------------------------------------------------------------ | ------------------------------------------------- | ------------------------- | --------------------------------------------------------------------------------- |
209-
| **A** Sign Up — happy path: Home → Log In → Sign Up → 1 year → Credit Card → payment | `tests/specs/signup/signup-flow.spec.ts:7` | chromium, firefox, webkit | `@smoke` |
210-
| **A** terms unchecked blocks payment | `tests/specs/signup/signup-flow.spec.ts:46` | chromium, firefox, webkit | negative |
211-
| **A** empty email keeps Next disabled | `tests/specs/signup/signup-flow.spec.ts:60` | chromium, firefox, webkit | negative |
212-
| **B** RU 1 month → `card_ru` (Карты Банков РФ) | `tests/specs/purchase/personal-vpn-ru.spec.ts:20` | chromium, firefox, webkit | `@smoke @ru` |
213-
| **B** RU 1 month → `stripe` (international card) | `tests/specs/purchase/personal-vpn-ru.spec.ts:20` | chromium, firefox, webkit | `@smoke @ru` |
214-
| **B** RU crypto path | `tests/specs/purchase/personal-vpn-ru.spec.ts:50` | n/a | `test.fixme` — gateway not exposed on `/payment/`. See `docs/ux-findings.md` B-1. |
215-
| **C** EN `1_month × stripe` | `tests/specs/purchase/personal-vpn-en.spec.ts:15` | chromium, firefox, webkit | `@en` |
216-
| **C** EN `1_month × crypto` (BTC) | `tests/specs/purchase/personal-vpn-en.spec.ts:15` | chromium, firefox, webkit | `@en` |
217-
| **C** EN `1_year × stripe` | `tests/specs/purchase/personal-vpn-en.spec.ts:15` | chromium, firefox, webkit | `@en` |
218-
| **C** EN `1_year × crypto` (BTC) | `tests/specs/purchase/personal-vpn-en.spec.ts:15` | chromium, firefox, webkit | `@en` |
213+
| **A** Sign Up — happy path: Home → Log In → Sign Up → 1 year → Credit Card → payment | `tests/specs/signup/signup-flow.spec.ts:8` | chromium, firefox, webkit | `@smoke` |
214+
| **A** terms unchecked blocks payment | `tests/specs/signup/signup-flow.spec.ts:36` | chromium, firefox, webkit | negative |
215+
| **A** empty email keeps Next disabled | `tests/specs/signup/signup-flow.spec.ts:49` | chromium, firefox, webkit | negative |
216+
| **B** RU 1 month → `card_ru` (Карты Банков РФ) | `tests/specs/purchase/personal-vpn-ru.spec.ts:11` | chromium, firefox, webkit | `@smoke @ru` (parametrised) |
217+
| **B** RU 1 month → `stripe` (international card) | `tests/specs/purchase/personal-vpn-ru.spec.ts:11` | chromium, firefox, webkit | `@smoke @ru` (parametrised) |
218+
| **B** RU crypto path | `tests/specs/purchase/personal-vpn-ru.spec.ts:35` | n/a | `test.fixme` — gateway not exposed on `/payment/`. See `docs/ux-findings.md` B-1. |
219+
| **C** EN `1_month × stripe` | `tests/specs/purchase/personal-vpn-en.spec.ts:13` | chromium, firefox, webkit | `@en` (parametrised) |
220+
| **C** EN `1_month × crypto` (BTC) | `tests/specs/purchase/personal-vpn-en.spec.ts:13` | chromium, firefox, webkit | `@en` (parametrised) |
221+
| **C** EN `1_year × stripe` | `tests/specs/purchase/personal-vpn-en.spec.ts:13` | chromium, firefox, webkit | `@en` (parametrised) |
222+
| **C** EN `1_year × crypto` (BTC) | `tests/specs/purchase/personal-vpn-en.spec.ts:13` | chromium, firefox, webkit | `@en` (parametrised) |
219223

220224
> Rows that share a spec line (B → `:20`, C → `:15`) are parametrised — one data-driven `test()` inside a `for` loop, not copy-paste.
221225
@@ -227,7 +231,7 @@ Without Make, the equivalent `npx playwright test ...` commands are documented i
227231
| --------------------------- | -------------------------------------------------------------------------------------------- |
228232
| Spec files | **3** |
229233
| Test cases | **30** (10 unique × 3 browsers) |
230-
| Lines of TypeScript / shell | **1 073** (factories + pages + specs + fixtures + tools) |
234+
| Lines of TypeScript / shell | **877** (factories + pages + specs + fixtures + support + tools) |
231235
| Cross-browser run duration | **6.8 min** (workers=1, retries=2, video on failure) |
232236
| Pass-rate (after retries) | **100%** of executed (24 passed + 3 flaky-but-eventually-passed) |
233237
| Fixme rate (known gaps) | **3 / 30 = 10%** (Scenario B crypto, see `docs/ux-findings.md` B-1) |

‎docs/ARCHITECTURE.md‎

Lines changed: 44 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,36 +6,42 @@ It is the deep companion to the short contract in [`../CLAUDE.md`](../CLAUDE.md)
66

77
---
88

9-
## 1. The 5 layers
9+
## 1. The 6 layers
1010

11-
The repository is organised into exactly five layers, pinned in [`../tests-config.json`](../tests-config.json). Imports flow in **one direction only**; a reverse import (e.g. a page object importing a spec) is a build failure, not a code-review nit.
11+
The repository is organised into six layers, pinned in [`../tests-config.json`](../tests-config.json). Imports flow in **one direction only**; a reverse import (e.g. a page object importing a spec) is a build failure, not a code-review nit.
1212

1313
```mermaid
1414
flowchart TD
1515
specs["specs/<br/>AAA tests, tags, assertions"]
1616
fixtures["fixtures/<br/>mergeTests composition + DI"]
17-
pages["pages/<br/>page objects: locators + actions"]
17+
pages["pages/<br/>page objects: verb actions"]
1818
factories["factories/<br/>Fishery + Faker, deterministic"]
19+
support["support/<br/>purchase flow + boundary oracle (pure)"]
1920
infra["infra/<br/>zod env loader, helpers"]
2021
2122
specs --> fixtures
23+
specs --> support
2224
fixtures --> pages
2325
fixtures --> factories
2426
fixtures --> infra
27+
pages --> support
2528
pages --> infra
2629
factories --> infra
2730
```
2831

29-
| Layer | Path | Responsibility | Must not |
30-
| ------------ | ----------------- | ---------------------------------------------------- | ---------------------------------------------------------------- |
31-
| `infra/` | `tests/infra` | zod-validated env access, cross-cutting helpers | import from any layer above |
32-
| `pages/` | `tests/pages` | locator builders + user actions (`BasePage` + 7 POs) | contain `expect`; import specs |
33-
| `factories/` | `tests/factories` | type-safe synthetic data via Fishery + Faker | touch the network, `Date.now`, `Math.random`, top-level mutables |
34-
| `fixtures/` | `tests/fixtures` | compose page objects + factories into injected deps | hold business logic |
35-
| `specs/` | `tests/specs` | the actual tests — Arrange / Act / Assert | inline locators, raw `fetch`/`axios`, `waitForTimeout` |
32+
| Layer | Path | Responsibility | Must not |
33+
| ------------ | ----------------- | --------------------------------------------------------------------------------- | ---------------------------------------------------------------- |
34+
| `infra/` | `tests/infra` | zod-validated env access, cross-cutting helpers | import from any layer above |
35+
| `support/` | `tests/support` | pure test orchestration: payment-boundary oracle, purchase flow + page interfaces | import concrete page objects; contain `expect` |
36+
| `pages/` | `tests/pages` | verb actions over private locators (`BasePage` + 6 POs) | contain `expect`; import specs |
37+
| `factories/` | `tests/factories` | type-safe synthetic data via Fishery + Faker | touch the network, `Date.now`, `Math.random`, top-level mutables |
38+
| `fixtures/` | `tests/fixtures` | compose page objects + factories into injected deps | hold business logic |
39+
| `specs/` | `tests/specs` | the actual tests — Arrange / Act / Assert | inline locators, raw `fetch`/`axios`, `waitForTimeout` |
3640

3741
There is **no** `components/`, `api/`, `clients/`, `generated/`, or `data/` layer. Those were removed during the reorganization; this project is black-box UI E2E with no API contract under our control (see [`CONSTRAINTS.md`](CONSTRAINTS.md) §2.1).
3842

43+
The `support/` layer depends on page objects only through interfaces (`OfferLandingPage`, `PaymentStep`) it declares itself — Dependency Inversion, so `purchaseVpn` is reusable across the RU and EN sites without knowing their concrete classes.
44+
3945
---
4046

4147
## 2. Data flow through one test
@@ -58,14 +64,14 @@ sequenceDiagram
5864
Spec->>PO: actions (open → login → signup → plan → method)
5965
PO->>Site: goto / click / programmatic input events
6066
Site-->>PO: navigation to payment provider boundary
61-
Spec->>PO: assert payment URL / DOM oracle (STOP — no Pay)
67+
Spec->>Spec: poll reachedPaymentProvider(url) (STOP — no Pay)
6268
```
6369

6470
Key invariants visible above:
6571

6672
- **No `baseURL`.** Three origins are in play, so each page object carries an absolute URL derived from `env.BASE_URL_*`. `goto()` is always explicit.
6773
- **Determinism.** All synthetic data comes from Faker seeded once in [`../tests/factories/_seed.ts`](../tests/factories/_seed.ts) (`SEED=1234`), so the same email/password is produced every run.
68-
- **The payment boundary is a hard stop.** Specs assert the provider page is reached and never submit card/crypto data ([`CONSTRAINTS.md`](CONSTRAINTS.md) §2.3). `PaymentRedirectPage` is an oracle only — it deliberately has no card-form locators.
74+
- **The payment boundary is a hard stop.** Specs poll the pure oracle `reachedPaymentProvider(url, origins)` in [`../tests/support/payment-boundary.ts`](../tests/support/payment-boundary.ts) and never submit card/crypto data ([`CONSTRAINTS.md`](CONSTRAINTS.md) §2.3). The oracle has no card-form interaction by construction.
6975

7076
---
7177

@@ -139,3 +145,29 @@ The skills carry portable copies of the same validators (e.g. `.claude/skills/pl
139145
### Disabled-by-constraint
140146

141147
For this black-box project, [`CONSTRAINTS.md`](CONSTRAINTS.md) §4 disables the OpenAPI-oriented capabilities (`api-client-from-openapi`, `playwright-test-author-api`, `/spec-sync`, `contract-drift-watch`). They remain committed to demonstrate that the workflow _supports_ contract testing when a repo owns a spec — they are simply switched off here.
148+
149+
---
150+
151+
## 5. Implementation notes (the non-obvious "why")
152+
153+
The test code is comment-free; intent is carried by names. The few facts a name cannot
154+
convey live here, so the knowledge is not lost.
155+
156+
- **`BasePage.toggleHiddenControl(locator)`** — the offer/gateway radios and the terms
157+
checkbox are native inputs that are visually hidden behind custom-styled cards and sit
158+
outside the layout viewport. A normal `.check()` can't reach them, so the helper sets
159+
`checked` and dispatches a bubbling `change` event to drive the site's JS state.
160+
- **`BasePage.fillReactively(locator, value)`** — Site A's `/order/` form (PrimeVue) only
161+
enables the Next + payment buttons after `input`, `change`, **and** `blur` fire;
162+
Playwright's native `.fill()` does not reliably emit `change` headless. The helper
163+
dispatches the full event chain.
164+
- **`SignupPage.fillEmail` waits for the plan toggle first** — that toggle renders only
165+
after the Vue form hydrates, so its visibility is a reliable "listeners are attached"
166+
marker; without the wait, events fire on an un-hydrated input when arriving from `/login/`.
167+
- **`reachedPaymentProvider(url, origins)`** — the boundary oracle. Success = navigated off
168+
our origins to a provider (or stayed in-house with a `gateway=` invoice param), and the
169+
URL is not a `*/failed|error|declined` page. It never inspects card forms.
170+
- **Known gaps (surfaced as `test.fixme` / omitted on purpose), see [`ux-findings.md`](ux-findings.md):**
171+
the RU site exposes no crypto gateway on `/payment/`; Site A's email validator is
172+
permissive (accepts addresses with no `@`), so that negative case is documented as a
173+
defect rather than asserted as passing.

‎tests-config.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44
"fixtures": "tests/fixtures",
55
"factories": "tests/factories",
66
"specs": "tests/specs",
7-
"infra": "tests/infra"
7+
"infra": "tests/infra",
8+
"support": "tests/support"
89
},
910
"naming": {
1011
"pageClass": "PascalCase",

0 commit comments

Comments
 (0)