Skip to content

The development scripts report results without having run anything #400

Description

@akiomik

Split out of #396, which was narrowed to the bugs that stop the scripts from
doing what they say. This collects the other half: every way
scripts/acceptance and scripts/benchmarks produce an answer when the thing
being measured never ran. None of them fails loudly; each looks like a result.

The cases

where what is missing what you get
acceptance/test.sh $PROJECT_ROOT/tmp, which is gitignored and which nothing creates both redirects fail, the second only after paying for a cargo run
acceptance/test.sh mdl (a Ruby gem, vendored nowhere) empty mdl.txt; two empty files compare equal, so mado and mdl "agree"
acceptance/test.sh $DOC_PATH, until setup.sh has run both files empty, same false agreement
acceptance/test.sh a build that fails, or no cargo cargo run writes only to stderr, mado.txt empty, exit 0
benchmarks/comparison.sh node_modules, which is gitignored and which nothing installs hyperfine --ignore-failure publishes markdownlint and markdownlint-cli2 as roughly a thousand times faster than mado
benchmarks/comparison.sh mado's binary, when CARGO_TARGET_DIR or build.target-dir sends it elsewhere cargo build succeeds, hyperfine times a command that cannot start, mado "wins"
both the repository, when run from another directory cargo cannot find the manifest

The node_modules one is the common case, not a corner: scripts/benchmarks/
tracks package.json and package-lock.json, node_modules is gitignored,
nothing runs npm ci, and README.md documents the benchmark as setup.sh then
comparison.sh. A fresh clone following the README publishes wrong numbers.

What a fix has to decide

Not just "add a check". hyperfine --ignore-failure is there deliberately,
because the linters legitimately exit non-zero when they find violations —
mado check on a file with violations returns 1 — so the scripts cannot simply
gate on exit status. The distinction that has to be drawn is present and
built
versus what it reported, and it has to be drawn for every external
tool the scripts invoke.

Whether setup.sh should also run npm ci, or the README should gain the
step, is part of the same decision.

Why the linter would not have caught it

None of these is visible to shellcheck. They are facts about
hyperfine --ignore-failure's semantics, about cargo's output path, about
which files git restores, and about what an empty file means when two are
compared — not about shell syntax. #398 is worth doing on its own terms, but it
would not have found a single row of the table above.

That is also the argument in #397, applied to these scripts rather than to
action/entrypoint.sh: the failure mode here is not that the shell is written
badly, it is that this much conditional external-tool orchestration has no
tests and no type system, and every review round finds another unguarded
dependency. Related: #399, where the same class turned destructive.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    ciChanges to our CI configuration files and scripts

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions