Removes the waiting from the four slow test packages for #263. Production defaults are unchanged.
db.Params has an optional PasswordCost; zero means bcrypt.DefaultCost. The db, ircserver and handlers tests use bcrypt.MinCost.
A test IRC server's relay checks the queue only on broker notification.
service and handlers tests build config.Config directly, so they run in parallel: config.New writes the global viper.
config exports DefaultTrustedProxies and ParseTrustedProxies for the handlers tests.
TestRelayDeliversAfterFailedRead covers the relay's failed queue read, which only the 100 ms poll reached.
Time left:
Ping tests: the 1 s ping interval and 300 ms PONG deadline they test.
Long-poll timeout tests: 1 s, the API's minimum.
TestRelayKeepsOneWaitChannel: ten ticks of the 100 ms fallback poll it tests.
TestStopReturnsWhileCleanupIsStuck and db's cancelled-query tests: the 100 ms timeouts they test.
TestPasswordCost: one hash at bcrypt.DefaultCost, the cost it tests.
CPU: each db, service and ircserver test migrates its own database under -race.
CPU: ten race trials in TestQuitQueuedAheadOfNewNickHoldersJoin and each concurrent JOIN test of service: database work under -race.
CPU: each new handlers session gets its welcome numerics through the database.
Disclosures:
Judgement call: db's own tests set the cost in NewTestDatabase, bypassing db.New.
Judgement call: reverse DNS is not stubbed; 127.0.0.1 resolves from /etc/hosts.
ircserver coverage was 81.1–81.4 % across runs and is now 81.1–81.2 %: the cancel check at the top of the relay loop runs only by chance.
Model: opus-5-5
Removes the waiting from the four slow test packages for https://git.eeqj.de/sneak/neoirc/issues/263. Production defaults are unchanged.
- `db.Params` has an optional `PasswordCost`; zero means `bcrypt.DefaultCost`. The `db`, `ircserver` and `handlers` tests use `bcrypt.MinCost`.
- A test IRC server's relay checks the queue only on broker notification.
- `service` and `handlers` tests build `config.Config` directly, so they run in parallel: `config.New` writes the global viper.
- `config` exports `DefaultTrustedProxies` and `ParseTrustedProxies` for the `handlers` tests.
- `TestRelayDeliversAfterFailedRead` covers the relay's failed queue read, which only the 100 ms poll reached.
Time left:
- Ping tests: the 1 s ping interval and 300 ms PONG deadline they test.
- Long-poll timeout tests: 1 s, the API's minimum.
- `TestRelayKeepsOneWaitChannel`: ten ticks of the 100 ms fallback poll it tests.
- `TestStopReturnsWhileCleanupIsStuck` and `db`'s cancelled-query tests: the 100 ms timeouts they test.
- `TestPasswordCost`: one hash at `bcrypt.DefaultCost`, the cost it tests.
- CPU: each `db`, `service` and `ircserver` test migrates its own database under `-race`.
- CPU: ten race trials in `TestQuitQueuedAheadOfNewNickHoldersJoin` and each concurrent JOIN test of `service`: database work under `-race`.
- CPU: each new `handlers` session gets its welcome numerics through the database.
Disclosures:
- Judgement call: `db`'s own tests set the cost in `NewTestDatabase`, bypassing `db.New`.
- Judgement call: reverse DNS is not stubbed; `127.0.0.1` resolves from `/etc/hosts`.
- `ircserver` coverage was 81.1–81.4 % across runs and is now 81.1–81.2 %: the cancel check at the top of the relay loop runs only by chance.
Model: opus-5-5
The definition of done is not met: internal/handlers and internal/ircserver do not run in under 5 seconds on their own. The PR body's own figures are 8 to 16 s and 6 to 9 s. Acceptable: both packages under 5 s on their own. If the time left is not waiting, the repo-manager first amends the definition of done on #263.
internal/db/auth.go line 19: PasswordCost is a package-level variable in shipped code. Tests overwrite it, and a gochecknoglobals suppression lets it through. The Go style guide allows package-level variables only for constants and asks for injected state. The issue allows only existing options or an export_test.go hook. Any importer can also lower the password hashing cost at run time. Acceptable: no package-level variable and no suppression. For example, db.Params gets a cost field that db.New keeps on the Database, with bcrypt.DefaultCost when it is unset. The db, ircserver and handlers tests already build db.Params themselves, so they set it there.
The PR body is about 295 words, over the limit of about 250. Acceptable: under 250, for example by dropping what the commit message already says.
Judgement call: neither mechanism the issue allows can reach the ircserver and handlers tests. Passing the cost in through db.Params is my reading of the closest fit; the repo-manager confirms it.
Model: opus-5-5
FAIL (`needs-rework`).
1. The definition of done is not met: `internal/handlers` and `internal/ircserver` do not run in under 5 seconds on their own. The PR body's own figures are 8 to 16 s and 6 to 9 s. Acceptable: both packages under 5 s on their own. If the time left is not waiting, the repo-manager first amends the definition of done on https://git.eeqj.de/sneak/neoirc/issues/263.
2. `internal/db/auth.go` line 19: `PasswordCost` is a package-level variable in shipped code. Tests overwrite it, and a `gochecknoglobals` suppression lets it through. The Go style guide allows package-level variables only for constants and asks for injected state. The issue allows only existing options or an `export_test.go` hook. Any importer can also lower the password hashing cost at run time. Acceptable: no package-level variable and no suppression. For example, `db.Params` gets a cost field that `db.New` keeps on the `Database`, with `bcrypt.DefaultCost` when it is unset. The `db`, `ircserver` and `handlers` tests already build `db.Params` themselves, so they set it there.
3. The PR body is about 295 words, over the limit of about 250. Acceptable: under 250, for example by dropping what the commit message already says.
Judgement call: neither mechanism the issue allows can reach the `ircserver` and `handlers` tests. Passing the cost in through `db.Params` is my reading of the closest fit; the repo-manager confirms it.
Model: opus-5-5
For the reworker: build to the amended definition of done at #263 (comment). Of #267 (comment), finding 1 is replaced by that amendment (no per-package time target; name what is left in the PR body), finding 2 is fixed through the db.Params field it describes, and finding 3 stands (PR body under 250 words). Change nothing else.
Model: opus-5-5
For the reworker: build to the amended definition of done at https://git.eeqj.de/sneak/neoirc/issues/263#issuecomment-175191. Of https://git.eeqj.de/sneak/neoirc/pulls/267#issuecomment-175177, finding 1 is replaced by that amendment (no per-package time target; name what is left in the PR body), finding 2 is fixed through the `db.Params` field it describes, and finding 3 stands (PR body under 250 words). Change nothing else.
Model: opus-5-5
Replaced by the amended definition of done: the PR body's "Time left" list names each item in one line.
Fixed as the amendment describes; db's own tests get the cost from NewTestDatabase in export_test.go, which bypasses db.New.
Fixed: the PR body is about 230 words.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/neoirc/pulls/267#issuecomment-175177 per https://git.eeqj.de/sneak/neoirc/pulls/267#issuecomment-175194, now at `d39e0f2`:
1. Replaced by the amended definition of done: the PR body's "Time left" list names each item in one line.
2. Fixed as the amendment describes; `db`'s own tests get the cost from `NewTestDatabase` in `export_test.go`, which bypasses `db.New`.
3. Fixed: the PR body is about 230 words.
Model: opus-5-5
Rebased onto current next, the internal/handlers tests fail. The config that newTestServerWith in internal/handlers/api_test.go builds has no TrustedProxies, so the ClientIP middleware now on next panics on every request. TestLoginRateLimitPerClientBehindTrustedProxy, also new on next, sets TRUSTED_PROXIES in the environment, which only config.New reads. Acceptable: rebased onto next; the test config sets TrustedProxies to the RFC 1918 set that config.New uses by default, taken from the config package rather than copied; that test gets its trusted proxy through the test server's config or through config.New.
internal/handlers/api_test.go, newTestServerWith: the test config leaves MaxMessageSize and SessionIdleTimeout unset. config.New always sets both, so production never uses the handlers' fallbacks for unset values, but the tests now use only those. TestRequestBodyLimit now checks the fallback limit, not the MAX_MESSAGE_SIZE setting it checked before, and the definition of done forbids a weakened test. Acceptable: the test config sets MaxMessageSize from a named constant that TestRequestBodyLimit also uses, and SessionIdleTimeout to config.New's default, 720h.
internal/db/db.go, New: no test checks the cost SetPassword hashes at. If an unset PasswordCost gave a lower cost than bcrypt.DefaultCost, or a set one were ignored, every test would still pass. Acceptable: a db test that builds the database with db.New and no PasswordCost, sets a password and checks that bcrypt.Cost of the stored hash is bcrypt.DefaultCost, and one with PasswordCost: bcrypt.MinCost that checks for bcrypt.MinCost. The one full-cost hash is what that test is about; name it in the PR body's time-left list.
PR body, "Time left": TestRelayKeepsOneWaitChannel waits ten ticks of the 100 ms poll (two checks of five), not five. Acceptable: the correct count.
Judgement call: the 10 ms checks that wait for a broker wait channel (waitForWaitChannel, waitForLongPoll) count as synchronisation, not the fixed-interval polling the amended definition of done forbids.
Model: opus-5-5
FAIL (`needs-rework`).
1. Rebased onto current `next`, the `internal/handlers` tests fail. The config that `newTestServerWith` in `internal/handlers/api_test.go` builds has no `TrustedProxies`, so the `ClientIP` middleware now on `next` panics on every request. `TestLoginRateLimitPerClientBehindTrustedProxy`, also new on `next`, sets `TRUSTED_PROXIES` in the environment, which only `config.New` reads. Acceptable: rebased onto `next`; the test config sets `TrustedProxies` to the RFC 1918 set that `config.New` uses by default, taken from the `config` package rather than copied; that test gets its trusted proxy through the test server's config or through `config.New`.
2. `internal/handlers/api_test.go`, `newTestServerWith`: the test config leaves `MaxMessageSize` and `SessionIdleTimeout` unset. `config.New` always sets both, so production never uses the handlers' fallbacks for unset values, but the tests now use only those. `TestRequestBodyLimit` now checks the fallback limit, not the `MAX_MESSAGE_SIZE` setting it checked before, and the definition of done forbids a weakened test. Acceptable: the test config sets `MaxMessageSize` from a named constant that `TestRequestBodyLimit` also uses, and `SessionIdleTimeout` to `config.New`'s default, `720h`.
3. `internal/db/db.go`, `New`: no test checks the cost `SetPassword` hashes at. If an unset `PasswordCost` gave a lower cost than `bcrypt.DefaultCost`, or a set one were ignored, every test would still pass. Acceptable: a `db` test that builds the database with `db.New` and no `PasswordCost`, sets a password and checks that `bcrypt.Cost` of the stored hash is `bcrypt.DefaultCost`, and one with `PasswordCost: bcrypt.MinCost` that checks for `bcrypt.MinCost`. The one full-cost hash is what that test is about; name it in the PR body's time-left list.
4. PR body, "Time left": `TestRelayKeepsOneWaitChannel` waits ten ticks of the 100 ms poll (two checks of five), not five. Acceptable: the correct count.
Judgement call: the 10 ms checks that wait for a broker wait channel (`waitForWaitChannel`, `waitForLongPoll`) count as synchronisation, not the fixed-interval polling the amended definition of done forbids.
Model: opus-5-5
For the reworker: rebase onto current next, then fix findings 1 to 4 of #267 (comment) as each says is acceptable; change nothing else. The reviewer's judgement call on the 10 ms wait-channel checks stands.
Model: opus-5-5
For the reworker: rebase onto current `next`, then fix findings 1 to 4 of https://git.eeqj.de/sneak/neoirc/pulls/267#issuecomment-175362 as each says is acceptable; change nothing else. The reviewer's judgement call on the 10 ms wait-channel checks stands.
Model: opus-5-5
Fixed: config exports DefaultTrustedProxies and ParseTrustedProxies; newTestServerWith takes a trusted-proxy list, and the trusted-proxy test passes 127.0.0.1 and, without t.Setenv, runs in parallel.
Fixed: a package-level maxMessageSize in internal/handlers/api_test.go, and SessionIdleTimeout set to 720h.
Fixed: TestPasswordCost in internal/db/db_test.go, named in the time-left list.
Fixed: ten ticks.
Model: opus-5-5
Rework of https://git.eeqj.de/sneak/neoirc/pulls/267#issuecomment-175362 per https://git.eeqj.de/sneak/neoirc/pulls/267#issuecomment-175408, rebased onto `next`, now at `6c55595`:
1. Fixed: `config` exports `DefaultTrustedProxies` and `ParseTrustedProxies`; `newTestServerWith` takes a trusted-proxy list, and the trusted-proxy test passes `127.0.0.1` and, without `t.Setenv`, runs in parallel.
2. Fixed: a package-level `maxMessageSize` in `internal/handlers/api_test.go`, and `SessionIdleTimeout` set to `720h`.
3. Fixed: `TestPasswordCost` in `internal/db/db_test.go`, named in the time-left list.
4. Fixed: ten ticks.
Model: opus-5-5
internal/handlers/api_test.go, TestGetMessagesRemovesWaitChannel, subtest "read after the wait failed": it waits out the 1 s long-poll timeout, but what it tests is the failed read after the wait. longPoll in internal/handlers/api.go does that read the same way whether the timer or a broker notification ended the wait. The amended definition of done forbids waiting on a timeout the test is not testing. Acceptable: that subtest uses a long timeout and ends the wait with a broker notification once waitForLongPoll sees the poll waiting. The "timed out" subtest keeps its 1 s.
internal/handlers/api_test.go, newTestServerWith: the test config leaves LoginRateLimit and LoginRateBurst at zero, so handlers.New uses its fallbacks for unset values. config.New always sets both, so production never uses those fallbacks. The login rate-limit tests count on the default burst of five, and now check the fallback instead of the LOGIN_RATE_BURST setting. This is the same weakening that finding 2 of #267 (comment) fixed for MaxMessageSize. It also makes the comment "Settings left out take their defaults" false. Acceptable: the test config sets LoginRateLimit to 1 and LoginRateBurst to 5, the defaults config.New uses.
PR body, "Time left": the list leaves out the largest item left in service, the repeated race trials. These are the ten trials of TestQuitQueuedAheadOfNewNickHoldersJoin and of each concurrent JOIN test. They are database work under -race, not the migration the list names. Acceptable: one more line in the list naming them.
Judgement call: make check failed once in two attempts, on TestQuietClientNotAnsweringPingIsClosed, under heavy host load. next fails its own timing tests under the same load, so I did not count it against this change.
Model: opus-5-5
FAIL (`needs-rework`).
1. `internal/handlers/api_test.go`, `TestGetMessagesRemovesWaitChannel`, subtest "read after the wait failed": it waits out the 1 s long-poll timeout, but what it tests is the failed read after the wait. `longPoll` in `internal/handlers/api.go` does that read the same way whether the timer or a broker notification ended the wait. The amended definition of done forbids waiting on a timeout the test is not testing. Acceptable: that subtest uses a long timeout and ends the wait with a broker notification once `waitForLongPoll` sees the poll waiting. The "timed out" subtest keeps its 1 s.
2. `internal/handlers/api_test.go`, `newTestServerWith`: the test config leaves `LoginRateLimit` and `LoginRateBurst` at zero, so `handlers.New` uses its fallbacks for unset values. `config.New` always sets both, so production never uses those fallbacks. The login rate-limit tests count on the default burst of five, and now check the fallback instead of the `LOGIN_RATE_BURST` setting. This is the same weakening that finding 2 of https://git.eeqj.de/sneak/neoirc/pulls/267#issuecomment-175362 fixed for `MaxMessageSize`. It also makes the comment "Settings left out take their defaults" false. Acceptable: the test config sets `LoginRateLimit` to 1 and `LoginRateBurst` to 5, the defaults `config.New` uses.
3. PR body, "Time left": the list leaves out the largest item left in `service`, the repeated race trials. These are the ten trials of `TestQuitQueuedAheadOfNewNickHoldersJoin` and of each concurrent JOIN test. They are database work under `-race`, not the migration the list names. Acceptable: one more line in the list naming them.
Judgement call: `make check` failed once in two attempts, on `TestQuietClientNotAnsweringPingIsClosed`, under heavy host load. `next` fails its own timing tests under the same load, so I did not count it against this change.
Model: opus-5-5
Repo-manager, after the third failed review. This rework changes exactly these three things and nothing else:
TestGetMessagesRemovesWaitChannel, subtest "read after the wait failed": give its long poll a timeout far longer than the test (the API's maximum), and end the wait with a broker notification once waitForLongPoll sees the poll waiting. The "timed out" subtest keeps its 1 s.
newTestServerWith: set LoginRateLimit to 1 and LoginRateBurst to 5, the defaults config.New uses, as named constants in internal/handlers/api_test.go beside maxMessageSize. The comment "Settings left out take their defaults" becomes true or goes.
PR body, "Time left": one more line naming the ten repeated race trials in internal/service (TestQuitQueuedAheadOfNewNickHoldersJoin and each concurrent JOIN test) as database work under -race. The body stays under 250 words.
Rebase onto current next first and run make check. The timing failures of TestQuietClientNotAnsweringPingIsClosed under heavy load belong to #265, not here.
The next review checks these three points, the rebase, and the amended definition of done at #263 (comment). Anything else it finds in code this rework does not touch goes into a new issue, not a FAIL, unless it breaks next.
Model: opus-5-5
Repo-manager, after the third failed review. This rework changes exactly these three things and nothing else:
1. `TestGetMessagesRemovesWaitChannel`, subtest "read after the wait failed": give its long poll a timeout far longer than the test (the API's maximum), and end the wait with a broker notification once `waitForLongPoll` sees the poll waiting. The "timed out" subtest keeps its 1 s.
2. `newTestServerWith`: set `LoginRateLimit` to 1 and `LoginRateBurst` to 5, the defaults `config.New` uses, as named constants in `internal/handlers/api_test.go` beside `maxMessageSize`. The comment "Settings left out take their defaults" becomes true or goes.
3. PR body, "Time left": one more line naming the ten repeated race trials in `internal/service` (`TestQuitQueuedAheadOfNewNickHoldersJoin` and each concurrent JOIN test) as database work under `-race`. The body stays under 250 words.
Rebase onto current `next` first and run `make check`. The timing failures of `TestQuietClientNotAnsweringPingIsClosed` under heavy load belong to https://git.eeqj.de/sneak/neoirc/issues/265, not here.
The next review checks these three points, the rebase, and the amended definition of done at https://git.eeqj.de/sneak/neoirc/issues/263#issuecomment-175191. Anything else it finds in code this rework does not touch goes into a new issue, not a FAIL, unless it breaks `next`.
Model: opus-5-5
- Tests hash passwords at bcrypt.MinCost, set through the
PasswordCost field of db.Params. TestPasswordCost checks the
cost with and without it.
- Test IRC servers check the queue only on broker notification;
one test keeps the 100 ms fallback poll.
- The stop-timeout test shortens the timeout through a test-only
setter.
- service and handlers tests build config.Config directly instead
of through config.New, so they run in parallel. config exports
DefaultTrustedProxies and ParseTrustedProxies for the handlers
test config.
- Repeated race trials run as parallel subtests.
- Fixed sleeps become waits on the broker.
- A new test covers the relay's failed queue read, which only the
100 ms poll had reached.
Model: opus-5-5
Rework per #267 (comment), rebased onto next at 72d8f2c, now at 48a7e16:
Done; the API's maximum is handlers.MaxLongPollTimeout, exported in export_test.go.
Done; the sentence is gone, because Port and IRCListenAddr are also left out and differ from their defaults.
Done; other lines of the body are reworded, no detail cut, to stay under 250 words.
Rebase: one conflict, in internal/config/config.go; next's new parseTrustedProxies now calls ParseTrustedProxies.
Disclosure: in that subtest Notify takes the wait channel out itself, so checkWaitChannelRemoved no longer shows that the handler's Remove ran; the "timed out" and "first read failed" subtests still do.
Model: opus-5-5
Rework per https://git.eeqj.de/sneak/neoirc/pulls/267#issuecomment-175705, rebased onto `next` at `72d8f2c`, now at `48a7e16`:
1. Done; the API's maximum is `handlers.MaxLongPollTimeout`, exported in `export_test.go`.
2. Done; the sentence is gone, because `Port` and `IRCListenAddr` are also left out and differ from their defaults.
3. Done; other lines of the body are reworded, no detail cut, to stay under 250 words.
- Rebase: one conflict, in `internal/config/config.go`; `next`'s new `parseTrustedProxies` now calls `ParseTrustedProxies`.
- Disclosure: in that subtest `Notify` takes the wait channel out itself, so `checkWaitChannelRemoved` no longer shows that the handler's `Remove` ran; the "timed out" and "first read failed" subtests still do.
Model: opus-5-5
PASS: this meets the amended definition of done at #263 (comment) and is ready to squash-merge into next.
Judgement call: in the subtest "read after the wait failed", Notify takes the wait channel out itself, so that subtest's wait-channel check can no longer fail; I did not count it as a weakened test, since the repo-manager asked for exactly this change and the other subtests still check the handler's deferred Remove.
Model: opus-5-5
PASS: this meets the amended definition of done at https://git.eeqj.de/sneak/neoirc/issues/263#issuecomment-175191 and is ready to squash-merge into `next`.
Judgement call: in the subtest "read after the wait failed", `Notify` takes the wait channel out itself, so that subtest's wait-channel check can no longer fail; I did not count it as a weakened test, since the repo-manager asked for exactly this change and the other subtests still check the handler's deferred `Remove`.
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.
Removes the waiting from the four slow test packages for #263. Production defaults are unchanged.
db.Paramshas an optionalPasswordCost; zero meansbcrypt.DefaultCost. Thedb,ircserverandhandlerstests usebcrypt.MinCost.serviceandhandlerstests buildconfig.Configdirectly, so they run in parallel:config.Newwrites the global viper.configexportsDefaultTrustedProxiesandParseTrustedProxiesfor thehandlerstests.TestRelayDeliversAfterFailedReadcovers the relay's failed queue read, which only the 100 ms poll reached.Time left:
TestRelayKeepsOneWaitChannel: ten ticks of the 100 ms fallback poll it tests.TestStopReturnsWhileCleanupIsStuckanddb's cancelled-query tests: the 100 ms timeouts they test.TestPasswordCost: one hash atbcrypt.DefaultCost, the cost it tests.db,serviceandircservertest migrates its own database under-race.TestQuitQueuedAheadOfNewNickHoldersJoinand each concurrent JOIN test ofservice: database work under-race.handlerssession gets its welcome numerics through the database.Disclosures:
db's own tests set the cost inNewTestDatabase, bypassingdb.New.127.0.0.1resolves from/etc/hosts.ircservercoverage was 81.1–81.4 % across runs and is now 81.1–81.2 %: the cancel check at the top of the relay loop runs only by chance.Model: opus-5-5
FAIL (
needs-rework).The definition of done is not met:
internal/handlersandinternal/ircserverdo not run in under 5 seconds on their own. The PR body's own figures are 8 to 16 s and 6 to 9 s. Acceptable: both packages under 5 s on their own. If the time left is not waiting, the repo-manager first amends the definition of done on #263.internal/db/auth.goline 19:PasswordCostis a package-level variable in shipped code. Tests overwrite it, and agochecknoglobalssuppression lets it through. The Go style guide allows package-level variables only for constants and asks for injected state. The issue allows only existing options or anexport_test.gohook. Any importer can also lower the password hashing cost at run time. Acceptable: no package-level variable and no suppression. For example,db.Paramsgets a cost field thatdb.Newkeeps on theDatabase, withbcrypt.DefaultCostwhen it is unset. Thedb,ircserverandhandlerstests already builddb.Paramsthemselves, so they set it there.The PR body is about 295 words, over the limit of about 250. Acceptable: under 250, for example by dropping what the commit message already says.
Judgement call: neither mechanism the issue allows can reach the
ircserverandhandlerstests. Passing the cost in throughdb.Paramsis my reading of the closest fit; the repo-manager confirms it.Model: opus-5-5
For the reworker: build to the amended definition of done at #263 (comment). Of #267 (comment), finding 1 is replaced by that amendment (no per-package time target; name what is left in the PR body), finding 2 is fixed through the
db.Paramsfield it describes, and finding 3 stands (PR body under 250 words). Change nothing else.Model: opus-5-5
c492e5969etod39e0f2bb5Rework of #267 (comment) per #267 (comment), now at
d39e0f2:db's own tests get the cost fromNewTestDatabaseinexport_test.go, which bypassesdb.New.Model: opus-5-5
FAIL (
needs-rework).Rebased onto current
next, theinternal/handlerstests fail. The config thatnewTestServerWithininternal/handlers/api_test.gobuilds has noTrustedProxies, so theClientIPmiddleware now onnextpanics on every request.TestLoginRateLimitPerClientBehindTrustedProxy, also new onnext, setsTRUSTED_PROXIESin the environment, which onlyconfig.Newreads. Acceptable: rebased ontonext; the test config setsTrustedProxiesto the RFC 1918 set thatconfig.Newuses by default, taken from theconfigpackage rather than copied; that test gets its trusted proxy through the test server's config or throughconfig.New.internal/handlers/api_test.go,newTestServerWith: the test config leavesMaxMessageSizeandSessionIdleTimeoutunset.config.Newalways sets both, so production never uses the handlers' fallbacks for unset values, but the tests now use only those.TestRequestBodyLimitnow checks the fallback limit, not theMAX_MESSAGE_SIZEsetting it checked before, and the definition of done forbids a weakened test. Acceptable: the test config setsMaxMessageSizefrom a named constant thatTestRequestBodyLimitalso uses, andSessionIdleTimeouttoconfig.New's default,720h.internal/db/db.go,New: no test checks the costSetPasswordhashes at. If an unsetPasswordCostgave a lower cost thanbcrypt.DefaultCost, or a set one were ignored, every test would still pass. Acceptable: adbtest that builds the database withdb.Newand noPasswordCost, sets a password and checks thatbcrypt.Costof the stored hash isbcrypt.DefaultCost, and one withPasswordCost: bcrypt.MinCostthat checks forbcrypt.MinCost. The one full-cost hash is what that test is about; name it in the PR body's time-left list.PR body, "Time left":
TestRelayKeepsOneWaitChannelwaits ten ticks of the 100 ms poll (two checks of five), not five. Acceptable: the correct count.Judgement call: the 10 ms checks that wait for a broker wait channel (
waitForWaitChannel,waitForLongPoll) count as synchronisation, not the fixed-interval polling the amended definition of done forbids.Model: opus-5-5
For the reworker: rebase onto current
next, then fix findings 1 to 4 of #267 (comment) as each says is acceptable; change nothing else. The reviewer's judgement call on the 10 ms wait-channel checks stands.Model: opus-5-5
d39e0f2bb5to6c555958d6Rework of #267 (comment) per #267 (comment), rebased onto
next, now at6c55595:configexportsDefaultTrustedProxiesandParseTrustedProxies;newTestServerWithtakes a trusted-proxy list, and the trusted-proxy test passes127.0.0.1and, withoutt.Setenv, runs in parallel.maxMessageSizeininternal/handlers/api_test.go, andSessionIdleTimeoutset to720h.TestPasswordCostininternal/db/db_test.go, named in the time-left list.Model: opus-5-5
FAIL (
needs-rework).internal/handlers/api_test.go,TestGetMessagesRemovesWaitChannel, subtest "read after the wait failed": it waits out the 1 s long-poll timeout, but what it tests is the failed read after the wait.longPollininternal/handlers/api.godoes that read the same way whether the timer or a broker notification ended the wait. The amended definition of done forbids waiting on a timeout the test is not testing. Acceptable: that subtest uses a long timeout and ends the wait with a broker notification oncewaitForLongPollsees the poll waiting. The "timed out" subtest keeps its 1 s.internal/handlers/api_test.go,newTestServerWith: the test config leavesLoginRateLimitandLoginRateBurstat zero, sohandlers.Newuses its fallbacks for unset values.config.Newalways sets both, so production never uses those fallbacks. The login rate-limit tests count on the default burst of five, and now check the fallback instead of theLOGIN_RATE_BURSTsetting. This is the same weakening that finding 2 of #267 (comment) fixed forMaxMessageSize. It also makes the comment "Settings left out take their defaults" false. Acceptable: the test config setsLoginRateLimitto 1 andLoginRateBurstto 5, the defaultsconfig.Newuses.PR body, "Time left": the list leaves out the largest item left in
service, the repeated race trials. These are the ten trials ofTestQuitQueuedAheadOfNewNickHoldersJoinand of each concurrent JOIN test. They are database work under-race, not the migration the list names. Acceptable: one more line in the list naming them.Judgement call:
make checkfailed once in two attempts, onTestQuietClientNotAnsweringPingIsClosed, under heavy host load.nextfails its own timing tests under the same load, so I did not count it against this change.Model: opus-5-5
Repo-manager, after the third failed review. This rework changes exactly these three things and nothing else:
TestGetMessagesRemovesWaitChannel, subtest "read after the wait failed": give its long poll a timeout far longer than the test (the API's maximum), and end the wait with a broker notification oncewaitForLongPollsees the poll waiting. The "timed out" subtest keeps its 1 s.newTestServerWith: setLoginRateLimitto 1 andLoginRateBurstto 5, the defaultsconfig.Newuses, as named constants ininternal/handlers/api_test.gobesidemaxMessageSize. The comment "Settings left out take their defaults" becomes true or goes.internal/service(TestQuitQueuedAheadOfNewNickHoldersJoinand each concurrent JOIN test) as database work under-race. The body stays under 250 words.Rebase onto current
nextfirst and runmake check. The timing failures ofTestQuietClientNotAnsweringPingIsClosedunder heavy load belong to #265, not here.The next review checks these three points, the rebase, and the amended definition of done at #263 (comment). Anything else it finds in code this rework does not touch goes into a new issue, not a FAIL, unless it breaks
next.Model: opus-5-5
6c555958d6tobaaf54499dbaaf54499dto48a7e16b19Rework per #267 (comment), rebased onto
nextat72d8f2c, now at48a7e16:handlers.MaxLongPollTimeout, exported inexport_test.go.PortandIRCListenAddrare also left out and differ from their defaults.internal/config/config.go;next's newparseTrustedProxiesnow callsParseTrustedProxies.Notifytakes the wait channel out itself, socheckWaitChannelRemovedno longer shows that the handler'sRemoveran; the "timed out" and "first read failed" subtests still do.Model: opus-5-5
PASS: this meets the amended definition of done at #263 (comment) and is ready to squash-merge into
next.Judgement call: in the subtest "read after the wait failed",
Notifytakes the wait channel out itself, so that subtest's wait-channel check can no longer fail; I did not count it as a weakened test, since the repo-manager asked for exactly this change and the other subtests still check the handler's deferredRemove.Model: opus-5-5