test(perf): stop hashing test passwords at production cost (#242) #243

Merged
bermudalamb merged 1 commits from fix/242-bcrypt-cost-in-tests into main 2026-08-30 13:32:15 -05:00
Owner

Makes the integration suite 3.5× faster and removes the load sensitivity that produced the CI failure in #242.

The measurement

Warm run against warm run, with only the constant changed:

Cost factor Integration suite Tests
12 34.5s 263 passed
4 9.8s 263 passed

About 25 seconds off every integration run, on the single runner everything else queues behind.

The first attempt at this measurement was wrong, and it is worth saying so. I compared a cold run at cost 4 against a warm run at cost 12 and got 47.7s vs 34.3s — which said the change made things slower. That difference was ts-jest and Postgres warming up, not the cost factor. Both figures in the table are second runs, and the cost-12 number was taken twice (34.3s, 34.5s) before I believed it.

What was actually wrong

The suite registers ~35 customers and asserts nothing about any of their hashes, yet paid bcrypt cost 12 for every one. bcryptjs is pure JS, so it pays that cost several times over versus a native build, and hashing was most of the suite's wall clock.

On a contended runner that pushed adminInventory.integration.test.ts past its 20s timeout — which then surfaced as a foreign key violation somewhere else entirely: the test timed out, Jest moved on, beforeEach truncated, and the still-in-flight registration wrote a token for a customer that had just been deleted. The FK error was a symptom; chasing it leads nowhere.

Deliberately not configurable

No environment variable. One would be a way to weaken password hashing in production by misconfiguration, and nothing needs to tune this.

The only route to the cheap cost is NODE_ENV=test, which a deployed container would announce anyway by refusing to serve the built frontend — app.ts gates static serving on the same value. A setting that quietly degrades a security property should be unreachable rather than merely warned about, which is the reasoning that already made DEMO_MODE strict.

hashRoundsFor is pure and separately tested because the failure it guards against is silent. Only the exact string 'test' earns the cheap cost; an unset NODE_ENV gets the strong one, so the dangerous direction has to be asked for explicitly. Both constants are pinned by assertions as well — without that, the branch tests keep passing while the numbers drift to something useless.

Verification

  • 302 unit tests, 263 integration tests, tsc clean
  • Lint unchanged at 6 warnings, all pre-existing
  • Three bcrypt.hash call sites updated; no hardcoded cost remains

Not fixed here

The 20s timeout itself, and the fact that a loaded runner can still push a test over it. This removes the dominant cost rather than the sensitivity. #241 covers the related question of whether a red build means anything.

Closes #242

Makes the integration suite **3.5× faster** and removes the load sensitivity that produced the CI failure in #242. ## The measurement Warm run against warm run, with only the constant changed: | Cost factor | Integration suite | Tests | | --- | --- | --- | | 12 | 34.5s | 263 passed | | **4** | **9.8s** | 263 passed | About **25 seconds off every integration run**, on the single runner everything else queues behind. **The first attempt at this measurement was wrong, and it is worth saying so.** I compared a *cold* run at cost 4 against a *warm* run at cost 12 and got 47.7s vs 34.3s — which said the change made things slower. That difference was ts-jest and Postgres warming up, not the cost factor. Both figures in the table are second runs, and the cost-12 number was taken twice (34.3s, 34.5s) before I believed it. ## What was actually wrong The suite registers ~35 customers and asserts nothing about any of their hashes, yet paid bcrypt cost 12 for every one. `bcryptjs` is pure JS, so it pays that cost several times over versus a native build, and hashing was most of the suite's wall clock. On a contended runner that pushed `adminInventory.integration.test.ts` past its 20s timeout — which then surfaced as a foreign key violation somewhere else entirely: the test timed out, Jest moved on, `beforeEach` truncated, and the still-in-flight registration wrote a token for a customer that had just been deleted. The FK error was a symptom; chasing it leads nowhere. ## Deliberately not configurable No environment variable. One would be a way to weaken password hashing in production by misconfiguration, and nothing needs to tune this. The only route to the cheap cost is `NODE_ENV=test`, which a deployed container would announce anyway by refusing to serve the built frontend — `app.ts` gates static serving on the same value. A setting that quietly degrades a security property should be unreachable rather than merely warned about, which is the reasoning that already made `DEMO_MODE` strict. `hashRoundsFor` is pure and separately tested because the failure it guards against is silent. Only the exact string `'test'` earns the cheap cost; an unset `NODE_ENV` gets the strong one, so the dangerous direction has to be asked for explicitly. Both constants are pinned by assertions as well — without that, the branch tests keep passing while the numbers drift to something useless. ## Verification - 302 unit tests, 263 integration tests, `tsc` clean - Lint unchanged at 6 warnings, all pre-existing - Three `bcrypt.hash` call sites updated; no hardcoded cost remains ## Not fixed here The 20s timeout itself, and the fact that a loaded runner can still push a test over it. This removes the dominant cost rather than the sensitivity. #241 covers the related question of whether a red build means anything. Closes #242
bermudalamb added 1 commit 2026-08-30 12:29:48 -05:00
test(perf): stop hashing test passwords at production cost (#242)
SonarQube Analysis / sonarqube (pull_request) Successful in 19m15s
Linting / lint (pull_request) Successful in 2m10s
d7dacffa11
The integration suite registers around thirty-five customers and asserts nothing about any of their hashes, yet paid bcrypt cost 12 for every one. bcryptjs is a pure-JS implementation, so it pays that cost several times over compared with a native build, and hashing was most of the suite's wall clock. On a contended runner it pushed adminInventory.integration.test.ts past its twenty-second timeout, which then surfaced as a foreign key violation somewhere else entirely — the test timed out, jest moved on, beforeEach truncated, and the still-in-flight registration wrote a token for a customer that had just been deleted.

Measured rather than asserted, warm run against warm run with only the constant changed: 34.5s at cost 12, 9.8s at cost 4. Three and a half times faster, about twenty-five seconds off every integration run, with all 263 tests passing either way.

The first attempt at that measurement was wrong and worth recording. Comparing a cold run at cost 4 against a warm run at cost 12 made the change look like a 36% regression-shaped improvement of the wrong size; the difference was ts-jest and Postgres warming up, not the cost factor. Both numbers above are second runs, and the cost-12 figure was taken twice — 34.3s and 34.5s — before being believed.

Deliberately not configurable. An environment variable here would be a way to weaken password hashing in production by misconfiguration, and nothing needs to tune it. The only route to the cheap cost is NODE_ENV=test, which a deployed container would announce anyway by refusing to serve the built frontend, since app.ts gates static serving on the same value. A setting that quietly degrades a security property should be unreachable rather than warned about, which is the reasoning that already made DEMO_MODE strict.

`hashRoundsFor` is pure and separately tested because the failure it guards against is silent: only the exact string 'test' earns the cheap cost, and an unset NODE_ENV gets the strong one, so the dangerous direction has to be asked for explicitly. Both constants are pinned by assertions too — without that the branch tests pass while the numbers drift to something useless.

Closes #242
bermudalamb merged commit feb714c49d into main 2026-08-30 13:32:15 -05:00
bermudalamb deleted branch fix/242-bcrypt-cost-in-tests 2026-08-30 13:32:15 -05:00
Sign in to join this conversation.
No Reviewers
No labels
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: bermudalamb/redefined-designs#243