Add docker-compose integration test harness - #104
Conversation
Miracle656
left a comment
There was a problem hiding this comment.
Thanks for this. A few things to resolve before it can land:
-
The Integration tests job is failing —
Cannot find module '@rolldown/binding-linux-x64-gnu'(the npm optional-deps bug, npm/cli#4828). The committedpackage-lock.jsondoesn't include the Linux native binding Vitest needs in CI. Regenerating the lockfile so the platform-optional deps are captured (or pinning the binding) should fix it. -
Overlap with the existing harness.
mainalready has a docker-compose integration setup (from #65). Please clarify what this adds on top of it — if it's an improvement, ideally extend the existing harness rather than adding a parallelvitest.integration.config.tspath; if it supersedes #65's, say so explicitly. -
Unrelated change:
src/__tests__/opa.test.tsand thesrc/index.tsedit don't look related to an integration harness — if they belong to a different concern, please split them out.
Once the integration job is green and the relationship to the existing harness is clear, I'll re-review. Thanks!
|
Merge conflict resolved |
|
A few things to sort before this can land:
The |
Of this PR's three changes, only SKIP_INDEXER survives review: - SKIP_INDEXER=true starts the API without the indexer loop. Kept and documented: the integration harness exercises HTTP routes against a seeded database, and letting the ingest loop run alongside would race it on the same tables and require live RPC the harness has no reason to depend on. - The vitest.integration.config.ts change narrowed include from tests/integration/**/*.test.ts to tests/integration/api.test.ts. That silently stops running e2e, migrations, reorg, reorg-rollover and ws — five of the six integration suites — while still reporting green. Not applied. - The lockfile bumped vitest from ^3.2.4 to ^4.1.7, a major version, inside a PR about a test harness. Not applied; a runner upgrade should be its own change so a behaviour difference has somewhere to be attributed. The docker-compose harness the title describes already exists on main (docker-compose.test.yml plus tests/integration/), so there was nothing left to add there.
Miracle656
left a comment
There was a problem hiding this comment.
Approved and merging a reduced version (292daba), since the wave has closed. I want to be straightforward about what I kept and what I dropped.
Kept: SKIP_INDEXER. This is a good idea and it is now documented. The integration harness exercises HTTP routes against a seeded database, and letting the ingest loop run alongside would have it racing the tests on the same tables — plus requiring live RPC the harness has no reason to depend on. Starting the API without the indexer is exactly the right escape hatch.
Dropped: the vitest.integration.config.ts change. This narrowed:
- include: ["tests/integration/**/*.test.ts"],
+ include: ["tests/integration/api.test.ts"],tests/integration/ holds six suites — api, e2e, migrations, reorg, reorg-rollover, ws. That change stops running five of them, and the run still reports green, so nothing would ever have told us. Reorg and migration coverage is exactly the coverage you least want to lose quietly.
I suspect this was a local narrowing to iterate faster that got committed by accident. Easy to do; worth catching.
Dropped: the lockfile's vitest ^3.2.4 → ^4.1.7. A major version bump of the test runner, arriving inside a PR about a test harness. If a suite starts behaving differently afterwards, nobody will think to look at the diff titled "integration test harness" — a runner upgrade needs to be its own change so a behaviour difference has somewhere to be attributed.
On the title: the docker-compose harness already exists on main — docker-compose.test.yml, docker-compose.yml and the whole tests/integration/ directory — so there was nothing to add there. That landed while this PR was open.
Verified: tsc --noEmit clean, full suite 389 passed.
The one-line idea in here was worth keeping, so it is in.
# Conflicts: # .env.example
Miracle656
left a comment
There was a problem hiding this comment.
Approved and merging a reduced version (292daba), since the wave has closed. I want to be straightforward about what I kept and what I dropped.
Kept: SKIP_INDEXER. This is a good idea and it is now documented. The integration harness exercises HTTP routes against a seeded database, and letting the ingest loop run alongside would have it racing the tests on the same tables — plus requiring live RPC the harness has no reason to depend on. Starting the API without the indexer is exactly the right escape hatch.
Dropped: the vitest.integration.config.ts change. This narrowed:
- include: ["tests/integration/**/*.test.ts"],
+ include: ["tests/integration/api.test.ts"],tests/integration/ holds six suites — api, e2e, migrations, reorg, reorg-rollover, ws. That change stops running five of them, and the run still reports green, so nothing would ever have told us. Reorg and migration coverage is exactly the coverage you least want to lose quietly.
I suspect this was a local narrowing to iterate faster that got committed by accident. Easy to do; worth catching.
Dropped: the lockfile's vitest ^3.2.4 → ^4.1.7. A major version bump of the test runner, arriving inside a PR about a test harness. If a suite starts behaving differently afterwards, nobody will think to look at the diff titled "integration test harness" — a runner upgrade needs to be its own change so a behaviour difference has somewhere to be attributed.
On the title: the docker-compose harness already exists on main — docker-compose.test.yml, docker-compose.yml and the whole tests/integration/ directory — so there was nothing to add there. That landed while this PR was open.
Verified: tsc --noEmit clean, full suite 389 passed.
The one-line idea in here was worth keeping, so it is in.
Summary
Added a docker-compose-based integration test harness for Wraith that runs against a real Postgres instance, seeds representative fixtures, and exercises live API endpoints through Vitest. Also wired the suite into CI so it runs on every pull request.
Related issue
Type of change
Checklist
npx tsc --noEmitpassesnpm run buildpasses