Follow-ups from the PR #92 review: config test hygiene and env docs #94

New Issue

clawbot · 2026-08-09T04:08:12+02:00

clawbot commented

2026-08-09 04:08:12 +02:00

Tracking issue for the non-blocking nits raised in the independent review of PR #92 (#80), so they do not become untracked. None blocked that merge.

Items

os.Unsetenv without restore. internal/config/env_test.go calls os.Unsetenv directly to simulate an unset variable. Unlike t.Setenv, that does not restore the prior value at test end, so it leaks unset state into later tests in the same binary. It exactly mirrors the pre-existing pattern in internal/config/config_test.go, which is why it was not a blocker. Fix both: capture and restore via t.Cleanup, or add a small helper that does.
config.New tests do not neutralise ambient environment. The config.New-level tests set only the variable under test. A developer with, say, DEBUG or PORT exported in their shell (or a .env file — note godotenv/autoload is imported by the config package) could get different results locally than CI. Explicitly clear the full set of variables the config reads at the start of those tests.
TestEnvPort error assertions are weaker than its siblings. The other helper tables assert the offending value appears in the error message; the envPort error branch only checks the sentinel via errors.Is. Make it consistent — the whole point of #80 is that the operator can see which key and which value were wrong.
ErrInvalidPort covers only the above-range case. Below-range values (0, negatives) surface as ErrNonPositiveValue from the inner envPositiveInt. Both correctly abort startup, so this is cosmetic, but a caller matching on ErrInvalidPort to mean "bad port" will miss half the bad ports. Either route both bounds through ErrInvalidPort or document the split.
README.md (~line 104) cites RETENTION_SWEEP_INTERVAL in prose but the variable is absent from the configuration table. Add it, with its default and the note that a set-but-unparseable value aborts startup.

Definition of done

Items 1-3 fixed; item 4 either fixed or documented; item 5 added to the table.
make check green via the repo's own entrypoints; .golangci.yml untouched.

Tracking issue for the non-blocking nits raised in the independent review of PR #92 (#80), so they do not become untracked. None blocked that merge. ## Items 1. **`os.Unsetenv` without restore.** `internal/config/env_test.go` calls `os.Unsetenv` directly to simulate an unset variable. Unlike `t.Setenv`, that does not restore the prior value at test end, so it leaks unset state into later tests in the same binary. It exactly mirrors the pre-existing pattern in `internal/config/config_test.go`, which is why it was not a blocker. Fix both: capture and restore via `t.Cleanup`, or add a small helper that does. 2. **`config.New` tests do not neutralise ambient environment.** The `config.New`-level tests set only the variable under test. A developer with, say, `DEBUG` or `PORT` exported in their shell (or a `.env` file — note `godotenv/autoload` is imported by the config package) could get different results locally than CI. Explicitly clear the full set of variables the config reads at the start of those tests. 3. **`TestEnvPort` error assertions are weaker than its siblings.** The other helper tables assert the offending value appears in the error message; the `envPort` error branch only checks the sentinel via `errors.Is`. Make it consistent — the whole point of #80 is that the operator can see which key and which value were wrong. 4. **`ErrInvalidPort` covers only the above-range case.** Below-range values (`0`, negatives) surface as `ErrNonPositiveValue` from the inner `envPositiveInt`. Both correctly abort startup, so this is cosmetic, but a caller matching on `ErrInvalidPort` to mean "bad port" will miss half the bad ports. Either route both bounds through `ErrInvalidPort` or document the split. 5. **`README.md` (~line 104) cites `RETENTION_SWEEP_INTERVAL`** in prose but the variable is absent from the configuration table. Add it, with its default and the note that a set-but-unparseable value aborts startup. ## Definition of done - Items 1-3 fixed; item 4 either fixed or documented; item 5 added to the table. - `make check` green via the repo's own entrypoints; `.golangci.yml` untouched.

clawbot referenced this issue

2026-08-09 04:08:25 +02:00

Fail loudly on set-but-unparseable env config values (closes #80) #92

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#94