Phase 3: datastore behavior tests on the MariaDB harness (retire the sqlmock pattern)#146
Merged
Conversation
…s; retire sqlmock auth tests Replace the sqlmock ValidateApiKey tests with behavior tests calling the real datastore against seeded fixtures (active key resolves active scopes only; revoked and unknown keys yield nil). ParseBearerToken's pure-function table moves to bearer_test.go unchanged. newMockDS moves into tickets_test.go temporarily until the remaining sqlmock ticket tests are replaced (issue #120). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
FindProfilesById (relation-key resolution asserted in both crossing directions, full Profile shape, secondary positions, not-found), FindProfilesByUsername, FindProfileByDiscordID (provider isolation: keycloak keys never resolve through the discord lookup), and FindProfileByGamertag — all against seeded fixtures, asserting domain data only. Not-found paths pin gorm.ErrRecordNotFound, which handlers map to 404. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…avior tests on the harness The three roster shapes (full/lite/S1 uniforms) asserted as domain data: relation-id keying, per-shape payloads, activity dates, uniform dates and the area-of-responsibility mapping. Position search pins primary and secondary matching, the frozen #137 empty-result contract (empty map, no error) and LIKE-metacharacter escaping. FindAwol pins the seven-day cutoff set (stale posters in, recent/never-posted/past members out) and the full entry shape. Rank and position-group catalogs pin ordering and the '----' divider filter — backed by a new divider fixture row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…qlmock pattern ListTickets pinned as domain data against seeded fixtures: default visibility and newest-first ordering with the id tie-break, full ticket hydration through the REAL reference cache (phrase names, category ancestors, participants, custom fields, derived counts), subtree-expanding category filter with the exclude opt-out, every filter knob plus conjunction, cursor walk without skips/duplicates, cursor resume inside a timestamp tie, malformed-cursor 400 sentinel, and forum_url assembly from FORUM_BASE_URL. GetTicket/GetTicketByRef, GetTicketFirstMessages (hidden exclusion in page and total, n cap) and ListTicketMessages (position-0 reachability — smoke ticket 6899 —, resume-after-cursor, hidden filter) covered likewise; ListCategories serves the nested-set tree in lft order. The last sqlmock tests and the newMockDS helper are deleted; the pure cursor-codec unit tests move to cursor_test.go unchanged. The sqlmock dependency is left in go.mod for a separate tidy-up (per wave plan). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r tests The new ./datastores harness tests opt in via the same TESTDB_ADDR seam as ./testdb, so the same env-wiring drift would silently degrade them to a no-op. Run both packages verbosely and fail on any skip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review findings on the #120 diff: * FindAwol exclusion map is keyed by forum user id, but the past-member check probed relation id 330 — no such user exists, so it pinned nothing. Probe the right keys and make each exclusion reason accurate. * No roster-1/2 fixture member had zero posts, so the never-posted exclusion was untestable. Seed Silent.I (user 302, relation 340, reserve) with no xf_post rows and pin the exclusion. * Past-member exclusion was shadowed: Discharged.F never posted, so the date filter excluded them regardless of the roster filter. Seed Retired.J (user 303, relation 350, roster 6) WITH a stale post so only the active-roster filter excludes them. * The non-relevant OPERATION record (1002) predated the latest award, so dropping the uniform-trigger's relevant-record-type filter changed nothing. Date it NEWER than every award/relevant record (1687000000) — now the filter is load-bearing. * Extend the position-search LIKE-escape test with the underscore metacharacter. Mutation evidence (each strengthened test fails when the guarded filter is removed): dropping posts.date cutoff -> user 302 flagged; widening roster filter to (1,2,6) -> user 303 flagged; dropping the relevant-record-type filter -> trigger moves to 1687000000. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…B-error path * openHarnessDatastore never closed gorm's pool — ~78 stranded idle connections per full run vs MariaDB's default max_connections=151. t.Cleanup the unwrapped *sql.DB. * The sqlmock retirement dropped the only coverage of ValidateApiKey's (nil, err) outcome — the 500-vs-401 auth boundary (mysql.go returns nil,nil for unknown keys but err for DB failures). Pin it behaviorally by killing the pool under a live datastore and asserting a non-nil error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* FindProfileByGamertag feeds raw input to LIKE (no escaping, unlike the position search) — pin the OBSERVED wildcard interpretation with an explicit escape-asymmetry flag, frozen pending the #137 verdict. * GetTicket/GetTicketByRef apply no discussion_state filter: deleted ticket 5 is directly retrievable — pin that list-visibility filtering is list-only. * GetTicketFirstMessages/ListTicketMessages on a nonexistent ticket: empty result, not an error. * ModifiedSince at exactly ticket 3's last_modified_date: boundary is inclusive (>=); note why 1740450000 sits between t4 and t3. * perPage=1 cursored walk across the hidden message gap (replaces the overclaiming single-page comment): cursor crosses position 1 without stalling or skipping. * FORUM_BASE_URL test restores the PRIOR viper value, not "". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* make test-integration: a failed 'docker compose port' produced an empty TESTDB_ADDR, every harness test skipped, and the target exited 0. Guard the discovered address and fail loudly when it's empty. * CI no-harness step asserted nothing: mirror the with-harness guard inverted — verbose run of ./testdb ./datastores teed to no-harness.log must contain at least one '--- SKIP'. * CI with-harness guard detected skips but not absence: add a positive sentinel requiring '--- PASS: TestValidateApiKey_ActiveKeyResolvesActiveScopes' in the log. Rename testdb-harness.log -> harness.log (covers both packages now). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n tests * bearer: 'inner extra spaces collapsed' renamed — the impl trims, never collapses; inner spaces would survive. * cursor: base64URL helper comment states intent (near-miss garbage via the real cursors' encoding); smoke-ticket-6899 breadcrumbs say what the regression was (empty cursor conflated with position 0). * TestMain discards datastores.Info/Warn (Error stays), mirroring contract/harness_test.go, so CI's tee'd harness.log stays readable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…map derefs - testdb: pin user 302's never-posted status alongside 301 — it is load-bearing for the AWOL never-posted exclusion in the datastores behavior tests, and a future recent post would silently re-vacuify it - ci: the no-harness step now requires the SAME named sentinel test to SKIP that the harness step requires to PASS — any single surviving skip no longer satisfies the gate while the rest drifts; reword both rationale comments (zero skips has more than one cause; the positive sentinel closes the deleted-function/build-tag gap, not the renamed- package one, which hard-errors under pipefail) - datastores: fail tests on gorm pool Close errors instead of discarding them; guard every direct roster-profile map deref with an ok-check so a missing relation is a Fatalf, not a nil-deref panic aborting the binary mid-suite Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- bearer: "whitespace is only trimmed, never collapsed" — the old phrasing contradicted the adjacent outer-whitespace case - tickets: cursor walk "advances past the hidden row and serves the next visible message" — the old "neither ... nor skipping past it" had an ambiguous antecedent that read as self-contradictory - profiles: state the gamertag LIKE-escaping asymmetry on its own terms instead of citing #137 (which is scoped to position-search empty results, not escaping) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SyniRon
added a commit
that referenced
this pull request
Jun 6, 2026
* Prometheus tracer: request counter route/method/status/key_id through the context label-holder (#130) metricsMiddleware (real impl, metrics.go) injects a mutable metricLabels holder into the request context; AuthMiddleware fills the key-id slot (decimal id, never the bearer), the routeLabel wrapper inside the mux fills the route slot from r.Pattern. Counter served by MetricsHandler (own registry + Go/process collectors) — the internal-only mount point for the cutover slice (#134), deploy-config assertion documented there. Adds github.com/prometheus/client_golang; go mod tidy also dropped the sqlmock leftover (#146 follow-up). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Metrics pins: histogram cardinality, error-tier metering, runtime collectors, bearer-absence, internal-only mount (#130) - duration histogram observes route/method only; test sweeps the family for any label outside {route, method} (no per-key histograms) - 401 meters with empty route/key_id (rejected before routing); scope 403 meters under the denied route with the key id; 404/405 meter under the bounded catch-all "/" pattern, never the raw path - go_* and process_* collectors asserted on the exposition - every auth tier driven with real cav7_ tokens, then the raw exposition swept: bearer material absent, key_id="101" present - /metrics through the public chain is 401/JSON-404, never the exposition; the exposition serves only from MetricsHandler's own listener Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Metrics: clamp method label to the RFC 9110 set, junk tokens meter as OTHER (#130) Unauthenticated clients could mint unbounded counter/histogram children per RFC 7230 token method — metrics sit outside auth. methodLabel clamps both call sites; bounded-sets comment updated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Metrics: defer recording so panicking handlers meter as 500, re-panic intact (#130) Recording ran after next.ServeHTTP returned — a panic bypassed metering entirely (flat error-rate dashboards under panic-per-request, #92). Deferred recording recovers, labels a never-written response 500 (sw.code==0 would otherwise read as implied 200), then re-raises so net/http and #132's recovery layer see semantics unchanged. sentryMiddleware placeholder notes the chain-order dependency for #132. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Metrics: wire promhttp ErrorLog to the package Error logger (#130) Nil ErrorLog swallowed gather/encode failures; *log.Logger satisfies promhttp.Logger. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Metrics: statusWriter.Unwrap so ResponseController tunnels through (#130) Without Unwrap, Flusher/Hijacker/deadline control silently vanished for every layer inside metrics. Pinned behaviorally over a real server conn. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Metrics: regression pins for the error tiers, 405 catch-all, self-scrape, gzip/HEAD, concurrency (#130) Pins (verified working, zero prior assertions): handler 500 under route+key_id; auth-tier 503 with empty route/key_id (plus the 503 tier in the bearer sweep, via a validateApiKey override on the fake); POST-on-GET 405 under the "/" catch-all; self-scrape neutrality on the family total; gzipped 200; HEAD under the GET pattern; 32 concurrent mixed-tier requests (race-clean). Internal Unwrap test made race-clean (channel, test bug). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Metrics: comment accuracy pass from the review (#130) Package doc: exposition listener is #134 future tense, not present; name the route/key_id labels instead of 'both labels'; drop the free-floating metricsMiddleware trailer. Counter+histogram Help: route="" covers every never-routed request (auth 401s, datastore 503), clause mirrored on the histogram. routeLabel: nil-holder check documented as mis-wiring defense, not the legacy-gateway path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Metrics: fix two panic-semantics comments, pin panic-after-written-200 as status=200 (#130) Round-2 review residuals (no functional changes): - metrics.go: the 500 label on a never-written panicked request is the conventional label for an aborted request — net/http recovers, logs, and closes the connection writing nothing (HTTP/2 resets the stream); it does not answer 500. Also states the committed-status rule. - rest.go: the #132 note's consequence was stale post-fix (recording is deferred; nothing vanishes). Real hazard: recovery INSIDE metrics that swallows a panic without writing meters as the implied 200, flattening error rates. Directive unchanged: recovery stays OUTSIDE metrics. - New test: a handler that writes a 200 then panics meters status="200" under the route (status already on the wire); panic still propagates. - go.mod: kylelemons/godebug indirect, pulled by prometheus testutil. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Closes #120. Part of #112 (PRD: Goodbye gRPC), Phase 3 — the SQL seam.
What
Replaces the sqlmock-style datastore tests — which asserted the implementation's own SQL back at itself — with behavior tests on the dockerized MariaDB harness (
testdb/): everyDatastoremethod is called against realistic fixtures and asserted on returned domain data, never SQL strings../datastoreswith positive + negative sentinels;make test-integrationhardened against silent port-discovery no-opdatastores_testpackage with one constructor seam — designed to survive the cutover type-swap with a mechanical updateNotes
go.moddeliberately untouched:go-sqlmockis now unused but its removal was held out of this branch (parallel-wave hot-file constraint).go mod tidyfollow-up after merge.🤖 Generated with Claude Code