sneak/netwatch
1
0
Fork 0
Code Issues 26 Pull Requests 4 Actions Packages Projects Releases Wiki Activity

make test is effectively a no-op: Go tests are compile stubs, frontend has no test framework #21

New Issue
Open
opened 2026-08-09 03:40:39 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-09 03:40:39 +02:00
Collaborator
Copy Link
Copy Source

Problem

REPO_POLICIES.md: "All repos with software must have tests that run via the platform-standard test framework... There is no excuse for make test to be a no-op." Both halves of this repo fail that. Verified on main at fbfe1df.

Frontend: no test framework at all

script/test in its entirety:

timeout 30 yarn build

The script's own comment admits it: "This repo has no unit tests; the production build serves as the test (fails on broken code)." A production bundle asserts nothing about behaviour — it proves the code parses, nothing more.

  • package.json devDependencies contain no test runner: no vitest, jest, mocha, or @testing-library/*.
  • package.json scripts has only dev, build, preview — no test script. CODE_STYLEGUIDE_JS.md requires that yarn run test and npm run test both work; today both fail.
  • Zero *.test.js / *.spec.js / __tests__ files exist.

Backend: both test files are compile-check stubs

backend/internal/handlers/handlers_test.go and backend/internal/reportbuf/reportbuf_test.go are 13 lines each and identical in shape:

func TestImport(t *testing.T) {
    t.Parallel()
    // Compilation check — verifies the package parses
    // and all imports resolve.
}

They assert nothing. Coverage by package:

cmd/netwatch-server      [no test files]
internal/config          [no test files]
internal/globals         [no test files]
internal/handlers        stub only
internal/healthcheck     [no test files]
internal/logger          [no test files]
internal/middleware      [no test files]
internal/reportbuf       stub only
internal/server          [no test files]

internal/reportbuf is 199 lines of buffered zstd compression and flushing — the most logic-dense and most failure-prone package in the repo — and has zero behavioural tests.

make test also lacks the mandated flags and rerun pattern

backend/Makefile: test: timeout 30 go test ./...

  • No -race. No -cover.

  • No conditional-verbose-rerun. REPO_POLICIES.md requires:

    test:
	@go test -timeout 30s -race -cover ./... || \
		{ echo "--- Rerunning with -v for details ---"; \
		  go test -timeout 30s -race -v ./...; exit 1; }

    Grepping the repo for Rerunning returns zero hits — the pattern is absent from both the Go and the frontend side.

Definition of done

  • internal/reportbuf has real behavioural tests: append then read back round-trips correctly; zstd output actually decompresses to the input; flush/rotation behaves at the buffer boundary; concurrent Append is safe under -race; error paths (unwritable target) are handled without panicking.
  • internal/handlers has real tests using httptest: HandleReport returns 200 on a valid body; returns 400 on malformed JSON; rejects a body over maxReportBodyBytes; HandleHealthCheck returns the documented JSON shape with Content-Type: application/json.
  • internal/config has a test proving env-var override and default precedence.
  • The two TestImport stubs are deleted or replaced — a compile check that duplicates what go build already proves is not a test.
  • A JS test framework is added and wired up. vitest is the default choice — it is the standard runner for a Vite project, shares the Vite config, and needs no separate build pipeline. Do not pick a niche alternative.
  • package.json gains a test script so yarn run test works.
  • Frontend tests cover the pure logic that is currently untested and easy to get wrong: humanDuration formatting, the latency-to-colour threshold functions, HostState min/max/average/median over a history buffer including the empty and all-unreachable cases, and the health-state classifier's four states.
    • Note: src/main.js calls init() at import time, which makes the module hard to import under test. Extracting the pure helpers into importable modules is in scope insofar as it is needed to make them testable; a full modularization of the 1262-line file is not in scope here and is tracked separately.
  • Both make test implementations use the conditional-verbose-rerun pattern from REPO_POLICIES.md, and the Go one adds -race and -cover.
  • make test completes in under 20 seconds with a 30-second timeout enforced, on both sides.
  • Root make check and cd backend && make check both pass.
  • README.md's TODO no longer lists "Add unit tests"; the Entrypoints description of script/test is corrected — it currently describes running the build as the test.
  • TODO.md updated in the same commit.
  • Commit title ends with (closes #N).

Implementation requirements

  • Tests must assert real behaviour. A test that only checks a function does not panic is not acceptable here — that is the stub problem this issue exists to fix.
  • Use table-driven tests on the Go side, per Go convention.
  • Any new dependency must be checked against the Go/JS package defaults before being added. vitest for JS; on the Go side prefer stdlib testing plus net/http/httptest and do not add an assertion library unless there is a clear reason.
  • Tests may use temporary directories but make check must not modify tracked files.
  • This issue is large. If the implementer finds it does not fit one commit-sized change, split it into a Go half and a frontend half, open both PRs, and note the split on this issue — do not silently deliver a partial fix.
  • make targets and script/ entrypoints only.
  • No attribution trailers in the commit message.
## Problem `REPO_POLICIES.md`: "All repos with software must have tests that run via the platform-standard test framework... **There is no excuse for `make test` to be a no-op.**" Both halves of this repo fail that. Verified on `main` at `fbfe1df`. ### Frontend: no test framework at all `script/test` in its entirety: ```sh timeout 30 yarn build ``` The script's own comment admits it: "This repo has no unit tests; the production build serves as the test (fails on broken code)." A production bundle asserts nothing about behaviour — it proves the code parses, nothing more. - `package.json` devDependencies contain no test runner: no `vitest`, `jest`, `mocha`, or `@testing-library/*`. - `package.json` `scripts` has only `dev`, `build`, `preview` — **no `test` script**. `CODE_STYLEGUIDE_JS.md` requires that `yarn run test` and `npm run test` both work; today both fail. - Zero `*.test.js` / `*.spec.js` / `__tests__` files exist. ### Backend: both test files are compile-check stubs `backend/internal/handlers/handlers_test.go` and `backend/internal/reportbuf/reportbuf_test.go` are 13 lines each and identical in shape: ```go func TestImport(t *testing.T) { t.Parallel() // Compilation check — verifies the package parses // and all imports resolve. } ``` They assert nothing. Coverage by package: ``` cmd/netwatch-server [no test files] internal/config [no test files] internal/globals [no test files] internal/handlers stub only internal/healthcheck [no test files] internal/logger [no test files] internal/middleware [no test files] internal/reportbuf stub only internal/server [no test files] ``` `internal/reportbuf` is 199 lines of buffered zstd compression and flushing — the most logic-dense and most failure-prone package in the repo — and has zero behavioural tests. ### `make test` also lacks the mandated flags and rerun pattern `backend/Makefile`: `test: timeout 30 go test ./...` - No `-race`. No `-cover`. - No conditional-verbose-rerun. `REPO_POLICIES.md` requires: ```makefile test: @go test -timeout 30s -race -cover ./... || \ { echo "--- Rerunning with -v for details ---"; \ go test -timeout 30s -race -v ./...; exit 1; } ``` Grepping the repo for `Rerunning` returns zero hits — the pattern is absent from both the Go and the frontend side. ## Definition of done - [ ] `internal/reportbuf` has real behavioural tests: append then read back round-trips correctly; zstd output actually decompresses to the input; flush/rotation behaves at the buffer boundary; concurrent `Append` is safe under `-race`; error paths (unwritable target) are handled without panicking. - [ ] `internal/handlers` has real tests using `httptest`: `HandleReport` returns 200 on a valid body; returns 400 on malformed JSON; rejects a body over `maxReportBodyBytes`; `HandleHealthCheck` returns the documented JSON shape with `Content-Type: application/json`. - [ ] `internal/config` has a test proving env-var override and default precedence. - [ ] The two `TestImport` stubs are deleted or replaced — a compile check that duplicates what `go build` already proves is not a test. - [ ] A JS test framework is added and wired up. **`vitest` is the default choice** — it is the standard runner for a Vite project, shares the Vite config, and needs no separate build pipeline. Do not pick a niche alternative. - [ ] `package.json` gains a `test` script so `yarn run test` works. - [ ] Frontend tests cover the pure logic that is currently untested and easy to get wrong: `humanDuration` formatting, the latency-to-colour threshold functions, `HostState` min/max/average/median over a history buffer including the empty and all-unreachable cases, and the health-state classifier's four states. - Note: `src/main.js` calls `init()` at import time, which makes the module hard to import under test. Extracting the pure helpers into importable modules is in scope insofar as it is needed to make them testable; a full modularization of the 1262-line file is **not** in scope here and is tracked separately. - [ ] Both `make test` implementations use the conditional-verbose-rerun pattern from `REPO_POLICIES.md`, and the Go one adds `-race` and `-cover`. - [ ] `make test` completes in under 20 seconds with a 30-second timeout enforced, on both sides. - [ ] Root `make check` and `cd backend && make check` both pass. - [ ] `README.md`'s TODO no longer lists "Add unit tests"; the **Entrypoints** description of `script/test` is corrected — it currently describes running the build as the test. - [ ] `TODO.md` updated in the same commit. - [ ] Commit title ends with ` (closes #N)`. ## Implementation requirements - Tests must assert real behaviour. A test that only checks a function does not panic is not acceptable here — that is the stub problem this issue exists to fix. - Use table-driven tests on the Go side, per Go convention. - Any new dependency must be checked against the Go/JS package defaults before being added. `vitest` for JS; on the Go side prefer stdlib `testing` plus `net/http/httptest` and do not add an assertion library unless there is a clear reason. - Tests may use temporary directories but `make check` must not modify tracked files. - This issue is large. If the implementer finds it does not fit one commit-sized change, split it into a Go half and a frontend half, open both PRs, and note the split on this issue — do not silently deliver a partial fix. - `make` targets and `script/` entrypoints only. - No attribution trailers in the commit message.
clawbot added this to the 1.0.0 milestone 2026-08-09 03:40:39 +02:00
clawbot commented 2026-08-09 04:26:09 +02:00
Author
Collaborator
Copy Link
Copy Source

Additional scope: the 30s test timeout is a real Docker-gate flake, not just a policy gap

Surfaced during the review of PR #31 and worth folding into this issue, because it turns an abstract compliance item into a concrete "main always green" problem.

A reviewer's first cold docker build -f Dockerfile.backend . at head 4d70317 died here:

make: *** [Makefile:35: test] Terminated

That is backend/Makefile's timeout 30 go test ./.... The cause is an empty Go build cache in a fresh container — compilation alone ate the budget before any test ran. The immediate retry, with a warm cache, compiled in 11 seconds and went green.

So the Docker gate can fail for reasons that have nothing to do with the code, on exactly the cold-cache path CI takes most often. That is a false red against the "main must always pass make check" policy, and false reds are corrosive: they train people to re-run rather than read.

What this changes about this issue

The existing definition of done already requires adopting the conditional-verbose-rerun pattern and -race -cover. Add to it:

  • The test timeout survives a cold container build cache. REPO_POLICIES.md requires make test to complete in under 20 seconds with a 30-second timeout — but that budget is about test execution, not first-compile. Either raise the timeout, or arrange for the build stage to have compiled the packages before make test runs, so the timeout measures what it is meant to measure.
  • Verify by pruning the Docker builder cache and running docker build -f Dockerfile.backend . three times running, all green. One passing run proves nothing here — the flake only appears cold.

Note the interaction with -race: adding it will make compilation slower, not faster. Whatever budget is chosen must be validated with -race enabled, not before.

Also note that timeout 30 uses the external timeout(1) binary, which does not exist on stock macOS. Go's own -timeout 30s flag is the portable way to express this and is what REPO_POLICIES.md's Go example uses. Switching to it fixes the portability problem and makes the timeout apply to test execution rather than to compilation-plus-execution — which is very likely the correct fix for the flake as well.

## Additional scope: the 30s test timeout is a real Docker-gate flake, not just a policy gap Surfaced during the review of PR #31 and worth folding into this issue, because it turns an abstract compliance item into a concrete "main always green" problem. A reviewer's **first cold** `docker build -f Dockerfile.backend .` at head `4d70317` died here: ``` make: *** [Makefile:35: test] Terminated ``` That is `backend/Makefile`'s `timeout 30 go test ./...`. The cause is an empty Go build cache in a fresh container — compilation alone ate the budget before any test ran. The immediate retry, with a warm cache, compiled in 11 seconds and went green. So the Docker gate can fail for reasons that have nothing to do with the code, on exactly the cold-cache path CI takes most often. That is a false red against the "`main` must always pass `make check`" policy, and false reds are corrosive: they train people to re-run rather than read. ### What this changes about this issue The existing definition of done already requires adopting the conditional-verbose-rerun pattern and `-race -cover`. Add to it: - [ ] The test timeout survives a **cold** container build cache. `REPO_POLICIES.md` requires `make test` to complete in under 20 seconds with a 30-second timeout — but that budget is about test execution, not first-compile. Either raise the timeout, or arrange for the build stage to have compiled the packages before `make test` runs, so the timeout measures what it is meant to measure. - [ ] Verify by pruning the Docker builder cache and running `docker build -f Dockerfile.backend .` **three times running**, all green. One passing run proves nothing here — the flake only appears cold. Note the interaction with `-race`: adding it will make compilation slower, not faster. Whatever budget is chosen must be validated with `-race` enabled, not before. Also note that `timeout 30` uses the external `timeout(1)` binary, which does not exist on stock macOS. Go's own `-timeout 30s` flag is the portable way to express this and is what `REPO_POLICIES.md`'s Go example uses. Switching to it fixes the portability problem and makes the timeout apply to test execution rather than to compilation-plus-execution — which is very likely the correct fix for the flake as well.
clawbot commented 2026-08-09 11:56:45 +02:00
Author
Collaborator
Copy Link
Copy Source

Correction: the right fix is Go's -timeout flag, not a bigger budget

My earlier comment on this issue offered two options for the cold-cache flake — "either raise the timeout, or arrange for the build stage to have compiled the packages before make test runs." Both were worse than the obvious answer, and I want to supersede that guidance before someone implements it.

The distinction I missed, raised by another repo in the org:

  • Shell timeout 30 go test ./... — what this repo uses (backend/Makefile:25, and carried into backend/script/test on PR #38's branch) — wraps the whole invocation, so the budget covers compilation plus execution. On an empty Go build cache in a fresh container, compilation alone can consume it before a single test runs. That is exactly the failure observed: make: *** [Makefile:35: test] Terminated, then a warm-cache retry passing in 11s.
  • Go's own -timeout 30s flag bounds test execution only. Compilation is not counted, so a cold cache cannot trip it.

So the exposure is not universal across the org's Go repos — it is specific to the shell-wrapper form, which this repo has.

Revised fix

Convert the shell wrapper to Go's flag. This is a one-line change and it fixes three things at once:

  1. The flake. The budget stops measuring compilation, so a cold container cache is no longer a failure mode.
  2. Portability. timeout(1) is coreutils and does not exist on stock macOS. -timeout is part of the toolchain and works everywhere.
  3. Policy alignment. REPO_POLICIES.md's Go example is go test -timeout 30s -race -cover ./... — the flag form, not the wrapper. The existing definition of done already requires adopting that exact line, so this converges rather than adding scope.

Do not raise the budget. A larger number would mask the flake rather than fix it, and would weaken the real 20-second execution ceiling the policy is trying to enforce.

Amended definition of done

Replacing the two bullets I added earlier:

  • make test uses Go's -timeout 30s flag. The external timeout(1) wrapper is gone from both backend/Makefile and backend/script/test.
  • Verified against a cold Go build cache — prune the container build cache for that one build (docker build --no-cache, never docker builder prune, which is shared state on this host) and confirm the gate passes. Do this with -race enabled, since race instrumentation makes compilation slower, not faster.

Note for whoever implements: PR #38 relocates the backend test implementation from backend/Makefile into backend/script/test, carrying the shell-wrapper form with it. Read the tree as it exists at implementation time; if #38 has landed, the line to change is in backend/script/test.

## Correction: the right fix is Go's `-timeout` flag, not a bigger budget My earlier comment on this issue offered two options for the cold-cache flake — "either raise the timeout, or arrange for the build stage to have compiled the packages before `make test` runs." **Both were worse than the obvious answer**, and I want to supersede that guidance before someone implements it. The distinction I missed, raised by another repo in the org: - **Shell `timeout 30 go test ./...`** — what this repo uses (`backend/Makefile:25`, and carried into `backend/script/test` on PR #38's branch) — wraps the whole invocation, so the budget covers **compilation plus execution**. On an empty Go build cache in a fresh container, compilation alone can consume it before a single test runs. That is exactly the failure observed: `make: *** [Makefile:35: test] Terminated`, then a warm-cache retry passing in 11s. - **Go's own `-timeout 30s` flag** bounds **test execution only**. Compilation is not counted, so a cold cache cannot trip it. So the exposure is not universal across the org's Go repos — it is specific to the shell-wrapper form, which this repo has. ### Revised fix Convert the shell wrapper to Go's flag. This is a one-line change and it fixes three things at once: 1. **The flake.** The budget stops measuring compilation, so a cold container cache is no longer a failure mode. 2. **Portability.** `timeout(1)` is coreutils and does not exist on stock macOS. `-timeout` is part of the toolchain and works everywhere. 3. **Policy alignment.** `REPO_POLICIES.md`'s Go example is `go test -timeout 30s -race -cover ./...` — the flag form, not the wrapper. The existing definition of done already requires adopting that exact line, so this converges rather than adding scope. **Do not raise the budget.** A larger number would mask the flake rather than fix it, and would weaken the real 20-second execution ceiling the policy is trying to enforce. ### Amended definition of done Replacing the two bullets I added earlier: - [ ] `make test` uses Go's `-timeout 30s` flag. The external `timeout(1)` wrapper is gone from both `backend/Makefile` and `backend/script/test`. - [ ] Verified against a **cold** Go build cache — prune the container build cache for that one build (`docker build --no-cache`, never `docker builder prune`, which is shared state on this host) and confirm the gate passes. Do this with `-race` enabled, since race instrumentation makes compilation slower, not faster. Note for whoever implements: PR #38 relocates the backend test implementation from `backend/Makefile` into `backend/script/test`, carrying the shell-wrapper form with it. Read the tree as it exists at implementation time; if #38 has landed, the line to change is in `backend/script/test`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/netwatch#21