Add characterization tests ahead of the merge into subscriptions - #70
Merged
Conversation
QMS had no tests at all. This adds a suite that runs the real router against a real PostgreSQL instance started by testcontainers, so the recorded responses describe what the service actually does rather than what a mock says it does. It is the acceptance criterion for the pending merge into the subscriptions service: the same tests must pass unchanged against the merged binary. The migrations are now embedded and applied through the iofs source driver instead of file://$PWD/migrations, so neither the tests nor the service depend on the working directory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Covers all eight /v1 path shapes terrain's clients/qms.clj calls, asserting
the full {result, error, status} envelope its route schemas require.
Two behaviors are recorded as they are rather than as they should be, so the
merge into subscriptions cannot change them silently:
- GET /v1/users/{username}/plan creates the user and subscribes them to the
default plan when QMS has not seen them before, so the read has a write
side effect and returns 200 rather than 404.
- POST /v1/usages answers 500, not 400, for an invalid update type.
httpStatusCode matches its sentinel errors with ==, but addUsage returns a
formatted error wrapping the offending value, so no case ever matches.
Quota and usage collections come back without an ORDER BY, so the harness
sorts those fields before comparing. Arrays returned directly by the listing
endpoints keep their order and are still verified.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The behaviors covered here are the ones the subscriptions service implements
differently, so a merge has to choose an implementation deliberately: whether
a usage change writes an audit row to `updates`, how a quota is addressed,
and that overlapping subscriptions get deactivated.
Writing them surfaced three bugs, all recorded as-is with a KNOWN BUG comment
so the merge cannot change them by accident:
- POST /v1/subscriptions?force=false fails for every request and reports
success anyway. GetActiveSubscriptionForDate passes no argument for the
placeholder in its Or() clause, so PostgreSQL rejects the statement, and
the handler buries the failure in a per-item failure_reason behind a 200.
Terrain's admin subscription route silently creates nothing.
- Every `updates` row records the ADD operation, even for a SET. GORM's
First only filters on the primary key, so the operation lookup ignores the
name it was given and returns whichever row sorts first. The running total
is unaffected; only the audit trail misreports.
- The two ways of subscribing a user store two different usernames for the
same person: the bulk endpoint keeps the configured suffix while every
other endpoint trims it, so an admin bulk-subscribing a user creates a
second user row their own routes never see.
QMS had no test workflow, so one is added, and the lint pin moves from
v0.0.4/go1.21 to v0.4.1/go1.25. The newer analyzers flag four redundant type
declarations in existing code; those are fixed here so the bump lands green.
Co-Authored-By: Claude Fable 5 <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
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.
Summary
QMS had no tests at all. This adds a suite that runs the real router against a
real PostgreSQL instance (testcontainers, stock
postgres:17, the repo's ownmigrations), so the recorded responses describe what the service actually does
rather than what a mock says it does.
It exists to make the planned merge of QMS into
cyverse-de/subscriptionsprovably behavior-preserving: the same tests move with the code and must pass
unchanged against the merged binary. That is the merge's acceptance criterion.
Coverage is scoped to the eight
/v1path shapes terrain actually calls(
terrain/src/terrain/clients/qms.clj), asserting the full{result, error, status}envelope terrain's route schemas require — not to100% of the API, since the rest has no live caller.
and timestamps redacted. The migration-seeded UUIDs are asserted exactly, so
a golden still pins which plan or resource type a response refers to.
differently — audit rows in
updates, quota addressing, subscriptiondeactivation — since those are where a merge silently changes semantics.
duplicated in both codebases; only one implementation survives the merge.
Migrations are now embedded and applied through the
iofssource driverinstead of
file://$PWD/migrations, so neither the tests nor the servicedepend on the working directory.
Bugs this surfaced
All three are recorded as-is with a
KNOWN BUGcomment rather than fixed,so the merge cannot change them by accident. Each should be fixed as its own
change, updating the corresponding assertion in the same commit.
POST /v1/subscriptions?force=falsefails for every request and reportssuccess.
GetActiveSubscriptionForDatebuilds.Or("? > subscriptions.effective_start_date AND ...")with no argument forthe placeholder, so PostgreSQL rejects the statement with
syntax error at or near ">". The handler turns that into a per-itemfailure_reasonand still answers200, so terrain'sPOST /terrain/admin/qms/subscriptionssilently creates nothing wheneverforceisn't set. This also means the upgrade-comparison rule the unforcedpath exists to implement has never run.
updatesrow recordsADD, even for aSET. The operation islooked up with
tx.First(&model.UpdateOperation{Name: usage.UpdateType}),but GORM's
Firstonly filters on the primary key, so it returns whicheverrow sorts first. The running total in
usagesis correct; only the audittrail misreports. The same unreachable lookup is why an invalid update type
returns
500instead of400.POST /v1/subscriptionspassesreq.Usernamethrough untouched while everyother endpoint trims the configured suffix, so an admin bulk-subscribing
user@domaincreates a second user row that the user's own/terrain/qms/user/*routes never see. Existing databases likely holdduplicate rows that need reconciling.
Also worth noting, and captured in a golden:
GET /v1/users/{username}/plancreates the user and subscribes them to the default plan when QMS hasn't seen
them, so the read has a write side effect and returns
200rather than404.That one appears intentional — terrain relies on it — so it is recorded as
expected behavior.
Notes
@v0.0.4/go1.21 to@v0.4.1/go1.25, and the newer analyzers flag fourredundant type declarations in existing code, fixed here so the bump lands
green.
unlike the env-var-gated database tests elsewhere in the org, these actually
execute in CI.
written and the test still fails, so a new baseline is committed
deliberately; an existing one is never overwritten.
Testing
go test ./...andgolangci-lint run ./...(v2.12.2, the CI pin) are clean;the suite was run repeatedly to confirm the goldens are stable.
🤖 Generated with Claude Code