test: align suite with the updated testing skill guideline - #863
Merged
Conversation
toEqual to toStrictEqual, describe-free "it " test titles, and a duplicate test removed, per the updated testing skill.
Converts try/catch narrowing to toThrowWithMessage/toMatchObject rejection assertions, try/finally connection teardown to onTestFinished, hand-rolled env restore to updateEnv (registering registerBunTestCleanup in @vers/db's preload so it takes effect), a module-level polling helper to the shared waitFor, and two job-queue sleep-then-drain waits to a polled drain.
Replaces isRedirect .catch() ternaries and instanceof Response throw-guards with direct rejects.toMatchObject assertions and invariant() narrowing; converts waitFor(() => getBy*) to findBy* where the check is a DOM query; seeds the @msw/data store instead of hand-crafting service responses; replaces the sole idle-worker raw-store write with its sanctioned setter; drops a satellite-store cleanup the preload's registerZustandReset already owns; and moves factory tests to full-shape toStrictEqual pairs.
Replaces factory-fed contract-schema tests with inline literal payloads and issue-path assertions, pins the checkpoint-hash golden test to toMatchInlineSnapshot, and replaces run-replay-segment's 369-line hand-written checkpoint fixture with a regenerated inline snapshot verified against the deleted literal's content.
Binds each service's env.DATABASE_URL boot test to its factory's own teardown (stopTelemetry, or the job queue's stop for email) so the booted service no longer leaks past the test. Adds the missing cross-actor and anonymous-caller authorisation pairs for advanceActivity and getLatestActivityProgress, and splits trackActivityProgress's mislabeled "foreign or missing" test into its two actual cases.
Replaces hand-rolled OTel meter-provider harnesses with the shared createInMemoryMetrics, routes sentryHandle test writes through setSentryHandleForTesting with capture-and-restore, and drops the checked-in JWT keypair for getTestJWTKeyPair. Production change: findCurrentSimVersion's deployedAt ordering gets a createdAt tiebreaker (sim_versions has no id/sequence column, so createdAt is the closest analog to release-registry's id tiebreaker) plus a covering test, following the testing guideline's timestamp-ordering rule.
bun types .rejects/.resolves chains as synchronous, so awaiting them
trips await-thenable and no-confusing-void-expression; the fixes in
prior waves awaited these to force ordering against a following
cookie/db read. Switches those sites to the documented drain-then-
assert idiom (await promise.catch(() => {}) before the unawaited
rejects assertion) and un-awaits the sites nothing depends on.
Also braces two waitFor callbacks in build-router.test.ts that
returned a void expression as an arrow shorthand.
createdAt defaults to now() pinned to transaction start, so two rows written in one transaction tie on it too and the ordering stays arbitrary. engineHash is sim_versions' actual primary key and gives a genuine total order, matching the pattern in find-latest-release. The tie test now asserts the winner by engineHash instead of hand-setting createdAt.
Removing the hand-rolled otel harnesses left sdk-metrics with no importer in product-analytics and service-activity, and knip flags both as unused. Both packages already use createInMemoryMetrics from test-utils instead.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe pull request modernizes tests across the web app, contracts, libraries, and services. It replaces manual asynchronous handling with direct rejection and query assertions, uses persisted and shared fixtures, adds authorization coverage, and strengthens replay snapshots and factory checks. ChangesWeb test synchronization and redirect handling
Contracts, services, and shared test infrastructure
Replay test coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 75 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…tion Remove the no-op stopTelemetry disposals (the factories expose no real teardown), restore regex-style messages with secret-absence assertions in the keys parsers, gate the continue-here click on the avatar query cache via the render util's exposed queryClient, drop a redundant findByRole, tighten retry-drain timeouts, and align replay factory test titles.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/testing/client-test-utils/src/orpc/build-contract-mock.test.ts (1)
99-116: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the error
codeso the test matches its title.The title claims the rejection surfaces as a defined
ORPCError. The assertion now checks onlydata.reason, so any rejection value that carries thatdatashape passes. Include thecodein the matched object to keep the defined-error contract covered.🧪 Proposed assertion
- expect(client.getSecret({})).rejects.toMatchObject({ data: { reason: 'missing-session' } }); + expect(client.getSecret({})).rejects.toMatchObject({ + code: 'UNAUTHORIZED', + data: { reason: 'missing-session' }, + });Replace
UNAUTHORIZEDwith the code declared for this procedure insecretContract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/testing/client-test-utils/src/orpc/build-contract-mock.test.ts` around lines 99 - 116, Update the rejection assertion in the mock-thrown typed-error test to also match the error code declared for getSecret in secretContract, replacing the hardcoded UNAUTHORIZED expectation with that contract-defined code while preserving the existing data.reason check.</code>Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/routes/-game/satellite-stack.test.tsx`:
- Around line 1-3: Restore test cleanup in the satellite registration setup by
importing and using onTestFinished with removeSatellite for the avatar-viewer
registration in satellite-stack.test.tsx. Ensure cleanup runs after each test so
registered satellite state cannot leak or conflict with later tests.
In `@contracts/avatar/src/avatar-data-schema.test.ts`:
- Around line 6-34: Replace the inline valid AvatarData payloads in the affected
tests with the package’s faker-defaulted create-mock-* factory, overriding mode
to trade and self_found respectively. Preserve the existing assertions and keep
the invalid hardcore payload inline.
In `@services/replay/src/dispatch/run-replay-segment.test.ts`:
- Around line 533-909: Consolidate the replay-output assertions in the relevant
test into one inline snapshot golden record. Capture the shared expected output
once, then have both the remote-dispatch and traceparent result assertions
compare against that record, preserving the parity checks without duplicating
the full snapshot.
---
Outside diff comments:
In `@libs/testing/client-test-utils/src/orpc/build-contract-mock.test.ts`:
- Around line 99-116: Update the rejection assertion in the mock-thrown
typed-error test to also match the error code declared for getSecret in
secretContract, replacing the hardcoded UNAUTHORIZED expectation with that
contract-defined code while preserving the existing data.reason check.</code>
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 836e2aae-156d-4004-a48d-676b4633fdc0
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (70)
apps/web/src/lib/activity/use-activity-rewards.test.tsxapps/web/src/lib/auth/run-logout.test.tsapps/web/src/lib/avatar/require-active-avatar.test.tsapps/web/src/lib/forms/use-form-submit.test.tsxapps/web/src/lib/rpc/clients/session-existence-client.test.tsapps/web/src/lib/rpc/clients/session-refresh-client.test.tsapps/web/src/lib/rpc/clients/user-client.test.tsapps/web/src/routes/-account-2fa-verify/load-two-factor-setup.test.tsapps/web/src/routes/-account-2fa-verify/verify-two-factor-setup-handler.test.tsapps/web/src/routes/-account-change-email/run-change-email.test.tsapps/web/src/routes/-account-change-password/run-change-password.test.tsapps/web/src/routes/-account/run-disable-two-factor-auth.test.tsapps/web/src/routes/-activity/activity-panel.test.tsxapps/web/src/routes/-avatar-create/run-avatar-create.test.tsapps/web/src/routes/-forgot-password/forgot-password-form.test.tsxapps/web/src/routes/-forgot-password/run-forgot-password.test.tsapps/web/src/routes/-game/nav-rail.test.tsxapps/web/src/routes/-game/playing-elsewhere-notice.test.tsxapps/web/src/routes/-game/satellite-stack.test.tsxapps/web/src/routes/-login-force-logout/run-force-logout.test.tsapps/web/src/routes/-login/login-form.test.tsxapps/web/src/routes/-login/run-login.test.tsapps/web/src/routes/-onboarding/onboarding-form.test.tsxapps/web/src/routes/-onboarding/run-onboarding.test.tsapps/web/src/routes/-reset-password/reset-password-form.test.tsxapps/web/src/routes/-reset-password/reset-password-handler.test.tsapps/web/src/routes/-signup/run-signup.test.tsapps/web/src/routes/-signup/signup-form.test.tsxapps/web/src/routes/-verify-otp/verify-otp-form.test.tsxapps/web/src/routes/-verify-otp/verify-otp-handler.test.tsapps/web/src/test-utils/factories/create-mock-avatar.test.tsapps/web/src/test-utils/factories/create-mock-user.test.tsapps/web/src/test-utils/render.tsxcontracts/activity/src/activity-contract.test.tscontracts/activity/src/build-checkpoint-hash.test.tscontracts/avatar/src/avatar-data-schema.test.tscontracts/email/src/email-contract.test.tslibs/data/db/package.jsonlibs/data/db/src/create-db.test.tslibs/data/db/src/test-support/resolve-test-db-target.test.tslibs/data/db/test-setup.tslibs/data/sim-registry/src/find-current-sim-version.test.tslibs/data/sim-registry/src/find-current-sim-version.tslibs/service/jobs/src/create-job-queue.test.tslibs/service/product-analytics/package.jsonlibs/service/product-analytics/src/metrics/record-delivery-failure.test.tslibs/service/service-runtime/src/create-service.test.tslibs/service/service-runtime/src/report-unexpected-error.test.tslibs/service/service-runtime/src/start-error-reporting.test.tslibs/service/service-utils/src/utils/create-token-verifier.test.tslibs/testing/client-test-utils/src/orpc/build-contract-mock.test.tslibs/testing/service-test-utils/src/bun/strategies/create-schema-test-db.test.tsscripts/src/utils/require-env-var.test.tsservices/activity/package.jsonservices/activity/src/handlers/advance-activity.test.tsservices/activity/src/handlers/get-latest-activity-progress.test.tsservices/activity/src/handlers/track-activity-progress.test.tsservices/activity/src/metrics/record-terminal-transition.test.tsservices/activity/src/test-utils/factories/create-mock-checkpoint-batch.test.tsservices/avatar/src/handlers/get-avatars.test.tsservices/email/src/build-router.test.tsservices/email/src/create-email-service.test.tsservices/keys/src/parse-roll-key-roots.test.tsservices/keys/src/parse-scope-secret-roots.test.tsservices/replay/src/dispatch/run-replay-segment.test.tsservices/replay/src/test-utils/factories/create-mock-activity-row.test.tsservices/replay/src/test-utils/factories/create-mock-chain-row.test.tsservices/replay/src/test-utils/factories/create-mock-encounter-node.test.tsservices/replay/src/test-utils/factories/create-mock-replay-segment.test.tsservices/replay/src/worker/run-replay-iteration.test.ts
💤 Files with no reviewable changes (3)
- services/activity/package.json
- libs/service/product-analytics/package.json
- services/activity/src/test-utils/factories/create-mock-checkpoint-batch.test.ts
…oldens Seed the owner's activity so the ownership filter is what rejects, and replace two duplicated replay snapshots with an in-test parity comparison and an outcome-kind assertion.
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.
Description
Aligns the test suite with the updated testing skill guideline across four waves, from mechanical matcher fixes through higher-value factory/snapshot/authorisation cleanups.
toEqual→toStrictEqual,describe-free titles, dropped a duplicate test.toThrowWithMessage/onTestFinished; hand-rolled env restore →updateEnv(neededregisterBunTestCleanup()wired intolibs/data/db/test-setup.ts); ad hoc polling → sharedwaitFor/polled drain, retry-drain timeouts tightened to just above the configured delay.isRedirect/instanceof Responseguards ontorejects.toMatchObject/invariant;waitFor(() => getBy*)→findBy*; the continue-here click now waits on the avatar query cache (the render util exposes itsqueryClient); seeded@msw/datainstead of hand-built responses.createInMemoryMetrics, secrecy tests assert the secret is absent from the message.findCurrentSimVersionbreaks adeployedAttie onengineHash, matchingfind-latest-release's tiebreaker pattern.Testing
bun run typecheckpassesbun run testpassesbun run lintpassesContext
Deferred to follow-up work, per the guideline-alignment plan:
setState→setter conversions (23 sites)result.datasweep (57 sites)libs/data/dbdatabase-creation leak redesigncreate-*-service.test.ts: the factories expose no db handle or dispose hook, so real teardown needs a factory API change (the email variant'squeue.stop()is the one real disposal available today)setTimeoutsites:create-worker-demux.test.ts:65waits out an eviction sweep interval with no pollable signal;create-worker-runtime.test.ts:153andstart-writer-election.test.ts:77prove an absence after a fixed window