Reject unrecognized options in steps and tests - #98
Merged
Conversation
An option key nothing consumes was dropped in silence, so the suite read as though it were set. The worst form is a misspelled or misplaced `evaluate`: the test then runs with zero checks and is reported as `ran`, and the suite exits 0 having asserted nothing. Rather than maintain a list of valid keys per type, which drifts the moment a factory gains an option, the option accessors record every key they read while a step or test is constructed. Whatever the factory never asked for is not an option of that type. Factories that reach into the options map directly record their reads explicitly. Errors name the offending key and the accepted set: Error: unknown option "evaluatte" in test "service responds" (an execute test accepts: capture, command, evaluate, extract, timeout) --check reports them, so a typo surfaces before any infrastructure exists. The check found a real dead option on its first run: `message` on the simulated step appears in every shipped example and was read by nothing. It now sets the status shown while the step waits, which is what the examples always implied it did. Note: keys at the test or step level rather than inside options: are still dropped by the YAML decoder before this runs. The suite summary from --check remains the way to catch those.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #97.
The problem
An option key that nothing consumes was dropped in silence, so the suite read as though it were set. The worst form:
The test runs with zero checks, is reported as
ranrather than passed or failed, and the suite exits 0 having asserted nothing. Same for an evaluator written one level too high (exit_code:besidecommand:instead of insideevaluate:), and forcapture:/extract:on a type that does not honour them.The approach
A hand-maintained list of valid keys per type would drift the moment a factory gained an option. Instead the option accessors record every key they read while a step or test is constructed; whatever the factory never asked for is, by definition, not an option of that type. The handful of factories that reach into the options map directly record their reads explicitly.
This is self-maintaining: a new option is accepted the moment a factory reads it, with nothing else to update.
--checkreports them, so a typo surfaces before any infrastructure exists.It found a real bug on its first run
messageon thesimulatedstep appears in every shipped example and was read by nothing — a dead option users have been copying since the examples were written. It now sets the status shown while the step waits, which is what the examples always implied. All 7 affected example suites validate again.That is the check working as intended: it distinguishes "the user typo'd" from "we documented an option we never implemented", and this was the latter.
Verification
examples/passes--check.path/filename) are covered too.go build,go vet,gofmt, and the full suite are green; the docs site builds.Known gap, documented rather than fixed
Keys at the test or step level rather than inside
options:— a stray key next toname:/node:/type:, ortest:instead oftests:— are dropped by the YAML decoder before any of this runs. Catching those means a strict decode at the config layer, which is a separate change with its own blast radius. The--checksuite summary remains the way to spot them, and the docs now say so plainly instead of describing the whole class as unchecked.