test+refactor: 95.5% backend / 89.5% frontend coverage, and the SOLID fixes behind it - #66
Merged
Merged
Conversation
Backend coverage was 46.3% line / 39.1% branch, and the gaps were not evenly spread: whole handlers had no test at all, and the one unauthenticated write path in the application -- the payment webhook -- had none either. Adds 104 tests across four areas: - Refresh-token rotation, logout, registration. The rotation cases are the point: a refresh token is single-use, so replaying a spent one has to revoke the entire family rather than mint a new token, and an expired one behaves the same way. - 2FA confirm/disable/recovery. Every factor change must rotate the security stamp, and a recovery code must work exactly once -- both now asserted rather than assumed. - Read-side handlers and projections, including the ownership boundary: another user's order id returns the same "Order not found." as a nonexistent one, because confirming an id exists is itself a leak. - Webhook verification, in depth. A signature made with the wrong secret, a genuine signature for a substituted body, a shifted timestamp, a short signature, a missing header, and an unconfigured secret all have to fail closed; rotation (two v1 values) and uppercase hex have to succeed. One fake was lying: InMemoryCartRepository.TouchAsync did nothing, so a handler could forget to stamp the cart and still pass. It now mirrors the real repository. Application 67.5% -> 90.5%, overall 46.3% -> 58.2%. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three of the weaknesses found while reviewing the design against SOLID, each fixed rather than documented away. The order state machine lived in UpdateOrderStatusHandler, so the rule that governs an order was held by the code that happened to call it -- the entity would let anything assign anything. Order now owns it: OrderStatus.AllowedNext/CanTransition hold the table, and Order.TransitionTo applies a move or throws. The handler asks permission first and reports a refusal as a result, because a rejected transition is an expected outcome at that boundary, not an exception. Order.UnitCount joins it, so "how many items" is answered in one place instead of being re-summed by each projection. Quoting and checkout each computed shipping + tax + total themselves. They agreed, but only by inspection -- a change to one was free to diverge from the other, and the difference between the price shown and the price charged is the kind of bug you find in a chargeback. OrderPricer is now the single calculation both call. Building the order row moved out too (OrderDraft), which drops CheckoutHandler from 165 lines and eight dependencies to 130 and seven, and leaves it sequencing steps rather than performing them. The webhook endpoint knew the literal header "Stripe-Signature" -- one provider's detail inside provider-agnostic transport. Parsers now declare their own SignatureHeaders and the endpoint just asks. 40 tests cover the moved rules: every illegal transition including out of a terminal state, tracking numbers surviving a delivery update, tax never applying to shipping, and an empty cart never being quoted for delivery. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The frontend had 21 tests across two pure modules and no way to render anything: no @testing-library, no jsdom, no coverage tooling. A component that threw on mount, a conditional that stopped rendering, or a confirm dialog that stopped confirming would all have shipped green. Adds jsdom, Testing Library, and v8 coverage, plus 39 tests over the paths where a silent break costs the most: - ProtectedRoute, every combination of signed-in/staff/role -- including the half-written session (refresh token, no role) that must not open an admin screen, and the deliberate difference between bouncing to /login and bouncing to /store. - AdminWidgetsPage delete: nothing is sent before confirmation, cancelling sends nothing at all, a Manager is never shown the control, and an archive is reported differently from a delete because the outcomes differ. - CheckoutPage: totals come from the server and are re-fetched when the state or shipping method changes, the picked payment method is the token actually submitted, and a decline leaves the shopper on the page with the reason rather than stranding them. - AddToCartButton: the busy guard that stops a double-click ordering two. - DemoGuidePage: the "no payment is ever taken" reassurance, all three credentials, and the role differences -- the claims a reviewer would call a lie if they silently vanished. jsdom ships <dialog> without showModal/close, so the setup file supplies them; without that every modal-based component throws on mount. Frontend now 60 tests, 36.4% statements measured (was unmeasured). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fifty-five more component tests, taking the frontend from 36% statements to 86% (89.5% lines). What they actually guard: - LoginPage: the two-step branch stores nothing until the code is verified, a wrong code keeps the challenge so a second attempt needs no fresh password, and a guest cart merges into the account on the way in -- but a merge failure never undoes an accepted sign-in. - Layout: the admin entry point is absent for a visitor and for a customer, present for a Manager and an Administrator. Role leaks show up in the chrome first. - Cart: quantity controls send an absolute quantity, not a delta. - AdminOrderPage: the status POST carries the typed tracking number, and null rather than an empty string when none was entered; a refused transition shows the API's reason. - Storefront and product pages, including the states nobody looks at -- out of stock, nothing matched, catalog down, widget not found. - OrderConfirmationPage: settling an awaiting-payment order posts the right reference to the webhook, and a rejected webhook is reported rather than shown as success. - Password reset: the token comes from the query string, submission is blocked without one, and a forgotten-password request answers identically whether or not the address exists -- the account-enumeration guard, now asserted. - OrderDetailPage as the receipt: every money line, the tracking number, and the print path. Two test-harness gaps fixed along the way: renderWithProviders can seed router location state (how the confirmation page receives its order) and take a route pattern separate from the URL, without which any :param page rendered blank and silently tested nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Infrastructure sat at 14.5% because the repositories are mostly SQL, and SQL cannot be tested against an in-memory fake -- a fake would only prove the fake works. 53 tests now run against a real PostgreSQL, on a throwaway database created and dropped per run, migrated by the same DbUp scripts the application runs at startup. The one that justifies the whole suite: ten concurrent buyers, two units each, ten in stock -- exactly five may win. Overselling is prevented by a conditional UPDATE inside a transaction, and nothing short of concurrent connections against a real server can demonstrate that. Alongside it: a refused reservation rolls the order row back too, a decline returns the stock to the shelf, and an AwaitingPayment order keeps its reservation so it cannot be sold twice while the provider settles. Also covered: SKU uniqueness folded through upper(), the ON CONFLICT cart upsert, cascading cart deletes, refresh-token family revocation, single-use recovery codes that are useless to another user, reset tokens invalidated when a newer one is issued, and a seeder that can run on every boot without duplicating an account or resetting a password someone changed. Two real findings, both fixed: Dapper's snake_case mapping is global process state that was set inline in AddInfrastructure. Any repository built outside the DI container -- a test, a console tool, a migration script -- silently mis-mapped every multi-word column, so tracking_number and order_number came back as defaults while single-word columns worked. That reads as missing data, not missing configuration. It is now DapperConfiguration.Apply(): explicit, idempotent, callable. Testcontainers was the obvious tool and was rejected: it pulls SSH.NET 2024.2.0, which has a known high-severity advisory, and this repo builds with NuGet audit as an error. The suite uses the Postgres that docker compose and CI already provide instead. Backend line coverage 46.3% -> 83.1%; Infrastructure 14.5% -> 72.1%. 273 backend tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… settings Takes the backend from 83% to 95.5% merged line coverage by testing the adapters that were still dark, all of them on paths where being wrong is expensive. - The Stripe adapter through a stub transport: the amount really is sent in minor units and rounded away from zero, the order number really is in metadata (which is how the webhook finds the order later), and each PaymentIntent status maps to the branch checkout expects. A status mapped to the wrong branch either ships goods nobody paid for or cancels an order that settled. - Google ID tokens, validated end to end against a locally generated RSA key served as JWKS. Tested by forging: right shape wrong signer, right signer wrong audience, right everything but expired. Each is refused, silently, and Google being unreachable refuses rather than surfacing as a 500 from our login endpoint. - The 2FA challenge token, which deliberately grants nothing on its own: an access token presented in its place is rejected on the purpose claim alone, and a tampered signature, foreign issuer, or foreign audience are all refused. - Email, where two historical bugs lived. The MIME shape (plain body, single HTML alternate view) and the HTML escaping of interpolated values are now assertions rather than memories. Building the message moved out of SmtpEmailSender so it can be checked without an SMTP server; delivery itself still needs one and stays uncovered. coverlet.runsettings makes the number honest. Composition roots are excluded with the reason written down -- asserting "AddScoped was called" restates the code, and the smoke test already proves the container resolves. CompilerGeneratedAttribute is deliberately NOT in the exclude list: every async method compiles to a state machine carrying it, so excluding it silently drops most of the codebase and reports a meaningless number. That cost an hour, and the comment exists so it costs nobody else one. Application 95.0%, Domain 95.7%, Infrastructure 96.2%. 348 backend tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The test suite gained a fourth job — the repository integration tests, against a real PostgreSQL service container — and two floors that fail the build on a regression. The backend floor is checked in its own job rather than inline, because neither suite reaches it alone: the repositories are only exercised by the integration tests and the handlers only by the unit tests. Both upload their cobertura report, and the floor job merges them by taking the highest hit count per line. Summing or averaging would understate the real figure, since a line covered by one suite is missed by the other. Floors are floors, not targets: 90% backend, and frontend thresholds in vitest.config.ts. They exist to catch a regression, not to invite tests written to move a number. check-coverage.sh executes each python candidate before accepting it. Windows ships a "python3" App Execution Alias that resolves on PATH, prints an advert for the Store and exits 0 — which turned the gate into a no-op that always passed. It was only noticeable because the floor also "passed" when set to 99%. Both new action pins were resolved against the GitHub API before use rather than recalled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ore bugs The testing chapter described three layers and a gate that no longer matches the workflow. It now covers all four -- backend unit, PostgreSQL integration, frontend component, end-to-end smoke -- with what each proves and what each needs, the real coverage figures, and the CI jobs including why the floor is checked in its own job rather than inline. The bugs table gains rows 33-39, a different class from the earlier ones: global state that only misbehaves outside the DI container, a coverage exclusion that silently dropped most of the codebase from measurement, a gate on Windows that reported success while doing nothing, an audit gate correctly rejecting a convenience dependency, a library that ignores the injected clock, and a test asserting a constraint the schema never had. Row 39 is a repeat of row 23 -- .at(-1) against an ES2020 target -- caught only because the build runs tsc and the test run does not. Left in rather than quietly fixed: a lesson that had to be learned twice is worth more written down than a table that implies it was learned once. Architecture records the two structural changes: the order owns its fulfilment transitions, and one pricer serves both the quote and the charge. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e floor Adding the integration project to the solution quietly broke `ci.yml`: its test step runs `dotnet test` across the solution, which now needs a PostgreSQL. It gets one as a service container, so every PR proves the reservation logic rather than only a deployment does, and the coverage floor runs on the merged report from the same command. The formatting gate has carried `continue-on-error: true` since the scaffold, with a note saying to run `dotnet format` once a local SDK was available and then re-enable it. A local SDK is available, so: formatted (two files), gate enabled. A warning nobody reads is not a gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dependency ReviewThe following issues were found:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Answering two questions that turned out to be the same question: does this actually follow
SOLID, and where are the tests? The honest audit found four real weaknesses; fixing them and
then covering the result took the suite from 165 tests to 463.
Coverage
Floors are enforced in CI — 90% backend, thresholds in
vitest.config.ts— so this cannotquietly slide back.
The SOLID fixes
The order now owns its fulfilment rules. The transition table lived in
UpdateOrderStatusHandler, so the rule governing an order was held by the code that happenedto call it, and the entity would let anything assign anything.
OrderStatus.AllowedNext/CanTransitionhold the table;Order.TransitionToapplies it.One pricer, two callers. Quoting and checkout each computed shipping + tax + total
themselves. They agreed by inspection only — and the gap between the price shown and the
price charged is the kind of bug you find in a chargeback.
OrderPriceris now the singlecalculation behind both.
CheckoutHandlersequences steps instead of performing them — 165 lines and eightdependencies down to 130 and seven, with order construction in
OrderDraft.The webhook endpoint no longer knows
Stripe-Signature. Parsers declare their ownSignatureHeaders; transport just asks.The tests
Four layers now, all four gating deployment:
secret, genuine signature for a substituted body, shifted timestamp, missing header,
unconfigured secret. All must fail closed; key rotation and uppercase hex must succeed.
prove the fake works. Ten concurrent buyers, two units each, ten in stock: exactly five may
win. Nothing short of concurrent connections to a real server demonstrates that.
never sees the control, the picked payment method is the token submitted, a decline leaves
the shopper on the page with the reason, and 2FA stores no session until the code verifies.
Four findings worth reading
Dapper's snake_case mapping is global process state, set inline in
AddInfrastructure.Any repository built outside the container silently mis-mapped every multi-word column —
tracking_numbercame back null whilestatusworked, which reads as missing data ratherthan missing configuration. Now
DapperConfiguration.Apply().A coverage exclusion that deleted the codebase.
CompilerGeneratedAttributeinExcludeByAttributedrops every async state machine — that is, nearly everything. Coverage"improved" for an unexplained reason, which is how it was caught.
A gate that passed while doing nothing. Windows resolves
python3to a Store alias thatprints an advert and exits 0, so
check-coverage.shalways succeeded — noticed only bytesting its failure path at a 99% floor.
Testcontainers was rejected, not overlooked: it pulls
SSH.NET 2024.2.0, which carries aknown high-severity advisory, and this repo builds with NuGet audit as an error. The suite
uses the Postgres compose and CI already provide.
Also:
ci.ymlneeded a database once the integration project joined the solution, and theformatting gate's
continue-on-errorbootstrap exemption is spent, so it is enforced now.🤖 Generated with Claude Code