Speed up server tests: skip DB reset for clean tests, cache server node_modules in CI - #2818
Conversation
…de_modules in CI Two independent optimizations of the server test pipeline: 1. Conditional database reset in the global beforeEach. Most of the ~5500 tests never write to the database, yet each one paid the snapshot copy (~13.5s of SQLite work per run) and a DuckDB DELETE (~9s per run). The reset is now skipped when the database is provably untouched, detected through two cheap freshness markers: total_changes() for writes made through the Sequelize connection, and PRAGMA data_version for writes made by any other connection or process (e.g. the sqlite3 CLI used by gateway.restoreBackup). The DuckDB DELETE is likewise replaced by a count check when the states table is already empty. Measured locally: 84% of tests skip the SQLite reset, 98% skip the DuckDB delete, full suite goes from 138s to 127s with strictly identical test results. 2. node_modules cache for the "Server test" and "Server lint" jobs, using the same pattern (and rationale) as the existing Cypress job cache: keyed on the server lockfiles and the node version, covering the service-level node_modules installed by the server postinstall. Saves ~40s of npm ci per job on warm cache.
github-actions
Bot
added
area:server
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe workflow adds keyed dependency caching to server lint and test jobs. The database test helper tracks SQLite freshness, skips unchanged restores, and conditionally clears DuckDB state. ChangesWorkflow dependency caching
Database reset tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Deploying gladys-plus with
|
| Latest commit: |
731cb65
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e823a2fa.gladys-plus.pages.dev |
| Branch Preview URL: | https://claude-slow-unit-tests-ci-4j.gladys-plus.pages.dev |
|
🐳 A Docker image has been built for this branch and pushed to the GitHub Container Registry. You can test this pull request (AMD64 only) by pulling the image below: For example, run it with: sudo docker run -d \
--log-driver json-file \
--log-opt max-size=10m \
--cgroupns=host \
--restart=always \
--privileged \
--network=host \
--name gladys-claude-slow-unit-tests-ci-4j4nks \
-e NODE_ENV=production \
-e SERVER_PORT=80 \
-e TZ=Europe/Paris \
-e SQLITE_FILE_PATH=/var/lib/gladysassistant/gladys-production.db \
-v /var/run/docker.sock:/var/run/docker.sock \
-v /var/lib/gladysassistant:/var/lib/gladysassistant \
-v /dev:/dev \
-v /run/udev:/run/udev:ro \
ghcr.io/gladysassistant/gladys-preview:claude-slow-unit-tests-ci-4j4nksThis comment and the image are automatically updated on every new commit pushed to this pull request. Need an ARM64 image (Raspberry Pi, Apple Silicon, …)? Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review verdict: approve
Solid, low-risk follow-up to the test-suite speedups in #2801 / #2802 / #2813. Scope is limited to the test DB helper and the PR CI workflow — production Gladys code is untouched.
Conditional SQLite / DuckDB reset (server/test/helpers/db.test.js)
The freshness-marker approach is well reasoned:
total_changes()covers DML on the Sequelize singleton connection, including statements later rolled back (so a dirty DB cannot look clean).PRAGMA data_versioncovers writes from other connections/processes (e.g. thesqlite3CLI path used bygateway.restoreBackup).- After a real reset, markers are re-read so the next clean test can skip again.
- DuckDB stays independent:
count(*)then conditionalDELETE, both on the serialized write queue.
Author reports identical pass/fail vs the previous always-reset path with a large skip rate — that is the right bar for this change.
CI node_modules cache (.github/workflows/docker-pr-build.yml)
Matches the Cypress pattern that already landed in #2813: key includes OS + Node version + server/**/package-lock.json, paths include server/services/*/node_modules, install (and global typescript / node-gyp) skipped on hit. Shared key between Server lint and Server test is fine; one job populating the cache is enough.
Labels
- Not
risk:high— test helpers + CI only. - Not
needs:human-review— same class as #2801 / #2802 / #2813; no product/philosophy tradeoff. - Device categories/types: N/A.
needs:cursor-reviewwas not present.
Soft notes (non-blocking)
- Same-connection non-DML mutations (
ALTER TABLE, sessionPRAGMAs, etc.) do not move either marker. The current suite does not appear to rely on that, and a false “dirty” is always safer than a false “clean”, but worth remembering if someone later adds mid-suite schema tests against the shared DB. docker-master-test.ymlstill always runsnpm cifor server jobs — intentional asymmetry for this PR is fine; consider mirroring the cache there in a follow-up for master CI parity.- Server lint keeps
permissions: contents: read, which may prevent that job from saving the cache; Server test can still populate it.
Sent by Cursor Automation: Automatic PR review
| // node-sqlite3 all() only runs the first statement of a script: two queries. | ||
| const totalChanges = await sqliteAll('SELECT total_changes() AS tc'); | ||
| const dataVersion = await sqliteAll('PRAGMA data_version'); | ||
| return JSON.stringify([totalChanges, dataVersion]); |
There was a problem hiding this comment.
Non-blocking: these two markers correctly cover DML on this connection and commits from other connections/processes. They will not move for same-connection non-DML work (e.g. ALTER TABLE, session-only PRAGMAs). That looks fine for today’s suite (no mid-suite schema mutations on the shared DB), and failing “dirty” is the safe direction — just something to keep in mind if schema-oriented tests are added later against this helper.
| path: | | ||
| server/node_modules | ||
| server/services/*/node_modules | ||
| key: server-deps-v1-${{ runner.os }}-node${{ steps.setup-node.outputs.node-version }}-${{ hashFiles('server/**/package-lock.json') }} |
There was a problem hiding this comment.
Non-blocking: same solid key/path pattern as the Cypress cache in #2813 (Node ABI + service-level node_modules). Soft follow-ups only: (1) Server lint’s permissions: contents: read may block saving this cache from that job — Server test can still write it; (2) docker-master-test.yml is still uncached for server installs if you want master CI parity later.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2818 +/- ##
=======================================
Coverage 99.49% 99.49%
=======================================
Files 1212 1212
Lines 85022 85022
=======================================
Hits 84589 84589
Misses 433 433 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 @.github/workflows/docker-pr-build.yml:
- Around line 77-82: Update the workflow steps around “Install Global NPM
Packages” so TypeScript and node-gyp are available even when
steps.deps-cache.outputs.cache-hit is true. Remove or adjust the cache-hit
condition for installs needed by server CI commands, including the corresponding
install block around lines 130-135, or ensure typescript is included in the
server dependencies/cache and restored consistently.
In `@server/test/helpers/db.test.js`:
- Around line 103-126: Add direct tests for the reset helper covering matching
markers without SQLite reset, marker changes after a write triggering
restoration, failed sqliteExec rollback recovery, and DuckDB cleanup when the
count is zero versus nonzero. Exercise the relevant readFreshnessMarkers,
sqliteExec, and duckDbWriteConnectionAllAsync paths, asserting both skipped and
executed operations plus preserved error propagation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c362540-e02f-4a1b-8556-759e67f69b47
📒 Files selected for processing (2)
.github/workflows/docker-pr-build.ymlserver/test/helpers/db.test.js
Covers the four outcomes: skip on a clean database, restore after a write through the Sequelize connection (total_changes marker), restore after a write from a foreign connection (data_version marker), and the DuckDB clear when states are present.
The clean-database test could not distinguish a skipped reset from an unnecessary one (both leave identical data). resetDb now counts the resets it actually performs (resetDbStats), and the tests assert the counters: unchanged on the clean path, incremented on each dirty path.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/test/helpers/dbReset.test.js (2)
21-44: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftAdd or verify coverage for the SQLite restore failure path.
This file covers successful restores only. The implementation in
server/test/helpers/db.test.jshas a failure branch that runsROLLBACK; PRAGMA foreign_keys = ONand rethrows. If no existing test covers that branch, inject a reset failure and assert that the original error propagates and a later query remains usable.As per coding guidelines, server tests must cover added error paths.
🤖 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 `@server/test/helpers/dbReset.test.js` around lines 21 - 44, Add coverage in the reset database tests for the failure branch that executes ROLLBACK and restores foreign-key enforcement. Inject a reset failure, assert the original error is rethrown, then perform a subsequent query to verify the database connection remains usable.Source: Coding guidelines
12-19: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the clean path skips the reset operations.
House.count()can remain unchanged whenresetDb()still restores SQLite or executes the DuckDBDELETE. Add a spy or narrow test seam around those operations, then assert that a clean call executes neither operation. Repeat the check after a dirty reset to verify thatcleanMarkersis refreshed.As per coding guidelines, server tests must cover added branches and helper behavior.
🤖 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 `@server/test/helpers/dbReset.test.js` around lines 12 - 19, Strengthen the clean-path test around resetDb by spying on or adding a narrow seam for the SQLite restore and DuckDB DELETE operations, then assert a clean call executes neither operation. After creating a dirty state and calling resetDb, verify the reset occurs and cleanMarkers is refreshed so a subsequent clean call again skips both operations.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 `@server/test/helpers/dbReset.test.js`:
- Around line 37-39: Update the external SQLite connection flow around the
promisified external.run call to close external in a finally block, ensuring
external.close executes whether the UPDATE succeeds or rejects.
---
Nitpick comments:
In `@server/test/helpers/dbReset.test.js`:
- Around line 21-44: Add coverage in the reset database tests for the failure
branch that executes ROLLBACK and restores foreign-key enforcement. Inject a
reset failure, assert the original error is rethrown, then perform a subsequent
query to verify the database connection remains usable.
- Around line 12-19: Strengthen the clean-path test around resetDb by spying on
or adding a narrow seam for the SQLite restore and DuckDB DELETE operations,
then assert a clean call executes neither operation. After creating a dirty
state and calling resetDb, verify the reset occurs and cleanMarkers is refreshed
so a subsequent clean call again skips both operations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 84df9752-998b-4d76-9bd3-a3b91c5e81d3
📒 Files selected for processing (1)
server/test/helpers/dbReset.test.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/test/helpers/dbReset.test.js (1)
28-28: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse a bound parameter for the
UPDATE.
house.idis local test data, so this is not an attacker-controlled application path. Avoid raw interpolation. Parameter binding keeps the test safe if fixture values change and removes the static-analysis finding.Suggested change
- await db.sequelize.query(`UPDATE t_house SET name = 'dirty' WHERE id = '${house.id}'`); + await db.sequelize.query( + 'UPDATE t_house SET name = :name WHERE id = :id', + { replacements: { name: 'dirty', id: house.id } }, + );Confirm the
replacementssyntax against Sequelize 6.26.0 and existing repository patterns.🤖 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 `@server/test/helpers/dbReset.test.js` at line 28, Update the UPDATE query in the database reset test to use a Sequelize bound replacement for house.id instead of interpolating it into the SQL string. Follow the replacements syntax supported by Sequelize 6.26.0 and match existing repository patterns, while preserving the current name and row-update behavior.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@server/test/helpers/dbReset.test.js`:
- Line 28: Update the UPDATE query in the database reset test to use a Sequelize
bound replacement for house.id instead of interpolating it into the SQL string.
Follow the replacements syntax supported by Sequelize 6.26.0 and match existing
repository patterns, while preserving the current name and row-update behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d25fc74-9caa-4762-8083-c0935f453ef0
📒 Files selected for processing (2)
server/test/helpers/db.test.jsserver/test/helpers/dbReset.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- server/test/helpers/db.test.js
Third PR of the server-test speedup series (after #2818 and #2820). With the suite dominated by real test code, mocha's --parallel mode spreads the files over one worker process per core (4 on the CI runners). Measured locally on a 4-vCPU machine: full suite 73s -> ~37s, with the same pass/fail list as the serial run. How the pieces fit: - test/bootstrap.test.js becomes test/hooks.js, a mocha root hook plugin loaded through --require: hooks declared inside a test file do not apply across files in parallel mode. Each worker boots its own Gladys (memoized: mocha calls beforeAll once per FILE, workers are reused for many files), and the API server listens on port 0 — no test depends on the actual port, supertest wraps the app object. - setup-env.js derives a per-process SQLITE_FILE_PATH (the DuckDB file and the reset snapshot both follow automatically, being derived from that path), and removes the worker's database files at exit. - nock is required first in hooks.js: it patches the core http module at load time, and each service embeds its own axios whose follow-redirects captures http.request when required — the patch must come first regardless of the file order a worker happens to load. - The eslint devDependencies allowance is widened to test helper files (test/**/*.js), since hooks.js is no longer named *.test.js. - npm run test-serial keeps the sequential mode available for debugging.
2 participants
Description
Two independent optimizations of the server test pipeline (first of a series of three PRs following a profiling session of the ~5 500-test server suite):
1. Conditional database reset in the global
beforeEach(server/test/helpers/db.test.js)Profiling showed the global reset costs ~22.5s per run (~13.5s SQLite snapshot copy + ~9s DuckDB
DELETE), even though most tests never write to the database. The reset is now skipped when the database is provably untouched, using two cheap freshness markers read before each copy:SELECT total_changes()— counts every row written through the Sequelize connection (including changes later rolled back, so a dirty database can never look clean);PRAGMA data_version— increments when the file is modified by any other connection or process, which coversgateway.restoreBackupshelling out to thesqlite3CLI.The DuckDB
DELETEis likewise replaced by acount(*)check when the states table is already empty (both statements go through the serialized write queue, so they cannot race a pending write).Measured locally: 84% of tests skip the SQLite reset, 98% skip the DuckDB delete; the full suite goes from 138s to 127s with strictly identical results (same passes, same failures, run-to-run).
2.
node_modulescache for the "Server test" and "Server lint" jobsSame pattern and rationale as the cache added to the Cypress job in #2813: keyed on the server lockfiles and the node version, covering the per-service
node_modulesinstalled by the server postinstall. On a warm cache both jobs skipnpm ci(~40s) and the global typescript/node-gyp install.Expected effect on the "Server test" job: ~3m24 → ~2m30 (cold cache) / ~2m10 (warm cache). Two follow-up PRs (per-file sinon sandboxes, then mocha parallel mode) will address the remaining test-time bottleneck.
Checklist
cd server && npm run coverage(changed files are test helpers and CI config, excluded from coverage)npm run eslint,npm run prettier)Generated by Claude Code
Summary by CodeRabbit
Tests
Chores