fix: resolve flaky tests caused by shared state between parallel test files#1957
Conversation
… files Tests running in parallel via bun test share a single database, causing race conditions when RPC tests create temporary data in workspace 2 while V1 tests assert workspace 2 is empty. Two fixes applied: 1. Move feature-specific permission checks (password-protection, email-domain-protection) before the page count check in the page creation route. This ensures the correct 402 error message is returned regardless of how many pages exist for the workspace. 2. Switch "return empty" tests to use workspace 3, which is not used by any RPC test for cross-workspace validation, eliminating the race condition with concurrent test files. Closes openstatusHQ#1928
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Instead of using a new workspace ID, it would be better to create some data in a before method. |
There was a problem hiding this comment.
Pull request overview
Adjusts server route behavior and test fixtures to better align status page creation limits with access-feature validations, and ensures “empty list” route tests use a workspace expected to have no seeded data.
Changes:
- Reordered the status page count/limit check in the
POST /v1/pagehandler to occur after custom-domain and access-type limit validations. - Updated several
get_allroute tests to usex-openstatus-key: "3"for the “empty” workspace scenario.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/server/src/routes/v1/pages/post.ts | Moves the status-page limit enforcement check later in the request validation flow. |
| apps/server/src/routes/v1/pages/get_all.test.ts | Uses workspace key 3 for the “empty pages” assertion. |
| apps/server/src/routes/v1/statusReports/get_all.test.ts | Uses workspace key 3 for the “empty status reports” assertion. |
| apps/server/src/routes/v1/incidents/get_all.test.ts | Uses workspace key 3 for the “empty incidents” assertion. |
| apps/server/src/routes/v1/notifications/get_all.test.ts | Uses workspace key 3 for the “empty notifications” assertion. |
| apps/server/src/routes/v1/maintenances/get_all.test.ts | Uses workspace key 3 for the “empty maintenances” assertion. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…orkspace IDs Refactor get_all test files to create their own test data in beforeAll and clean up in afterAll, following the pattern from monitor.test.ts. Each file uses a unique TEST_PREFIX for isolation during parallel runs. Use workspace 3 for empty tests since other tests write to workspace 2.
johnib
force-pushed
the
fix/flaky-tests-1928-upstream
branch
from
a986ec8 to
f3e92b8
Compare
|
@johnib is attempting to deploy a commit to the OpenStatus Team on Vercel. A member of the Team first needs to authorize it. |
@thibaultleouay I updated the PR. Did some mess apparently in this PR. I used the CI to validate the tests pass over multiple runs, meant to have that on my own fork and then update this PR when all looked good to me. Apparently it all synced over here. Anyway - let me know if this change satisfies your request. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Summary
Fixes #1928
The CI test suite has been intermittently failing (~37% failure rate overall, ~10% on main) due to test isolation issues when
bun testruns test files in parallel against a shared database.This PR fixes two categories of flaky tests:
1. "Return empty" tests using a dedicated workspace
Five
get_all.test.tsfiles had "return empty" test cases querying workspace 2 and expecting zero records. Since other tests transiently create/delete records, there's a race condition. The fix moves these tests to workspace 3, which is seeded with no data for these resource types (incidents, maintenances, notifications, pages, status reports).Files changed:
incidents/get_all.test.ts,maintenances/get_all.test.ts,notifications/get_all.test.ts,pages/get_all.test.ts,statusReports/get_all.test.ts2. Validation order in
pages/post.tsThe page creation route checked the page count limit before feature-specific permission checks (password-protection, email-domain-protection). When workspace 2 had transient pages from parallel tests, the count check fired first, returning a generic "Upgrade for more status pages" 402 instead of the specific feature error the test expected. Moving the count check after the feature checks ensures correct error messages regardless of concurrent state.
Verification
Ran CI 20 consecutive times on this branch — all 20 passed. This is against a baseline where upstream main fails ~10% of the time on the same tests.
Test plan