Repository navigation
fix(e2e): refuse a bad flag at the runner's entry instead of running everything, through node:util parseArgs - #567
Merged
Merged
Conversation
…arseArgs does The curated runner's parseFlags walked argv by hand and ignored whatever it did not recognize: a typo'd flag, a stray word, an inline `--sections=labels` or `--scenario=name`, a flag with no value, or an empty `--sections ""` ran the whole corpus for minutes and exited green, and `--` was skipped so the flags after it were read. node:util's parseArgs, which test/e2e/fuzz.ts already uses, reads the three flags in every form it defines, and the runner now refuses the rest at the entry with exit 2, naming the argument and the usage line, as the fuzzer does. Answers that change, each an input a developer can type and none the repository itself passes: - `--sections=labels` and `--scenario=name` are read as the flag's value; the old parser ignored both and ran everything. - `--sections labels --sections all` ends with no filter: `all` clears the list, so the last flag wins whichever word it carries; the old parser kept `labels` because `all` only ever meant "do not set". - A `--` and the arguments after it are refused as unexpected; the old parser skipped the `--` and read the flags after it. bun itself drops a `--` that directly follows the script path, so `bun test/e2e/run.ts -- --sections labels` reaches the script without it and filters as before, while a `--` after a flag reaches the script and is refused. - An unknown flag, a stray word, a flag with no value, `--print-stdout=yes`, and an empty or all-commas `--sections` are refused; the old parser ran everything, or filtered on an empty key and found nothing. Differential check, the old parser against the new: every invocation the repository makes (`bun run test:e2e` from package.json, checks.yml, and CONTRIBUTING; the `--scenario <name>` replay lines the runner writes; the header's `--sections a,b`, `--sections all`, `--print-stdout`, and their pairings) parses identically. Of 6000 generated argv shapes over the flags, their inline forms, `all`, `--`, stray words, misspellings, and empty values, 149 parse identically and 5851 differ only as listed above: 5770 by a refusal where the old parser accepted silently, 81 by an inline value or a later `all` now honored. The repeated `--sections` forms the old parser got right (`all` first, or two lists) still parse the same. The new test/e2e/run.test.ts spawns the runner on each refused shape and on the two honored shapes a filter nothing matches exposes through its message, so neither can regress to a silent full run.
Contributor
File size check0 over a hard cap (fails), 6 warning(s).
Split the file, wrap the line, shorten or exempt the comment, or list the path in 5 managed file(s) skipped; repo-platform owns them. |
There was a problem hiding this comment.
🟡 Changes recommended
String options can still swallow subsequent flags or --, allowing malformed invocations to run the entire corpus.
1 open issue
What changed in this PR
This PR adds argument validation to the end-to-end runner to prevent malformed invocations from silently running the full scenario corpus.
Changes:
- Replaces manual argument parsing with
node:utiltoken parsing and explicit exit-2 errors. - Supports inline string values and lets a later
--sections allclear earlier filters. - Adds nine spawn-based cases covering refusals and filter parsing.
| File | Description |
|---|---|
test/e2e/run.ts |
Adds flag validation, diagnostics, and updated filter handling. |
test/e2e/run.test.ts |
Adds process-level assertions for invalid arguments and honored filters. |
Review effort: Balanced
…belongs parseArgs hands a string flag the next word whatever it is, so `--sections -- --sections all` took `--` as the sections value, the later `all` cleared the filter, and the whole corpus ran instead of exit 2. A non-inline value that starts with `-` is now refused as the value missing (`--sections needs a value; ...`), before `--sections` or `--scenario` applies; an inline `--scenario=-name` still passes. One answer changes with it: `--sections --print-stdout` is refused at the entry, where the hand-rolled loop and the first commit both took the flag as the value and the run then reported that no scenario matched it. Two regression rows pin both shapes.
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.
Before
After
How
--sections=labelsand--scenario=nameare honored; a later--sections allclears the list; a--after a flag is refused; a string flag followed by--or another flag is refused as a missing value; an unknown flag, stray word, or--print-stdout=yesexits 2.bun run test:e2e, the replay lines the runner writes, and the header's eight flag pairings.node:utilparseArgs, already used bytest/e2e/fuzz.ts.Proof
run.test.ts, 11 cases (9 refusals with exact stderr and exit 2, 2 honored shapes), every row red against the parser it replaced; e2e 373/373.Technical details
allnow honored; no other class.-(the Open Group convention), so a one-predicate guard refuses such a non-inline value; an inline--scenario=-namekeeps its dash.--that directly follows the script path (node keeps it), sobun test/e2e/run.ts -- --sections labelsfilters on labels in both old and new; the test puts a flag ahead of the--.strict: truewould reject the same shapes by throwing, at the cost of translating its messages to keep the runner's wording.--helpflag.test/e2e/run.ts,test/e2e/run.test.ts(new).