0ec606439125198a1fddec432a3f6a9917d9eba0
7
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 bermudalamb
|
5ef97bef21 |
fix(cart): make the reservation countdown tick, and warn against the real hold (#97)
The cart showed "2h 15m left" and turned it red in the final hour. Neither updated. Both readings happened during render from the wall clock, and nothing scheduled a re-render — no setInterval anywhere in the file — so the number a customer read was whatever it was when the page loaded, and the warning colour could only appear by accident, because the component had already rendered before the final stretch began. That matters more here than in most shops: every item is one of a kind, so a lapsed reservation is not "buy it later", it is someone else buying the only one. New useNow hook returns the time as state rather than merely forcing a re-render, and that is the point. A component reading Date.now() while rendering produces output that depends on the clock, which React is entitled to assume it does not — react-hooks/purity says so, and this was the only instance in the codebase precisely because it was the only place doing it. Reading `now` from state makes render a function of its inputs again, so the rule is satisfied rather than suppressed. It ticks every 30s, which matches the display's one-minute resolution, and only while the cart holds something, so an empty cart is not waking React forever. A second defect the issue did not mention. The red warning was hardcoded to the final hour, but the hold became admin-configurable in #136 and accepts values as low as half an hour — so on any setting below an hour every item was red from the moment it was reserved, and a warning that is always on is not a warning. It now keys off the last tenth of the item's own added_at-to-expires_at span. Reading it from the item rather than from the setting also means an admin changing the value does not retroactively relabel a reservation granted under the old one. At zero the row keeps saying "expiring…" and the page refetches on each tick while anything is lapsed, so it clears within one interval of the server's sweep actually releasing it. The client cannot know when that lands — the sweep runs every few minutes — so the wording claims imminence, which is true, rather than completion, which is not ours to say. The header badge is refreshed alongside, since it counts held items and goes stale the same way. Verified against the unfixed component, not just the fixed one: two of the three new tests fail on the old code. The third documents the "expiring…" wording rather than the fix, and passes either way — worth having, but it is not evidence. The tests use Playwright's clock control rather than waiting in real time, which also makes them deterministic: without it, "the text changed" would depend on where in the minute the run happened to start. Refs #97 |
||
|
bermudalamb
|
3b3bd06bbe |
test(e2e): convert the remaining admin specs, completing the POM refactor (#137)
The last nine: admin-taxonomy, admin-save-failures, admin-theme, admin-inline-category, admin-item-preview, admin-disable-customer, admin-reserved-items, admin-inventory-filters, email-templates and admin-email-settings. Every spec in tests/e2e now goes through page objects. Measured rather than asserted: - raw CSS and antd-internal locators in spec files: 0 (was 13) - local `register` helpers: 0 (was 9) - hardcoded http://localhost:5173: 0 (was 3) The antd knowledge that was spread across seven spec files is now in four page objects, each with the reason written next to it. Three of those are things no reader could have guessed from the locator: Segmented hides its real radio behind a styled label, so the input is found by role and cannot be clicked — the title attribute is the handle. The status multi-select renders an invisible role="listbox" shim beside the real list, so getByRole('option') finds something zero-sized; and a selected status renders again as a tag carrying the same title, so an unscoped getByTitle is ambiguous. Matching the visible option class avoids both. The dropdown is also opened only when closed, because antd keeps it open after a selection in multiple mode. Select popups render into a portal at the end of <body>, outside the tab panel they belong to, so a dropdown cannot be found by scoping to the panel. AdminPage.tab gained an `exact` flag for one specific collision: the Emails tab contains a rail that also renders tabs, and "Email verification" contains "Email". Without exact matching, opening the Emails tab is ambiguous with the template inside it. Two helpers stayed local rather than moving into page objects, because they belong to their file's subject rather than to a surface: favorites-filter's `favorite()`, which settles the opt-in modal and waits for its fade before the wrapper stops intercepting pointer events, and admin-theme's `luminance()`, which is a WCAG calculation and not a locator. Both now take page objects as parameters instead of reaching for locators themselves. Verified: 26/26 spec files converted, tsc clean over the whole tree, lint at the 30-warning src baseline with nothing added. Full suite 123 passed / 5 failed; all five pass in a 33/33 serial re-run, which is the load-related flakiness this suite has had throughout and not a change here — the backend hashes passwords with bcryptjs, a pure-JS implementation that blocks the event loop for every request while it runs. Closes #137 |
||
|
bermudalamb
|
549a08038e |
test(e2e): convert the favorites, availability and orders specs (#137)
favorites, favorites-filter, sold-filter, orders and pending-publish. Five more copies of "register a customer" and three more of the hardcoded base URL go with them. Two locators that were hiding real knowledge are now named. `gridCell` is the item's whole antd column rather than its card, needed because the SOLD ribbon renders outside the card — three specs reached for `.ant-col` directly to get at it. And `chooseAvailability` goes through the title attribute because antd's Segmented hides the real radio behind a styled label, so the input is found by role and cannot be clicked; that fact was written out twice in comments and is now written once in code. The favorite control is located page-wide rather than within a card. The storefront paginates as items accumulate and the control is named for its item anyway, so scoping to a card bought nothing and broke whenever the card was on another page. favorites-filter keeps its local `favorite()` helper. It is genuinely local — decline the opt-in, wait for the fading modal to stop intercepting pointer events, confirm the heart flipped — and belongs to that file's subject rather than to the storefront. It now takes page objects as parameters instead of reaching for locators itself, which is what a spec-level helper should look like. Two specs still drive the registration form rather than taking the `customer` fixture, and deliberately. Both are about a signed-out visitor being interrupted mid-action — favoriting an item, or switching on the favorites filter — and the claim is that the thing they asked for survives the interruption. Replacing the interruption with an API call would delete the test. Verified: favorites 6/6, favorites-filter 7/7, sold-filter 6/6, orders and pending-publish 8/8. Notably favorites-filter's "keeps showing a favorite after it sells" passes, which had been failing on a strict-mode violation from two items sharing a name across runs. Refs #137 |
||
|
bermudalamb
|
9b3a03d1bf |
test(e2e): convert the storefront and filter specs onto page objects (#137)
storefront, storefront-errors, theme, error-boundary and filters. filters.spec.ts carried the last hand-rolled copies of createCategory, createTag and createItem, and the hardcoded `http://localhost:5173` that meant changing the port in the config would have moved every test except this one. Seeding now goes through support/api, and the host it needs lives in one constant. It has to be a constant rather than the config's baseURL: beforeAll runs with worker-scoped fixtures only and cannot read a test-scoped option, which is why the URL was inlined there in the first place. New FilterDrawer object. The two rules it encodes are the ones the tests exist to pin down and neither is guessable from a locator: categories are a tree because the filter matches a node and everything filed beneath it, and tags combine with AND rather than OR, so selecting two means "must have both". The active-filter chips go on StorefrontPage rather than the drawer, because that is where they render — and the scoping matters, since the drawer carries a "Clear all" of its own that an unscoped locator also matches. StorefrontPage gains the three things the catalogue says instead of listing items. They are named together deliberately: the distinction between "No items yet" and "Couldn't load items" is the point, and several tests assert one is showing while the other is not, because telling a customer the shop is empty when the server is broken hides the outage. The theme switch and the attribute it writes are both on Header now. The switch is in the header and `data-theme` lands on <body>, so a spec previously had to know about `body` to observe the control it had just clicked. Verified: 12/12 across the four small specs, 8/8 on filters, tsc clean, lint unchanged at the 30-warning src baseline. Refs #137 |
||
|
bermudalamb
|
abcc684447 |
test(e2e): convert the auth specs onto page objects (#137)
First conversion batch: auth, password-reset, resend-verification. verify-email is left alone — it already used semantic locators and duplicated nothing, and rewriting it to prove a point would be churn. Four of the nine copies of "register a customer" go here. auth.spec.ts and password-reset.spec.ts each carried their own `register`, and resend-verification.spec.ts its own `registerCustomer`, all three re-explaining the same bcrypt wait and the same "the header, not the URL, proves the session exists" reasoning in slightly different words. Two also carried their own `logout`, and two their own `uniqueEmail` with different prefixes. Most of those tests were not about registering. They needed an account to exist so they could test logging out, resetting a password, or resending a verification email, and they paid for a bcrypt round-trip through the form to get one. Those now take the `customer` fixture, which registers through the API. The three tests that genuinely are about the registration form still drive it, because the thing under test has to be the thing exercised. The batch drops from 47s to 26s as a side effect, which is the cost of that round-trip made visible. password-reset.spec.ts loses its inline pg.Client. The reasoning for reading the database directly is unchanged and still right — an endpoint returning a reset token for an arbitrary address is account takeover if it is ever reachable — but it now lives in support/db.ts where it cannot be copied into the next spec wanting a shortcut. It also stops defaulting to port 55432, which is the integration suite's disposable Postgres rather than the database the app under test is connected to, and is Hyper-V-reserved on at least one machine here. Both tests in that file previously failed with a bare ECONNREFUSED unless TEST_PGPORT was set by hand; they now pass with no environment at all. New PasswordResetPages object covers both halves of recovery — requesting a link, and using one — because they are one flow and a test usually crosses between them. One lint decision worth recording. Requesting a Playwright fixture IS using it: destructuring `customer` is what makes the account exist, whether or not the body then reads the address. The linter cannot see that side effect and reports every such fixture as an unused variable. The first attempt at appeasing it was a `void customer;` line per test, which is noise standing in for a comment — and sonarjs flags that too, so it traded one warning for another. `no-unused-vars` is now configured with `args: 'none'` for tests only, with the reason written next to it. Variables are still checked; only parameters are exempt. Verified: 26/26 in the converted batch, and 127 passed in the full suite with four failures — three in the known #116 flaky family, and resend-verification's rate-limit test, which passes 9/9 across three repeats in isolation and is timing-sensitive under parallel load rather than changed by this commit. Refs #137 |
||
|
bermudalamb
|
5c907fcf9a |
test(e2e): add page objects, fixtures and a typed test build (#137)
Foundation only. No spec is converted in this commit, so the suite behaves exactly as before — the conversions follow in themed batches, each leaving the suite green. The suite had grown by copy-paste. Registering a customer was implemented nine times, as `register` in five files and `registerCustomer` in four more, each carrying its own re-explanation of the same bcrypt wait and the same "the header, not the URL, proves the session exists" reasoning. `uniqueEmail` was reinvented per file with a different prefix and a different encoding each time. Thirteen locators reached into antd's internals — `.ant-tabs-tab-active .ant-tabs-tab-btn`, `.ant-select-item-option[title=...]`, `.ant-col` — spread across seven files, so an antd upgrade breaks tests that have nothing to do with it. Page objects hold named locators and the actions that operate on them; assertions stay in the specs, so a test reads as its own statement of what it verifies. The exception is an action waiting for its own completion — registering waits for the account button, opening an admin tab waits for its panel — because that wait is the action's contract, and pushing it to callers would recreate the duplication being removed. Fixtures carry the setup rather than the specs. `customer` registers through the API rather than the form: nine specs drove the registration form purely to arrive at a signed-in session, so a broken form failed a hundred tests that were not about it, and each paid for a bcrypt round-trip through the UI. `page.request` shares the browser context's cookie jar, so the session belongs to `page`. The specs that are genuinely about registration drive the form properly through `authModal`. `adminApi` takes its base URL from the Playwright config. One spec built its own request context against a hardcoded http://localhost:5173, so changing the port in the config would have moved every test except that one. The inline pg.Client in password-reset.spec.ts moves to support/db.ts. The reasoning for reading the database directly is unchanged and still right — an endpoint that returns a reset token for an arbitrary address is account takeover if it is ever reachable, and an environment gate is a thin thing to stand between that and production — but it no longer sits in a spec where it can be copied into the next one wanting a shortcut. Its default port becomes 55500, the local stack's, rather than 55432: that is the integration suite's disposable Postgres, a different database with different credentials that the app under test is not connected to, and it is Hyper-V-reserved on at least one machine here, so the spec failed with a bare ECONNREFUSED naming a port nobody had chosen. tsconfig.test.json type-checks the tree and runs as part of `npm run build`. It is separate from tsconfig.json rather than widening its `include`, because scripts/check-sonar-tsconfig.js compares the two configs' include arrays, and pulling the Playwright suite into SonarQube's analysis program is a different decision from type-checking it. The whole existing suite type-checks clean on the first run. Lint now covers tests/ with `project` rather than `projectService` — the service resolves a file to the nearest tsconfig.json, which for tests/ is the one that excludes them, and every file then errors as not part of a project. no-floating-promises is an error here: Playwright's API is almost entirely promises, and a missing await on an assertion does not fail, it passes having asserted nothing. Four rule families are switched off for tests rather than left as warnings. Bringing these files in scope added 45, of which none were defects, and #60's argument is that a gate nobody reads is not a gate. There is no React in this directory, and the hooks rules fire on ordinary functions whose parameter is named `use` — which Playwright fixtures are, by its own API. Test credentials are the point of a test and the project's own rule is that they live only in test paths, which is here. Math.random builds unique fixture names so parallel workers do not collide, and a cryptographic generator would say something untrue about what the value is for. The count is back to the 30 that src carried before. Refs #137 |
||
|
bermudalamb
|
332c1e7cd0 |
feat(ci): import test coverage into SonarQube (#61)
SonarQube reported 0% coverage for 78 unit, 134 integration and 83 end-to-end tests, so the coverage-on-new-code gate — the most useful thing SonarQube offers a project this size — has been failing permanently while looking configured. It now reports 69.6%, verified by a real scan. Backend coverage comes from both suites, written to separate directories because jest writes coverage/lcov.info by default and the second run would silently overwrite the first. Both are needed rather than just the fast one: the unit suite alone reports 11%, because everything in src/routes is exercised by the integration suite. That suite is manual-only after hanging for 3h12m post-run, so it runs here with --forceExit and the job carries a hard timeout; jest confirmed during testing that it would otherwise have hung. The frontend had no unit tests at all, so its coverage comes from Playwright driving an istanbul-instrumented dev server, collected per test by an auto-fixture and merged with nyc. The 17 specs now import from a local fixtures module that re-exports @playwright/test, which is what lets the fixture attach without touching each test body. Instrumentation is gated behind COVERAGE=true and loaded by dynamic import, since vite-plugin-istanbul is ESM-only while vite.config.ts evaluates as CommonJS. Both directions were checked rather than assumed: a normal build contains no instrumentation, and the dev server instruments nested modules as well as top-level ones — the first attempt used an include glob of src/* which would have silently missed everything under src/admin and src/cart. coverage:report fails when nothing was collected instead of writing an empty report, and that guard was fired deliberately to confirm it works. This project has been bitten twice by tools succeeding while measuring nothing — SonarQube skipping the whole frontend and still exiting EXECUTION SUCCESS in #67, and an ESLint matcher silently matching no files during #60 — and coverage has exactly that shape: an uninstrumented dev server lets every test pass while gathering nothing, and the 0% that follows reads as lost coverage rather than broken collection. Worth knowing when reading the numbers: end-to-end coverage flatters. Istanbul marks a line covered when the browser ran it, so a component rendered during a test counts as covered with nothing asserting anything about it. Recorded in the design doc and the project context rather than left to be discovered. Also declares sonar.tests so test files are analysed under the test rule set rather than as production code. Closes #61 |