fix(db): give the last two embedded-Postgres migration tests a timeout (#11313)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The `@paperclipai/db` package owns the database schema and its migrations > - Some migration tests start an embedded Postgres server and replay a migration against it > - An embedded Postgres server needs 7 to 12 seconds to start on a CI runner > - Vitest stops a test after 5 seconds unless the test sets its own timeout > - Two of these tests do not set a timeout, so they fail on CI before they assert anything > - This pull request gives both tests a 30 second timeout > - The benefit is that unrelated pull requests stop failing on a test they did not change ## Linked Issues or Issue Description No public issue exists for this. The problem follows. **What happened?** The test `packages/db/src/company-secret-proposals-migration.test.ts` fails on CI. The error is `Test timed out in 5000ms`. The test never reaches its assertions. The suite reports `1 failed | 104 passed`. The failure is not caused by the branch under test. It appeared on three different branches in a few hours: | Run | Head | Failing jobs | | --- | --- | --- | | 31630781317 | `95622fa3` | `General tests (workspaces-b)`, `verify`, `e2e shard (2/3)`, `e2e` | | 31652020976 | `feba90c9` | `General tests (workspaces-b)`, `verify`, `e2e shard (3/3)`, `e2e` | | 31651467721 | `f2115207` | `General tests (workspaces-a (1/2))`, `verify` | The `verify` job reads the result of the general tests. One timeout therefore turns into two red checks. A reviewer sees two failures and reads them as a regression. **Expected behavior** The test starts an embedded Postgres server, replays the migration, and asserts the schema. It must pass on a normal CI runner. **Steps to reproduce** 1. Open any pull request against `master`. 2. Wait for the job `General tests (workspaces-b)`. 3. Read the failure. The test times out after 5000 ms. The failure needs a slow runner. A fast development machine starts embedded Postgres in less than 5 seconds, so the test passes there. **Paperclip version or commit** `master` at `a09d7dcc0`. **Deployment mode** CI only. GitHub Actions, `ubuntu24` runner image. ## What Changed - `packages/db/src/company-secret-proposals-migration.test.ts` — the test now uses a 30 second timeout. The migration suites in this package already use 20 to 60 seconds. 30 seconds is the most common value. - `packages/db/src/status-card-migrations.test.ts` — the same change. This test has the same defect. It does not fail yet because it replays fewer statements. A fix to only one test moves the problem instead of removing it. - Both tests get a comment. The comment tells the next author why the 5 second default is too short. These two tests were the only embedded-Postgres migration tests in the package without a timeout. ## Verification - Run `pnpm vitest run src/company-secret-proposals-migration.test.ts src/status-card-migrations.test.ts` in `packages/db`. Both tests pass. - These suites skip themselves when the Postgres binaries are absent. A pass alone therefore proves nothing. Run the command with `--reporter=verbose`. The output contains Postgres `NOTICE` messages, for example `relation "status_cards" already exists, skipping`. These messages prove the tests ran real SQL. - Run the same command with `--testTimeout=1`. Both tests still pass. This proves the per-test timeout overrides the global timeout. Before this change, the same command fails immediately. - All CI jobs on this pull request pass. The job `General tests (workspaces-b)` passes. This job failed on the three runs listed above. Not done: no attempt to reproduce the timeout on a development machine. A fast machine starts embedded Postgres in less than 5 seconds, so the failure does not occur there. ## Risks Low risk. The change adds two timeout arguments to tests. It changes no source code, no schema, and no dependency. A longer timeout cannot hide a regression here. The tests assert the same conditions as before. A migration that truly hangs now fails after 30 seconds. Before, it failed after 5 seconds with a message that pointed at the wrong cause. The `e2e` failures on the runs above have a different cause. The spec `mcp-user-stories.spec.ts › US-9` fails with `502 — fetch failed` and `fetch failed: bad port`. These errors come from MCP tool-connection health checks. The failures hit different shards on different runs. This pull request does not change that behavior. `e2e shard (2/3)` passes here, which supports the view that those failures are unstable infrastructure. To revert, remove the two timeout arguments. ## Model Used Claude Opus 5 (`claude-opus-5`), through Claude Code. Extended thinking enabled. Tool use enabled: file read and edit, shell command execution for the local test runs, and the GitHub CLI to read the failing CI logs. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
parent
a09d7dcc06
commit
1e07d5b9aa
|
|
@ -24,6 +24,11 @@ if (!embeddedPostgresSupport.supported) {
|
|||
describeEmbeddedPostgres("company secret proposals migration", () => {
|
||||
afterEach(async () => Promise.all(cleanups.splice(0).map((cleanup) => cleanup())));
|
||||
|
||||
// Starting an embedded Postgres and replaying a migration against it does not
|
||||
// fit vitest's 5s default: the neighbouring replay suites measure 7-12s on
|
||||
// CI. Every other embedded-Postgres migration test in this package carries an
|
||||
// explicit timeout for that reason; this one did not, so it failed on any
|
||||
// runner that was not unusually fast.
|
||||
it("can be reapplied after its migration journal entry is removed", async () => {
|
||||
const database = await startEmbeddedPostgresTestDatabase("paperclip-secret-proposals-migration-");
|
||||
cleanups.push(database.cleanup);
|
||||
|
|
@ -40,5 +45,5 @@ describeEmbeddedPostgres("company secret proposals migration", () => {
|
|||
(SELECT count(*)::int FROM pg_indexes WHERE tablename = 'company_secret_proposals') AS indexes
|
||||
`;
|
||||
expect(result).toEqual({ constraints: 13, indexes: 5 });
|
||||
});
|
||||
}, 30_000);
|
||||
});
|
||||
|
|
|
|||
|
|
@ -22,6 +22,10 @@ describeEmbeddedPostgres("status card migrations", () => {
|
|||
await Promise.all(cleanups.splice(0).map((cleanup) => cleanup()));
|
||||
});
|
||||
|
||||
// Same reason as every other embedded-Postgres migration test here: starting
|
||||
// the server and replaying migrations against it does not fit vitest's 5s
|
||||
// default. This suite has not tripped yet only because it replays fewer
|
||||
// statements than its neighbours — it is the same latent failure.
|
||||
it("can be reapplied after the schema already exists", async () => {
|
||||
const database = await startEmbeddedPostgresTestDatabase("paperclip-status-card-migrations-");
|
||||
cleanups.push(database.cleanup);
|
||||
|
|
@ -35,5 +39,5 @@ describeEmbeddedPostgres("status card migrations", () => {
|
|||
);
|
||||
await sql.unsafe(migrationSql);
|
||||
}
|
||||
});
|
||||
}, 30_000);
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue