Repository navigation
Run all DB-gated web tests in CI - #5683
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR consolidates database behavior test execution by introducing a new gated runner script that discovers and validates DB-behavior tests, wiring it through npm scripts, local test workflows, CI pipelines, and adding regression guards to ensure the integration remains correct. ChangesDB Behavior Test Runner Consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 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 |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR replaces four hard-coded
Confidence Score: 4/5Safe to merge — the discovery runner correctly wires up DB-gated test expansion in both CI and local flows, and the guard prevents future regressions to hard-coded subsets. The runner logic is sound and the CI integration is correct. Three observations in the runner are worth a follow-up: temp files created by mktemp have no trap for cleanup on interruption, the zero_test_files check exits before reporting simultaneously failing files, and the Bun summary-line regex could silently miss zero-test files if Bun changes its output format. web/scripts/run-db-behavior-tests.sh deserves a second look on the error-reporting order and the Bun output format assumption. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CI[CI: web-db-migrations job] -->|bun run test:db:behavior| PKG[web/package.json\ntest:db:behavior]
LOCAL[Local: bun db:test] -->|bash db-local.sh test| DL[web/scripts/db-local.sh\ntest case]
PKG -->|bash scripts/run-db-behavior-tests.sh| RUNNER
DL -->|bash run-db-behavior-tests.sh| RUNNER
RUNNER[run-db-behavior-tests.sh]
RUNNER -->|find tests -name *.test.ts| FIND[find all *.test.ts files]
FIND -->|grep process.env.CMUX_DB_TEST| GATE{CMUX_DB_TEST-gated?}
GATE -->|No| SKIP[Skip file]
GATE -->|Yes| RUN[bun test file\nwith CMUX_DB_TEST=1]
RUN --> CHECK{Check output}
CHECK -->|zero tests run| ZERO_FAIL[Exit: zero tests]
CHECK -->|skips detected| SKIP_FAIL[Exit: skips remain]
CHECK -->|non-zero exit| FAIL[Exit: tests failed]
CHECK -->|all pass| NEXT[Next file]
NEXT --> FIND
GUARD[test_ci_self_hosted_guard.sh\ncheck_web_db_behavior_tests] -->|verifies| RUNNER
GUARD -->|verifies| PKG
GUARD -->|verifies| CI
Reviews (1): Last reviewed commit: "Run all DB-gated web tests in CI" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| output_file="$(mktemp /tmp/cmux-db-behavior-test.XXXXXX.log)" | ||
| set +e | ||
| bun test "$test_file" 2>&1 | tee "$output_file" | ||
| test_status=${PIPESTATUS[0]} | ||
| set -e | ||
|
|
||
| if ! grep -Eq 'Ran [1-9][0-9]* tests? across [1-9][0-9]* files?' "$output_file"; then | ||
| zero_test_files+=("$test_file") | ||
| fi | ||
| if grep -Eq '^\(skip\) |^[[:space:]]*[1-9][0-9]* skips?$' "$output_file"; then | ||
| skipped_test_files+=("$test_file") | ||
| fi | ||
| rm -f "$output_file" |
There was a problem hiding this comment.
mktemp is called inside the loop but there is no trap to remove $output_file on SIGINT, SIGTERM, or set -e exit. If the script is killed mid-iteration (e.g., by a CI timeout), the /tmp/cmux-db-behavior-test.*.log file from the current iteration stays on disk. Adding a trap 'rm -f "$output_file"' EXIT immediately after the mktemp call ensures cleanup on any exit path.
| if ! grep -Eq 'Ran [1-9][0-9]* tests? across [1-9][0-9]* files?' "$output_file"; then | ||
| zero_test_files+=("$test_file") | ||
| fi | ||
| if grep -Eq '^\(skip\) |^[[:space:]]*[1-9][0-9]* skips?$' "$output_file"; then | ||
| skipped_test_files+=("$test_file") | ||
| fi | ||
| rm -f "$output_file" | ||
|
|
||
| if [[ "$test_status" -ne 0 ]]; then | ||
| failed_files+=("$test_file") | ||
| fi | ||
| done | ||
|
|
||
| if [[ "${#zero_test_files[@]}" -gt 0 ]]; then | ||
| printf '\n%s DB behavior test file(s) executed zero tests:\n' "${#zero_test_files[@]}" >&2 | ||
| printf ' %s\n' "${zero_test_files[@]}" >&2 | ||
| exit 1 |
There was a problem hiding this comment.
zero_test_files check fires before failed_files, hiding crash details
When bun test crashes (e.g., an import error) it exits non-zero AND produces no "Ran N tests across M files" line. The file lands in both zero_test_files and failed_files, but the zero_test_files check fires first and calls exit 1 — so the final error message says "executed zero tests" rather than "failed", and other files that genuinely failed (with tests running) are never listed. Checking or printing failed_files before exiting for zero_test_files would give a more complete picture in a single CI run.
| if ! grep -Eq 'Ran [1-9][0-9]* tests? across [1-9][0-9]* files?' "$output_file"; then | ||
| zero_test_files+=("$test_file") | ||
| fi | ||
| if grep -Eq '^\(skip\) |^[[:space:]]*[1-9][0-9]* skips?$' "$output_file"; then |
There was a problem hiding this comment.
The pattern 'Ran [1-9][0-9]* tests? across [1-9][0-9]* files?' matches Bun's current summary output, but if Bun changes the phrasing (e.g., to "Ran N tests in N files" or omits the line on empty suites), any test file that actually ran zero tests would silently pass this check. The PR's guard prevents adding new gated files that skip, but a format drift here would let a file with genuinely zero tests slip through without a CI failure.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@web/scripts/run-db-behavior-tests.sh`:
- Around line 7-10: Update the error message emitted by the conditional that
checks [[ -z "${DIRECT_DATABASE_URL:-${DATABASE_URL:-}}" ]] so it clearly states
that at least one of the environment variables must be provided; replace the
current echo ("DATABASE_URL or DIRECT_DATABASE_URL is required for DB behavior
tests") with a clearer message such as "At least one of DATABASE_URL or
DIRECT_DATABASE_URL must be set for DB behavior tests" and ensure the message
references the environment variable names DIRECT_DATABASE_URL and DATABASE_URL
used in the conditional.
- Around line 32-44: The temporary log file created into variable output_file
via mktemp in run-db-behavior-tests.sh isn’t removed if the script is
interrupted; add a shell trap that unconditionally removes "$output_file" on
EXIT and on common signals (INT, TERM) so cleanup runs for both normal and early
termination, and ensure the trap is set immediately after output_file is created
(before running bun test) and that rm -f "$output_file" remains for the normal
path; reference the output_file variable and the mktemp call when adding the
trap.
🪄 Autofix (Beta)
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
Run ID: f47c9277-2ee9-400c-8dea-b6f1500ebab6
📒 Files selected for processing (5)
.github/workflows/ci.ymltests/test_ci_self_hosted_guard.shweb/package.jsonweb/scripts/db-local.shweb/scripts/run-db-behavior-tests.sh
| if [[ -z "${DIRECT_DATABASE_URL:-${DATABASE_URL:-}}" ]]; then | ||
| echo "DATABASE_URL or DIRECT_DATABASE_URL is required for DB behavior tests" >&2 | ||
| exit 2 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider clarifying the error message.
The current message "DATABASE_URL or DIRECT_DATABASE_URL is required" is correct, but could be more explicit that at least one of them must be set (not both).
📝 Optional clarity improvement
if [[ -z "${DIRECT_DATABASE_URL:-${DATABASE_URL:-}}" ]]; then
- echo "DATABASE_URL or DIRECT_DATABASE_URL is required for DB behavior tests" >&2
+ echo "At least one of DATABASE_URL or DIRECT_DATABASE_URL is required for DB behavior tests" >&2
exit 2
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -z "${DIRECT_DATABASE_URL:-${DATABASE_URL:-}}" ]]; then | |
| echo "DATABASE_URL or DIRECT_DATABASE_URL is required for DB behavior tests" >&2 | |
| exit 2 | |
| fi | |
| if [[ -z "${DIRECT_DATABASE_URL:-${DATABASE_URL:-}}" ]]; then | |
| echo "At least one of DATABASE_URL or DIRECT_DATABASE_URL is required for DB behavior tests" >&2 | |
| exit 2 | |
| fi |
🤖 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 `@web/scripts/run-db-behavior-tests.sh` around lines 7 - 10, Update the error
message emitted by the conditional that checks [[ -z
"${DIRECT_DATABASE_URL:-${DATABASE_URL:-}}" ]] so it clearly states that at
least one of the environment variables must be provided; replace the current
echo ("DATABASE_URL or DIRECT_DATABASE_URL is required for DB behavior tests")
with a clearer message such as "At least one of DATABASE_URL or
DIRECT_DATABASE_URL must be set for DB behavior tests" and ensure the message
references the environment variable names DIRECT_DATABASE_URL and DATABASE_URL
used in the conditional.
| output_file="$(mktemp /tmp/cmux-db-behavior-test.XXXXXX.log)" | ||
| set +e | ||
| bun test "$test_file" 2>&1 | tee "$output_file" | ||
| test_status=${PIPESTATUS[0]} | ||
| set -e | ||
|
|
||
| if ! grep -Eq 'Ran [1-9][0-9]* tests? across [1-9][0-9]* files?' "$output_file"; then | ||
| zero_test_files+=("$test_file") | ||
| fi | ||
| if grep -Eq '^\(skip\) |^[[:space:]]*[1-9][0-9]* skips?$' "$output_file"; then | ||
| skipped_test_files+=("$test_file") | ||
| fi | ||
| rm -f "$output_file" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add trap for temp file cleanup on script interruption.
The temp file created at line 32 is only cleaned up at line 44 in the normal execution path. If the script is interrupted (Ctrl+C, SIGTERM, or early exit), temp files will accumulate in /tmp.
🔧 Recommended improvement for cleanup resilience
printf 'Running %s DB behavior test file(s) with CMUX_DB_TEST=1\n' "${`#test_files`[@]}"
failed_files=()
zero_test_files=()
skipped_test_files=()
+cleanup_files=()
+trap 'rm -f "${cleanup_files[@]}"' EXIT INT TERM
+
for test_file in "${test_files[@]}"; do
printf '\n==> bun test %s\n' "$test_file"
output_file="$(mktemp /tmp/cmux-db-behavior-test.XXXXXX.log)"
+ cleanup_files+=("$output_file")
set +e
bun test "$test_file" 2>&1 | tee "$output_file"
test_status=${PIPESTATUS[0]}
set -e
if ! grep -Eq 'Ran [1-9][0-9]* tests? across [1-9][0-9]* files?' "$output_file"; then
zero_test_files+=("$test_file")
fi
if grep -Eq '^\(skip\) |^[[:space:]]*[1-9][0-9]* skips?$' "$output_file"; then
skipped_test_files+=("$test_file")
fi
- rm -f "$output_file"
if [[ "$test_status" -ne 0 ]]; then
failed_files+=("$test_file")
fi
done🤖 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 `@web/scripts/run-db-behavior-tests.sh` around lines 32 - 44, The temporary log
file created into variable output_file via mktemp in run-db-behavior-tests.sh
isn’t removed if the script is interrupted; add a shell trap that
unconditionally removes "$output_file" on EXIT and on common signals (INT, TERM)
so cleanup runs for both normal and early termination, and ensure the trap is
set immediately after output_file is created (before running bun test) and that
rm -f "$output_file" remains for the normal path; reference the output_file
variable and the mktemp call when adding the trap.
This branch was successfully deployed
1 participant
Summary
CMUX_DB_TEST-gated test fileweb-db-migrationsCI job and localbun db:testCoverage
Before, CI ran 4 DB-backed web test files in
web-db-migrations. This branch runs all 7CMUX_DB_TEST-gated files currently inweb/tests.This is a small split from the stale/conflicting broader CI branch at #4945.
Verification
bash tests/test_ci_self_hosted_guard.shbash -n web/scripts/run-db-behavior-tests.sh web/scripts/db-local.sh tests/test_ci_self_hosted_guard.shgit diff --checkcd web && bun testcd web && bun db:testDeflake report
Flaky path:
web-db-migrationsCMUX_DB_TESTCoverage preserved:
web-db-migrationsBefore:
web-db-migrationswas 34s on Re-enable FileExplorerStore XCTest in CI #5682After:
bun db:testcompleted in 4.65s after dependencies were installedChange:
CMUX_DB_TEST-gated files and fail if any execute zero tests or still skip underCMUX_DB_TEST=1Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Cursor Bugbot is generating a summary for commit 52b2c6d. Configure here.
Summary by cubic
Run all CMUX_DB_TEST-gated web tests in CI by replacing hard-coded file lists with a discovery runner. Increases coverage in the web-db-migrations job from 4 files to all 7 DB-backed test files and prevents silent skips.
web/scripts/run-db-behavior-tests.shto auto-discover tests gated byprocess.env.CMUX_DB_TESTand run them with CMUX_DB_TEST=1; fails on zero executed tests or if any are skipped.test:db:behaviorinweb/package.jsonand run it in CI viabun run test:db:behavior; also used indb-local.sh.tests/test_ci_self_hosted_guard.shto ensure CI uses the discovery runner with CMUX_DB_TEST=1.Written for commit 52b2c6d. Summary will update on new commits.
Summary by CodeRabbit
Release Notes