Skip to content

refactor(get): move the get command's body into internal/get - #26

Merged
dimoschi merged 5 commits into
masterfrom
refactor/get-into-internal
Sep 13, 2026
Merged

dimoschi merged 5 commits into
masterfrom
refactor/get-into-internal

Conversation

@dimoschi

Copy link
Copy Markdown
Collaborator

Closes #25.

What

cmd/get.go held the PoolManager construction and the json/text/spew output switch inside a cobra Run closure, none of it reachable from a test. That is the shape server.go had before #20, and it is why the mutation survivors in #24 had to be accepted rather than covered.

internal/get now exposes Run(cfg, w io.Writer) error, writing to an injected writer so every format can be asserted. cmd/get.go is a Config, a call, and an exit code.

Chasing the survivors found three real defects

Rather than accept what the mutation gate reported, each survivor was treated as either dead code or a missing test. Three turned out to be dead or wrong code:

The JSON encode error was unreachable. Everything PoolManager marshals is a string, int64, map or slice, and both custom MarshalJSON methods return a nil error unconditionally, so json.Marshal cannot fail. Encoding straight to the writer removes the branch and leaves one error a caller can actually hit — the write.

uitable.Wrap was dead. Rendering the same table with it on and off is byte-identical, because MaxColWidth elides the cell first. Deleted rather than pinned. The width itself does matter and is now asserted through its effect, by measuring the widest rendered cell.

Every failed scrape was logged twice. Pool.error logged it and collectPools logged it again with context, so the server path emitted two identical-ish lines per failure per scrape. That predates this branch; returning the joined error from Update in #24 had quietly made it three. Pool.error now only records, leaving the line that carries context.

And two broken tests of my own

Worth stating, because both passed while testing nothing:

  • The test logger discarded Info, Debug and Error, keeping only the *f variants. The duplicate-logging test therefore passed against the exact duplication it was written to catch.
  • The column-width assertion compared the measured cell against maxColWidth, so mutating the constant moved both sides together. It now compares against 80 directly and fails on an off-by-one in either direction, which I verified by making the change and watching it fail.

Behaviour changes

  • An unrecognised --out value is an error rather than a logged message, so it exits non-zero instead of silently succeeding with no output.
  • Writing to stdout is checked, so a failed write is reported rather than discarded.
  • --out json gains a trailing newline, a side effect of streaming through json.Encoder. Parsers do not care; jq pipelines are unaffected.

Verification

$ mise exec -- go test ./... -race
ok  	github.com/hackthebox/php-fpm_exporter/internal/get
ok  	github.com/hackthebox/php-fpm_exporter/internal/server
ok  	github.com/hackthebox/php-fpm_exporter/phpfpm
ok  	github.com/hackthebox/php-fpm_exporter/phpfpm/fcgi

All three output formats checked against the built binary, plus the exit codes (1 on scrape failure, 0 otherwise).

golangci-lint (v2.12.2, as CI pins): 4 issues, unchanged from master, none new.

Mutation survivors went 23 → 7. The remaining 7 are all in cmd/get.go — the Config literal, the if err != nil, and the log.Error — and are accepted because covering them means putting tests back into cmd, which is what this PR exists to avoid. Nothing in internal/ or phpfpm/ survives.

Closes #25.

cmd/get.go held the PoolManager construction and the json/text/spew output
switch inside a cobra Run closure, none of it reachable from a test. That is the
same shape server.go had before #20, and it is why the mutation survivors in #24
had to be accepted rather than covered.

internal/get exposes Run(cfg, w io.Writer) error, writing to an injected writer
so every format can be asserted. cmd/get.go is now a Config, a call, and an exit
code.

One behaviour change falls out of it: an unrecognised --out value is an error
rather than a logged message, so it exits non-zero instead of silently
succeeding with no output. Writing to stdout is also checked now, so a failed
write is reported rather than discarded.
The first pass only checked that each label appeared, against a fixture whose
values were all zero, so dropping any single row was invisible. Each field now
carries a distinct value, and the blank separator between pools and the wrapping
of an overlong value are asserted too.
The configured ScrapeTimeout was not asserted, so dropping it and silently
falling back to the package default went unnoticed.

Writing the wrapping test also corrected an assumption: uitable elides an
overlong cell at MaxColWidth rather than wrapping it, so a scrape URI longer
than 80 characters is shown truncated. The test now documents that rather than
claiming the value is preserved.

Uses a generic pool name in the fixture.
…res twice

Following the principle that a mutation survivor no test can kill is either dead
code or a missing test, rather than something to accept.

The JSON encode error was unreachable: everything PoolManager marshals is a
string, int64, map or slice, and both custom MarshalJSON methods return a nil
error unconditionally. Encoding straight to the writer removes the branch and
leaves one error a caller can actually hit, the write.

uitable's Wrap had no effect: rendering the same table with it on and off is
byte-identical, because MaxColWidth elides the cell first. Deleted rather than
pinned. The width itself does matter and is now asserted through its effect, by
measuring the widest rendered cell, so an off-by-one either way fails.

Pool.error logged every scrape failure and so did collectPools, so the server
path emitted two lines per failure per scrape. That predates this branch; adding
the joined error to Update made it three. Pool.error now only records, leaving
the line that carries context.

The test logger was discarding Info, Debug and Error, which is why the first
attempt at this test passed against the very duplication it was meant to catch.
The width assertion compared the measured cell against maxColWidth, so changing
the constant moved the expectation with it and the test could never fail. It now
compares against 80 directly, which is the contract a reader of the output
depends on.
@dimoschi
dimoschi merged commit 934f600 into master Sep 13, 2026
7 checks passed
@dimoschi
dimoschi deleted the refactor/get-into-internal branch September 13, 2026 21:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Move the get command's body out of cmd/ into internal/

1 participant