feat: warn when a matcher property is an empty list - #1097
Conversation
A list of patterns is a list of conditions that must all hold, so an empty list is zero conditions: isSet reports the property as absent, no check is created, and the resource passes without asserting anything. That is easy to write by accident and silent when you do. Warn about it on stderr instead of failing. The behaviour is unchanged on purpose. goss add file emits contents: [] to record that it captured no content expectation, and empty lists appear in this repository's own testdata and integration gossfiles, so rejecting them would invalidate every gossfile goss has ever generated. The warning covers all six properties that route through isSet: command.stdout, command.stderr, file.contains, file.contents, http.headers and http.body. It names the resource and property in the same "<id>: <type>.<property>" shape as the existing deprecation warnings, and goes to stderr like they do, so the text, JSON, TAP, JUnit and nagios output on stdout is untouched. Refs goss-org#1095 Signed-off-by: Victor Solano <victor.solanonunez@gmail.com>
|
The red I assumed it was mine first and went looking.
Everything else here is green: If you merge #1098 first I'll rebase this one and the check should go green. |
Picks up goss-org#1098 (serve-test log-buffer race fix), which is the cause of the stale red `coverage` check on this PR's previous head 6a9d2dd. Signed-off-by: Victor Solano <victor.solanonunez@gmail.com>
kgaughan
left a comment
There was a problem hiding this comment.
Looks good. There's a small change I'm hoping to add as part of some modernisation.
Refs #1095. Follow-up to #1096, implementing what you asked for there:
So: warning, behaviour untouched.
What changed
isSetWarnEmptysits next toisSetand wraps it. It warns when the property holds a list written out as empty, then returns exactly whatisSetreturned, so no matcher changes outcome.All six properties that route through
isSetare covered —command.stdout,command.stderr,file.contains,file.contents,http.headers,http.body:Count: 1, Failed: 0is unchanged frommaster— the check is still skipped and still passes.The message follows the
<id>: <type>.<property>shapedeprecateAtoIalready uses, and goes to stderr like the existing deprecation warnings, so stdout stays clean. Verified:-f jsonstill parses as JSON,-f tapis unchanged, andintegration-tests/test.shcaptures stdout only, so nothing there sees the warning.stderr: ""— the form the warning points at — produces no warning and correctly reportsCount: 2, Failed: 1.The thing you should decide
A gossfile generated by
goss add fileproduces one warning per file. Measured, not guessed:That is the generator writing
contents: [](resource/file.go:125) and the warning correctly reporting that it asserts nothing. It is accurate, but it means the warning fires on goss's own output.This repository's own fixtures hit it too — 8 empty-list matcher properties across
testdata/passing.goss.yaml,testdata/failing.goss.yaml,goss-shared.yaml,goss-serve.yamlandgoss-dummy.yamlwould each print one line on stderr during an integration run. No test breaks, because they all read stdout. I left those files alone rather than editing fixtures to silence a warning about the thing they exercise.If that noise is not acceptable, the fix is to stop
goss add filewritingcontents: []. That is a generator behaviour change and it would needintegration-tests/goss/*/goss-expected-q.yamlregenerating, so I did not fold it into this PR. Say the word and I'll do it as a separate one.One more thing worth flagging:
goss servere-validates on cache miss, so a long-running server repeats the warning each time. That is exactly how the existingfile.containsdeprecation warning already behaves, so I kept it consistent rather than inventing a dedupe mechanism. Happy to add once-per-process dedupe if you'd rather.Testing
resource/resource_test.gois new. It pins both halves of the contract: the warning fires only for an empty list, and the set/unset answer is byte-identical toisSet's for every input — nil, empty list, non-empty list,"", a string, and an empty map.Both warning assertions were checked by disabling the warning and confirming they go red;
TestIsSet*is otherwise vacuous.TestIsSetWarnEmptyNamesThePropertyalso asserts exactly one line is written, so a future change that warns per element rather than per property fails.gofmt -l,go vet ./...,go build ./...,go test ./... -count=1all clean.npx markdownlint-cli2 "docs/**/*.md"reports 0 issues in 14 files, matching the docs workflow.Docs: the two admonitions added in #1096 now say the warning exists and that generated gossfiles will trigger it, so the documentation still matches the behaviour.
AI assistance
This contribution was prepared with AI assistance. Every console block above is real output from the built binary on this branch,
GOSS_USE_ALPHA=1, Go 1.26.5.📚 Documentation preview 📚: https://goss--1097.org.readthedocs.build/en/1097/