From 54d675bcf86c682cbd03478144ba1cf1bdd1ef13 Mon Sep 17 00:00:00 2001 From: Chemaclass Date: Fri, 14 Aug 2026 09:39:04 +0200 Subject: [PATCH] fix(install): stop advising a -d flag that does not exist All three destination errors ended with "Choose another destination with -d". install.sh parses positional arguments only, so the one message whose job is to tell the reader how to recover pointed at a form the parser rejects with "Invalid arguments". The existing tests asserted the diagnosis line and stopped there, so the invented flag was never executed. The new test runs the suggested form instead of string-matching it, and checks the advice on the run that actually prints it -- a flag-shaped argument dies in the parser first, so asserting there compares against nothing and passes for free. That vacuous version was written, caught by mutating the message back, and replaced. Closes #1221 --- CHANGELOG.md | 1 + install.sh | 6 ++--- tests/acceptance/install_test.sh | 45 ++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 663f40c6..ef9f00ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ ### Changed - `bashunit doc ` says `No assertion matches ''` instead of printing nothing, which was indistinguishable from a broken install. Mirrors the existing `--custom` wording; the exit code stays 0 because `doc` is informational (#1201) +- `install.sh` destination errors no longer advise a `-d` flag that does not exist — the script takes positional arguments, so following the advice produced `Invalid arguments`. The one message whose job is to tell you how to recover pointed at a form the parser rejects (#1221) - The `example/` demo is covered by the suite. `docs/examples.md` and `example/README.md` send new users to `./bashunit example` as their first contact with the framework, and nothing ran it — `make test` collects from `tests/` only — so it could break while CI stayed green. The dead `EXAMPLE_TEST_SCRIPTS` variable, pointing at a file deleted several releases ago, is gone with it (#1219) - Docs: an empty entry in the `-e/--env/--boot` file assigns an empty value rather than restoring the built-in default, so blanking a boolean disables it; the guide said it "wipes" the value, which reads as a reset. The rest of the precedence ladder is now covered by tests (#1217) - Docs: the test-function name rule is stated correctly — the prefix is a literal, lowercase `test_`, so the guide's own `testRenderAllTestsPassedWhenNotFailedTests` example and its claim that names are case-insensitive were both wrong, and a function named either way is silently never run (#1215) diff --git a/install.sh b/install.sh index d72cf63b..7432666e 100755 --- a/install.sh +++ b/install.sh @@ -199,19 +199,19 @@ cd "$(dirname "$0")" # directory` with no mention of bashunit at all (#1197). if [ -e "$DIR" ] && [ ! -d "$DIR" ]; then echo "Error: '$DIR' exists and is not a directory." >&2 - echo "Choose another destination with -d, or remove that file." >&2 + echo "Choose another destination by passing it as an argument, or remove that file." >&2 exit 1 fi if [ ! -d "$DIR" ] && ! mkdir -p "$DIR" 2>/dev/null; then echo "Error: cannot create the '$DIR' folder." >&2 - echo "Choose another destination with -d, or re-run with sufficient permissions." >&2 + echo "Choose another destination by passing it as an argument, or re-run with sufficient permissions." >&2 exit 1 fi if [ ! -w "$DIR" ]; then echo "Error: cannot write to the '$DIR' folder." >&2 - echo "Choose another destination with -d, or re-run with sufficient permissions." >&2 + echo "Choose another destination by passing it as an argument, or re-run with sufficient permissions." >&2 exit 1 fi diff --git a/tests/acceptance/install_test.sh b/tests/acceptance/install_test.sh index dcb0a32b..5f261ff4 100644 --- a/tests/acceptance/install_test.sh +++ b/tests/acceptance/install_test.sh @@ -344,3 +344,48 @@ function test_install_rejects_a_destination_it_cannot_write() { assert_general_error "" "" "$ec" assert_contains "cannot write to" "$output" } + +# The three destination errors above all end with advice on how to recover, and +# the advice named a `-d` flag the script never had -- `install.sh` parses +# positional arguments only, so following it produced "Invalid arguments" +# (#1221). The assertions covered the diagnosis line and stopped there, which is +# exactly how an invented flag survives. +# +# So run the suggested form instead of string-matching it. This needs no network: +# the destination is validated first, and an unparseable argument is rejected +# before that. +function test_the_recovery_advice_names_a_form_the_parser_accepts() { + local dir + dir="$(bashunit::temp_dir)" + cp ./install.sh "$dir/install.sh" + printf 'not a dir\n' >"$dir/blocked" + + local output + output=$(cd "$dir" && ./install.sh 0.47.0 blocked 2>&1) || true + assert_contains "Choose another destination" "$output" + + # The advice must not name a flag. Asserted against the run that actually + # prints it: a flag-shaped argument dies in the parser before any of this, + # so checking the advice there would compare against nothing at all. + assert_not_contains "-d" "$output" + + # And what it does tell the reader to do -- pass a different destination as + # an argument -- has to get past argument parsing. + local retry + retry=$(cd "$dir" && ./install.sh 0.47.0 elsewhere 2>&1) || true + + assert_not_contains "Invalid arguments" "$retry" +} + +# The parser takes positional arguments only, which is what the advice above +# now describes. +function test_a_flag_shaped_destination_is_rejected() { + local dir + dir="$(bashunit::temp_dir)" + cp ./install.sh "$dir/install.sh" + + local output + output=$(cd "$dir" && ./install.sh -d somewhere 2>&1) || true + + assert_contains "Invalid arguments" "$output" +}