Skip to content

docs(doubles): the spy assertion family disagrees with itself on argument order #984

Description

@Chemaclass

Summary

Within one family, two assertions put the spy in different positions:

assert_have_been_called_times "expected_count" "command"   # count first
assert_have_been_called_with  "command" "expected_args"    # command first

Both are documented correctly. The problem is that they are documented differently, and nothing at the call site tells them apart — assert_have_been_called_times 2 my_cmd and assert_have_been_called_with my_cmd "arg" look like the same shape to a reader skimming a test file.

Why it bites rather than just reads oddly

A swapped pair does not error. It compares two strings that happen to be in the wrong slots and reports a plain assertion failure, so the output is a plausible-looking mismatch rather than a hint that the call is malformed:

Expected 'my_cmd' but got '2'

Someone reading that will check what their spy recorded before they check the argument order.

The wider catalogue has the same split, which is why this is worth writing down rather than fixing by feel:

Assertion Subject position
assert_same "expected" "actual" expected first
assert_contains "needle" "haystack" needle first
assert_array_contains "needle" "haystack" needle first
assert_have_been_called_times "count" "command" count first
assert_have_been_called_with "command" "args" command first
assert_json_contains "key" "expected" "json" subject last

The majority convention is expected-value first, subject second. assert_have_been_called_with and assert_json_contains are the outliers.

What I am not proposing

Renaming or reordering these. They are public API, they are correct as documented, and silently swapping argument meaning would break every existing suite in the worst possible way — tests that keep passing while asserting something else. Any reordering would need a deprecation cycle, and I do not think the churn is worth it.

Proposal

Make the inconsistency visible where it is encountered, rather than only in the reference table.

  1. Say it in the docs. docs/doubles.md and the spy section of docs/assertions.md should state plainly that _times takes the count first while _with takes the spy first, instead of leaving the reader to infer it from two adjacent signatures. One sentence, positioned where someone is choosing between them.

  2. Name the arguments in the failure message. Today a swapped call produces a bare value mismatch. If the failure said which side was read as the spy and which as the expectation, the mistake is self-evident:

    Expected spy 'my_cmd' to have been called 2 times, got 0
    

    This is the higher-value half — it helps at the moment of confusion, and it costs nothing at runtime.

  3. Consider a guard for the obviously-swapped case. assert_have_been_called_times with a non-numeric first argument is always a mistake — a call count is a number. Failing that with a usage error would catch the exact swap this issue is about. Overlaps with the arity work in the missing-argument issue; whoever picks up either should look at both.

Constraints

  • Public API: signatures do not change. Only messages and docs.
  • Per-assertion path must stay fork-free — see .claude/rules/perf-fork-budget.md.
  • Failure output is compared verbatim in this suite, so message changes are mechanically verifiable; grep tests/ for the current strings before editing.
  • Bash 3.0+.

Acceptance criteria

  • Docs state the _times / _with order difference explicitly, where the reader chooses between them
  • Spy failure messages name what was read as the spy and what as the expectation
  • A decision is recorded on whether a non-numeric count is rejected as a usage error
  • No signature changes; existing suites behave identically
  • make sa · make lint · ./bashunit --parallel --simple --strict tests/ · bash build.sh bin -v

Metadata

Metadata

Assignees

Labels

documentationImprovements or additions to documentation

Type

No type

Projects

Status
No status

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions