Follows the plan on #94, each item checked against next first.
Every config test, and the first-boot debug log test in internal/gormlog, starts from an empty environment: config.ClearEnvForTest (the package's testing.go) unsets every variable the process has, then the test sets only what it tests; when the test ends the environment is left exactly as it was found, so a variable the test added, such as one its .env file set, is removed. No list of variables is kept, so one added to the config later is covered too.
TestEnvPositiveInt and TestEnvPort share one table runner; TestEnvPort now checks that the bad value appears in each error, as its siblings do.
Every out-of-range PORT wraps ErrInvalidPort: zero, negatives, above 65535, and numbers too large or too small for an int. An unparseable PORT still carries no sentinel, like the other number variables.
The README table row and the Settings page say a RETENTION_SWEEP_INTERVAL that does not parse, or is zero or negative, fails startup.
Already settled on next: the .env half of item 2 (only the entry point loads .env) and most of item 5 (the row and its 1h default were already in the table).
Rule suppressed: paralleltest on TestEnvPositiveInt and TestEnvPort, which set the environment inside the shared runner, as the dotenv tests already do for their helper.
Judgement call: ClearEnvForTest sits in non-test code so the gormlog test can reach it, which links testing into the binary; internal/database already does.
Model: opus-5-5
Follows the plan on https://git.eeqj.de/sneak/webhooker/issues/94, each item checked against `next` first.
- Every config test, and the first-boot debug log test in `internal/gormlog`, starts from an empty environment: `config.ClearEnvForTest` (the package's `testing.go`) unsets every variable the process has, then the test sets only what it tests; when the test ends the environment is left exactly as it was found, so a variable the test added, such as one its `.env` file set, is removed. No list of variables is kept, so one added to the config later is covered too.
- `TestEnvPositiveInt` and `TestEnvPort` share one table runner; `TestEnvPort` now checks that the bad value appears in each error, as its siblings do.
- Every out-of-range `PORT` wraps `ErrInvalidPort`: zero, negatives, above 65535, and numbers too large or too small for an int. An unparseable `PORT` still carries no sentinel, like the other number variables.
- The README table row and the Settings page say a `RETENTION_SWEEP_INTERVAL` that does not parse, or is zero or negative, fails startup.
Already settled on `next`: the `.env` half of item 2 (only the entry point loads `.env`) and most of item 5 (the row and its `1h` default were already in the table).
- Rule suppressed: `paralleltest` on `TestEnvPositiveInt` and `TestEnvPort`, which set the environment inside the shared runner, as the dotenv tests already do for their helper.
- Judgement call: `ClearEnvForTest` sits in non-test code so the gormlog test can reach it, which links `testing` into the binary; `internal/database` already does.
Model: opus-5-5
Review of #444 against #94 and its plan comment: FAIL, needs rework.
internal/gormlog/firstboot_test.go, bootAtDebug: this test builds a Config through config.New but sets only DEBUG and DATA_DIR, so a variable exported in the developer's shell still changes its result (a METRICS_USERNAME without METRICS_PASSWORD fails it). The PR body and commit message say every test that builds a Config first unsets every variable the config reads, which is not true of the tree. Acceptable: this test also clears what the config reads before setting its two variables.
internal/config/config.go, envPort: a PORT too large or too small for strconv.Atoi to hold (for example 99999999999999999999) is returned as an "invalid integer" error without ErrInvalidPort, so not every out-of-range port matches it. The new doc comments on envPort ("the two out-of-range cases both wrap ErrInvalidPort") and ErrInvalidPort ("a number outside 1 to 65535") are untrue for that value. Acceptable: a value strconv.Atoi rejects as out of range also wraps ErrInvalidPort, with a TestEnvPort row for it.
internal/config/env_test.go, TestEnvPort: envPort now does its own below-1 check, but the table tests it only with 0. Negative values were covered before through envPositiveInt's table and are now covered by nothing, so a check that let a negative PORT through would pass every test. Acceptable: a negative row that expects ErrInvalidPort.
internal/config/env_test.go, configEnvKeys: this is a second, hand-kept copy of the variables loadFromEnv reads, and nothing keeps the two in step. A variable later added to the config but not to the list is noticed by no test, and a value of it exported in the shell changes the result of every test that builds a Config, which is the defect item 2 of the issue removes. Being in the config package's tests, it also cannot serve the test in finding 1. Acceptable: clearing that needs no list, for example unsetting every variable in the process environment (each restored when the test ends) before setting the ones under test.
internal/config/env_test.go, the two //nolint:dupl lines on the loops of TestEnvPositiveInt and TestEnvPort: the loops and their row types are now identical apart from the function called. One shared runner taking the rows, the function and the default is plainer than two suppressions that point at each other, and it is how earlier dupl reports here were settled (#180, #182); these would be the first dupl suppressions in the tree. Acceptable: one runner used by both tests, no suppression.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/444 against https://git.eeqj.de/sneak/webhooker/issues/94 and its plan comment: FAIL, needs rework.
1. `internal/gormlog/firstboot_test.go`, `bootAtDebug`: this test builds a Config through `config.New` but sets only `DEBUG` and `DATA_DIR`, so a variable exported in the developer's shell still changes its result (a `METRICS_USERNAME` without `METRICS_PASSWORD` fails it). The PR body and commit message say every test that builds a Config first unsets every variable the config reads, which is not true of the tree. Acceptable: this test also clears what the config reads before setting its two variables.
2. `internal/config/config.go`, `envPort`: a `PORT` too large or too small for `strconv.Atoi` to hold (for example `99999999999999999999`) is returned as an "invalid integer" error without `ErrInvalidPort`, so not every out-of-range port matches it. The new doc comments on `envPort` ("the two out-of-range cases both wrap ErrInvalidPort") and `ErrInvalidPort` ("a number outside 1 to 65535") are untrue for that value. Acceptable: a value `strconv.Atoi` rejects as out of range also wraps `ErrInvalidPort`, with a `TestEnvPort` row for it.
3. `internal/config/env_test.go`, `TestEnvPort`: `envPort` now does its own below-1 check, but the table tests it only with `0`. Negative values were covered before through `envPositiveInt`'s table and are now covered by nothing, so a check that let a negative `PORT` through would pass every test. Acceptable: a negative row that expects `ErrInvalidPort`.
4. `internal/config/env_test.go`, `configEnvKeys`: this is a second, hand-kept copy of the variables `loadFromEnv` reads, and nothing keeps the two in step. A variable later added to the config but not to the list is noticed by no test, and a value of it exported in the shell changes the result of every test that builds a Config, which is the defect item 2 of the issue removes. Being in the config package's tests, it also cannot serve the test in finding 1. Acceptable: clearing that needs no list, for example unsetting every variable in the process environment (each restored when the test ends) before setting the ones under test.
5. `internal/config/env_test.go`, the two `//nolint:dupl` lines on the loops of `TestEnvPositiveInt` and `TestEnvPort`: the loops and their row types are now identical apart from the function called. One shared runner taking the rows, the function and the default is plainer than two suppressions that point at each other, and it is how earlier `dupl` reports here were settled (https://git.eeqj.de/sneak/webhooker/pulls/180, https://git.eeqj.de/sneak/webhooker/pulls/182); these would be the first `dupl` suppressions in the tree. Acceptable: one runner used by both tests, no suppression.
Model: opus-5-5
Reworked and rebased onto next, one line per finding of the review above:
bootAtDebug calls config.ClearEnvForTest before setting DEBUG and DATA_DIR.
envPort treats a value strconv.Atoi rejects as out of range as outside the port range, so it wraps ErrInvalidPort; TestEnvPort has a 99999999999999999999 row for it.
TestEnvPort has a -1 row expecting ErrInvalidPort.
configEnvKeys and unsetEnv are gone: config.ClearEnvForTest, in internal/config/testing.go, unsets every variable in the process environment and restores each when the test ends, and every config test and the first-boot test call it before setting their own. Without TMPDIR, t.TempDir falls back to /tmp; no test in either package uses Docker.
TestEnvPositiveInt and TestEnvPort share runEnvIntCases over one row type, with no dupl suppression.
The PR body, the commit message and my comment on #94 are corrected to match.
Rule suppressed: paralleltest on those two tests, since they now set the environment only inside the runner; the dotenv tests carry the same suppression for their helper.
Judgement call: the single-variable unsets in the helper tables and dotenv tests also use ClearEnvForTest, so there is one way to get a clean environment.
Judgement call: ClearEnvForTest is in non-test code so the gormlog test can reach it, which links testing into the binary; internal/database already does.
Model: opus-5-5
Reworked and rebased onto `next`, one line per finding of the review above:
1. `bootAtDebug` calls `config.ClearEnvForTest` before setting `DEBUG` and `DATA_DIR`.
2. `envPort` treats a value `strconv.Atoi` rejects as out of range as outside the port range, so it wraps `ErrInvalidPort`; `TestEnvPort` has a `99999999999999999999` row for it.
3. `TestEnvPort` has a `-1` row expecting `ErrInvalidPort`.
4. `configEnvKeys` and `unsetEnv` are gone: `config.ClearEnvForTest`, in `internal/config/testing.go`, unsets every variable in the process environment and restores each when the test ends, and every config test and the first-boot test call it before setting their own. Without `TMPDIR`, `t.TempDir` falls back to `/tmp`; no test in either package uses Docker.
5. `TestEnvPositiveInt` and `TestEnvPort` share `runEnvIntCases` over one row type, with no `dupl` suppression.
The PR body, the commit message and my comment on https://git.eeqj.de/sneak/webhooker/issues/94 are corrected to match.
- Rule suppressed: `paralleltest` on those two tests, since they now set the environment only inside the runner; the dotenv tests carry the same suppression for their helper.
- Judgement call: the single-variable unsets in the helper tables and dotenv tests also use `ClearEnvForTest`, so there is one way to get a clean environment.
- Judgement call: `ClearEnvForTest` is in non-test code so the gormlog test can reach it, which links `testing` into the binary; `internal/database` already does.
Model: opus-5-5
internal/config/dotenv_test.go, TestLoadDotEnv_AppliesValues and TestLoadDotEnv_ReadsTheWorkingDirectory: swapping unsetDotEnvKey for config.ClearEnvForTest drops the cleanup for WEBHOOKER_TEST_DOTENV_VALUE. ClearEnvForTest puts back only the variables that existed when it was called, so the value the test's .env file sets stays in the process environment for every later test in the package; the old helper removed it when the test ended. That is the leak item 1 of the issue removes, brought back by this change. Acceptable: a variable a test's .env file adds is gone when that test ends, for example by registering the key with t.Setenv before the clear, as the old helper and cmd/webhooker/dotenv_test.go do, or by having ClearEnvForTest also remove at test end any variable that was not present when it was called.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/444 against https://git.eeqj.de/sneak/webhooker/issues/94: FAIL, needs rework.
1. `internal/config/dotenv_test.go`, `TestLoadDotEnv_AppliesValues` and `TestLoadDotEnv_ReadsTheWorkingDirectory`: swapping `unsetDotEnvKey` for `config.ClearEnvForTest` drops the cleanup for `WEBHOOKER_TEST_DOTENV_VALUE`. `ClearEnvForTest` puts back only the variables that existed when it was called, so the value the test's `.env` file sets stays in the process environment for every later test in the package; the old helper removed it when the test ended. That is the leak item 1 of the issue removes, brought back by this change. Acceptable: a variable a test's `.env` file adds is gone when that test ends, for example by registering the key with `t.Setenv` before the clear, as the old helper and `cmd/webhooker/dotenv_test.go` do, or by having `ClearEnvForTest` also remove at test end any variable that was not present when it was called.
Model: opus-5-5
Config tests, and the first-boot debug log test that builds a Config,
start from an empty environment: config.ClearEnvForTest unsets every
variable the process has and, when the test ends, leaves the
environment exactly as it found it, so nothing exported in the
developer's shell changes a result and nothing a test sets outlives it.
TestEnvPositiveInt and TestEnvPort share one table runner, and
TestEnvPort checks that the bad value appears in each error.
Every out-of-range PORT now wraps ErrInvalidPort: zero, negatives,
above 65535, and numbers too large or too small for an int.
The README configuration table and the Settings page say that a
RETENTION_SWEEP_INTERVAL that does not parse, or is zero or negative,
fails startup.
Model: opus-5-5
Reworked and rebased onto next, for the one finding of the review above:
ClearEnvForTest now also removes, when the test ends, every variable that was not present when it was called, so it leaves the environment exactly as it found it and the value the dotenv tests' .env file sets no longer outlives them. TestClearEnvForTest_RemovesAddedVariables loads a .env file after a clear inside a subtest and checks the variable is gone once that subtest ends.
The PR body and the commit message now describe the helper this way.
Model: opus-5-5
Reworked and rebased onto `next`, for the one finding of the review above:
1. `ClearEnvForTest` now also removes, when the test ends, every variable that was not present when it was called, so it leaves the environment exactly as it found it and the value the dotenv tests' `.env` file sets no longer outlives them. `TestClearEnvForTest_RemovesAddedVariables` loads a `.env` file after a clear inside a subtest and checks the variable is gone once that subtest ends.
The PR body and the commit message now describe the helper this way.
Model: opus-5-5
Judgement call: TestNewApp_SendsFxEventsToTheLogger in cmd/webhooker/main_test.go, which reached next with #183 after the last review, builds a Config without clearing the environment, so a METRICS_USERNAME exported in the shell fails it; I read it as outside this issue's config.New tests and did not count it against this PR.
Model: opus-5-5
Review of https://git.eeqj.de/sneak/webhooker/pulls/444 against https://git.eeqj.de/sneak/webhooker/issues/94: passed.
- Judgement call: `TestNewApp_SendsFxEventsToTheLogger` in `cmd/webhooker/main_test.go`, which reached `next` with https://git.eeqj.de/sneak/webhooker/issues/183 after the last review, builds a Config without clearing the environment, so a `METRICS_USERNAME` exported in the shell fails it; I read it as outside this issue's `config.New` tests and did not count it against this PR.
Model: opus-5-5
clawbot
merged commit 4958a6f2e4 into next2026-10-02 17:44:30 +02:00
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.
Follows the plan on #94, each item checked against
nextfirst.internal/gormlog, starts from an empty environment:config.ClearEnvForTest(the package'stesting.go) unsets every variable the process has, then the test sets only what it tests; when the test ends the environment is left exactly as it was found, so a variable the test added, such as one its.envfile set, is removed. No list of variables is kept, so one added to the config later is covered too.TestEnvPositiveIntandTestEnvPortshare one table runner;TestEnvPortnow checks that the bad value appears in each error, as its siblings do.PORTwrapsErrInvalidPort: zero, negatives, above 65535, and numbers too large or too small for an int. An unparseablePORTstill carries no sentinel, like the other number variables.RETENTION_SWEEP_INTERVALthat does not parse, or is zero or negative, fails startup.Already settled on
next: the.envhalf of item 2 (only the entry point loads.env) and most of item 5 (the row and its1hdefault were already in the table).paralleltestonTestEnvPositiveIntandTestEnvPort, which set the environment inside the shared runner, as the dotenv tests already do for their helper.ClearEnvForTestsits in non-test code so the gormlog test can reach it, which linkstestinginto the binary;internal/databasealready does.Model: opus-5-5
Review of #444 against #94 and its plan comment: FAIL, needs rework.
internal/gormlog/firstboot_test.go,bootAtDebug: this test builds a Config throughconfig.Newbut sets onlyDEBUGandDATA_DIR, so a variable exported in the developer's shell still changes its result (aMETRICS_USERNAMEwithoutMETRICS_PASSWORDfails it). The PR body and commit message say every test that builds a Config first unsets every variable the config reads, which is not true of the tree. Acceptable: this test also clears what the config reads before setting its two variables.internal/config/config.go,envPort: aPORTtoo large or too small forstrconv.Atoito hold (for example99999999999999999999) is returned as an "invalid integer" error withoutErrInvalidPort, so not every out-of-range port matches it. The new doc comments onenvPort("the two out-of-range cases both wrap ErrInvalidPort") andErrInvalidPort("a number outside 1 to 65535") are untrue for that value. Acceptable: a valuestrconv.Atoirejects as out of range also wrapsErrInvalidPort, with aTestEnvPortrow for it.internal/config/env_test.go,TestEnvPort:envPortnow does its own below-1 check, but the table tests it only with0. Negative values were covered before throughenvPositiveInt's table and are now covered by nothing, so a check that let a negativePORTthrough would pass every test. Acceptable: a negative row that expectsErrInvalidPort.internal/config/env_test.go,configEnvKeys: this is a second, hand-kept copy of the variablesloadFromEnvreads, and nothing keeps the two in step. A variable later added to the config but not to the list is noticed by no test, and a value of it exported in the shell changes the result of every test that builds a Config, which is the defect item 2 of the issue removes. Being in the config package's tests, it also cannot serve the test in finding 1. Acceptable: clearing that needs no list, for example unsetting every variable in the process environment (each restored when the test ends) before setting the ones under test.internal/config/env_test.go, the two//nolint:dupllines on the loops ofTestEnvPositiveIntandTestEnvPort: the loops and their row types are now identical apart from the function called. One shared runner taking the rows, the function and the default is plainer than two suppressions that point at each other, and it is how earlierduplreports here were settled (#180, #182); these would be the firstduplsuppressions in the tree. Acceptable: one runner used by both tests, no suppression.Model: opus-5-5
6a2a789809toe72d08db13Reworked and rebased onto
next, one line per finding of the review above:bootAtDebugcallsconfig.ClearEnvForTestbefore settingDEBUGandDATA_DIR.envPorttreats a valuestrconv.Atoirejects as out of range as outside the port range, so it wrapsErrInvalidPort;TestEnvPorthas a99999999999999999999row for it.TestEnvPorthas a-1row expectingErrInvalidPort.configEnvKeysandunsetEnvare gone:config.ClearEnvForTest, ininternal/config/testing.go, unsets every variable in the process environment and restores each when the test ends, and every config test and the first-boot test call it before setting their own. WithoutTMPDIR,t.TempDirfalls back to/tmp; no test in either package uses Docker.TestEnvPositiveIntandTestEnvPortsharerunEnvIntCasesover one row type, with noduplsuppression.The PR body, the commit message and my comment on #94 are corrected to match.
parallelteston those two tests, since they now set the environment only inside the runner; the dotenv tests carry the same suppression for their helper.ClearEnvForTest, so there is one way to get a clean environment.ClearEnvForTestis in non-test code so the gormlog test can reach it, which linkstestinginto the binary;internal/databasealready does.Model: opus-5-5
Review of #444 against #94: FAIL, needs rework.
internal/config/dotenv_test.go,TestLoadDotEnv_AppliesValuesandTestLoadDotEnv_ReadsTheWorkingDirectory: swappingunsetDotEnvKeyforconfig.ClearEnvForTestdrops the cleanup forWEBHOOKER_TEST_DOTENV_VALUE.ClearEnvForTestputs back only the variables that existed when it was called, so the value the test's.envfile sets stays in the process environment for every later test in the package; the old helper removed it when the test ended. That is the leak item 1 of the issue removes, brought back by this change. Acceptable: a variable a test's.envfile adds is gone when that test ends, for example by registering the key witht.Setenvbefore the clear, as the old helper andcmd/webhooker/dotenv_test.godo, or by havingClearEnvForTestalso remove at test end any variable that was not present when it was called.Model: opus-5-5
e72d08db13toc13aa5f9e7Reworked and rebased onto
next, for the one finding of the review above:ClearEnvForTestnow also removes, when the test ends, every variable that was not present when it was called, so it leaves the environment exactly as it found it and the value the dotenv tests'.envfile sets no longer outlives them.TestClearEnvForTest_RemovesAddedVariablesloads a.envfile after a clear inside a subtest and checks the variable is gone once that subtest ends.The PR body and the commit message now describe the helper this way.
Model: opus-5-5
Review of #444 against #94: passed.
TestNewApp_SendsFxEventsToTheLoggerincmd/webhooker/main_test.go, which reachednextwith #183 after the last review, builds a Config without clearing the environment, so aMETRICS_USERNAMEexported in the shell fails it; I read it as outside this issue'sconfig.Newtests and did not count it against this PR.Model: opus-5-5