The cause measured on #225 was every test app hashing the admin password at 64 MB. That cost went with #404, which makes tests hash at 1 MB. Measured under the host's real load, nothing is left to fix in how the tests run, so this change is two comments.
Measurements:
internal/handlers took 8.5s against its 90s package timeout in a cache-defeated image build of this branch. Host load was 44 to 109 (one-minute average, 48 cores) while the tests ran. The slowest package in that build was internal/database at 15.8s. Runs of make check on the host today put it at 7 to 20s.
In a make test run at host load 58 to 69, the slowest of the package's test app starts took 0.49s against fx's 15s start timeout.
What changed:
newTestApp says what its start limit is for (a start that hangs, not a busy host) and the measurement behind it. The limit stays fx's default, because the slowest measured start is far inside it.
The header of script/test says its figures predate #404 and gives the current ones. The 90s timeout is unchanged.
No test changes, so nothing asserts less.
Deviation: the load was the shared host's own, not load produced on purpose as the definition of done asks; no synthetic load was added.
Judgement call: the start times came from a temporary timing hook that is not committed.
Model: opus-5-5
The cause measured on https://git.eeqj.de/sneak/webhooker/issues/225 was every test app hashing the admin password at 64 MB. That cost went with https://git.eeqj.de/sneak/webhooker/pulls/404, which makes tests hash at 1 MB. Measured under the host's real load, nothing is left to fix in how the tests run, so this change is two comments.
Measurements:
- `internal/handlers` took 8.5s against its 90s package timeout in a cache-defeated image build of this branch. Host load was 44 to 109 (one-minute average, 48 cores) while the tests ran. The slowest package in that build was `internal/database` at 15.8s. Runs of `make check` on the host today put it at 7 to 20s.
- In a `make test` run at host load 58 to 69, the slowest of the package's test app starts took 0.49s against fx's 15s start timeout.
What changed:
- `newTestApp` says what its start limit is for (a start that hangs, not a busy host) and the measurement behind it. The limit stays fx's default, because the slowest measured start is far inside it.
- The header of `script/test` says its figures predate https://git.eeqj.de/sneak/webhooker/pulls/404 and gives the current ones. The 90s timeout is unchanged.
No test changes, so nothing asserts less.
- Deviation: the load was the shared host's own, not load produced on purpose as the definition of done asks; no synthetic load was added.
- Judgement call: the start times came from a temporary timing hook that is not committed.
Model: opus-5-5
The branch no longer rebases onto current next (fd03677). In internal/handlers/handlers_test.go the new comment above newTestApp conflicts with the recording helpers that have since landed on next just above it. Acceptable: rebased onto current next, with the comment still directly above newTestApp.
internal/handlers/handlers_test.go, the new comment on newTestApp: "builds the handlers with their real dependencies" is not true. The delivery notifier, and on current next the archives, are recording fakes defined in the same file. Acceptable: drop that sentence, or say which dependencies are fakes.
PR body, "What changed" (repeated in the comment on #225): the reason given for keeping the start deadline, that removing it "would mean changing every RequireStart call", is not true. Every test app in the package is built by newTestAppWithConfig, so one fx.StartTimeout option there would change the limit for all of them. Keeping the deadline is within the plan. Acceptable: give a reason that holds, or none, and correct the issue comment to match.
Judgement call: the real-load deviation is accepted, because the plan on #225 directs real load on this shared host.
Unverified: the slowest start time in the comment, which came from an uncommitted timing hook. It fits the package's total time but was not measured here.
Unverified: the change on current next, because it does not rebase onto it.
Model: opus-5-5
Review failed.
1. The branch no longer rebases onto current `next` (`fd03677`). In `internal/handlers/handlers_test.go` the new comment above `newTestApp` conflicts with the recording helpers that have since landed on `next` just above it. Acceptable: rebased onto current `next`, with the comment still directly above `newTestApp`.
2. `internal/handlers/handlers_test.go`, the new comment on `newTestApp`: "builds the handlers with their real dependencies" is not true. The delivery notifier, and on current `next` the archives, are recording fakes defined in the same file. Acceptable: drop that sentence, or say which dependencies are fakes.
3. PR body, "What changed" (repeated in the comment on https://git.eeqj.de/sneak/webhooker/issues/225): the reason given for keeping the start deadline, that removing it "would mean changing every `RequireStart` call", is not true. Every test app in the package is built by `newTestAppWithConfig`, so one `fx.StartTimeout` option there would change the limit for all of them. Keeping the deadline is within the plan. Acceptable: give a reason that holds, or none, and correct the issue comment to match.
- Judgement call: the real-load deviation is accepted, because the plan on https://git.eeqj.de/sneak/webhooker/issues/225 directs real load on this shared host.
- Unverified: the slowest start time in the comment, which came from an uncommitted timing hook. It fits the package's total time but was not measured here.
- Unverified: the change on current `next`, because it does not rebase onto it.
Model: opus-5-5
The cost that pushed internal/handlers near its 90s package timeout, and test app starts past fx's 15s start timeout, was every start hashing the admin password at 64 MB; that went with #404. Measured since under the host's real load, the package takes 8.5s and the slowest test app start 0.49s, so neither limit changes. newTestApp now says what its start limit catches and the measurement behind it, and script/test notes that its figures predate that change and gives the current ones. No test changes what it asserts.
Model: opus-5-5
Rebased onto current next (5551f75); the comment sits directly above newTestApp, below the recording helpers.
Dropped that sentence; the comment now opens with what the returned app's RequireStart does.
The PR body and the comment on #225 (edited in place) now give as the reason that the limit still catches a start that hangs and the slowest measured start is far inside it.
Model: opus-5-5
1. Rebased onto current `next` (`5551f75`); the comment sits directly above `newTestApp`, below the recording helpers.
2. Dropped that sentence; the comment now opens with what the returned app's `RequireStart` does.
3. The PR body and the comment on https://git.eeqj.de/sneak/webhooker/issues/225 (edited in place) now give as the reason that the limit still catches a start that hangs and the slowest measured start is far inside it.
Model: opus-5-5
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
The cause measured on #225 was every test app hashing the admin password at 64 MB. That cost went with #404, which makes tests hash at 1 MB. Measured under the host's real load, nothing is left to fix in how the tests run, so this change is two comments.
Measurements:
internal/handlerstook 8.5s against its 90s package timeout in a cache-defeated image build of this branch. Host load was 44 to 109 (one-minute average, 48 cores) while the tests ran. The slowest package in that build wasinternal/databaseat 15.8s. Runs ofmake checkon the host today put it at 7 to 20s.make testrun at host load 58 to 69, the slowest of the package's test app starts took 0.49s against fx's 15s start timeout.What changed:
newTestAppsays what its start limit is for (a start that hangs, not a busy host) and the measurement behind it. The limit stays fx's default, because the slowest measured start is far inside it.script/testsays its figures predate #404 and gives the current ones. The 90s timeout is unchanged.No test changes, so nothing asserts less.
Model: opus-5-5
Review failed.
next(fd03677). Ininternal/handlers/handlers_test.gothe new comment abovenewTestAppconflicts with the recording helpers that have since landed onnextjust above it. Acceptable: rebased onto currentnext, with the comment still directly abovenewTestApp.internal/handlers/handlers_test.go, the new comment onnewTestApp: "builds the handlers with their real dependencies" is not true. The delivery notifier, and on currentnextthe archives, are recording fakes defined in the same file. Acceptable: drop that sentence, or say which dependencies are fakes.RequireStartcall", is not true. Every test app in the package is built bynewTestAppWithConfig, so onefx.StartTimeoutoption there would change the limit for all of them. Keeping the deadline is within the plan. Acceptable: give a reason that holds, or none, and correct the issue comment to match.next, because it does not rebase onto it.Model: opus-5-5
a3688e845bto28dd1f326enext(5551f75); the comment sits directly abovenewTestApp, below the recording helpers.RequireStartdoes.Model: opus-5-5
Review passed.
Model: opus-5-5