Skip to content

test_build_refusal.py: _sh(srcdir, expr) never reads srcdir, so four arms are not parameterised by the tree they are handed #933

Description

@OffgridwithJD

test/pytest/test_build_refusal.py has a helper that takes a tree and ignores it:

def _sh(srcdir, expr):
    """Evaluate one lib.sh expression against a tree, and return its stdout."""
    script = f'. "{SRCDIR}/test/lib.sh" || exit 1; {expr}'
    p = subprocess.run(["bash", "-c", script], capture_output=True, text=True)
    return p.stdout.strip(), p.returncode

srcdir — the parameter — is never read. SRCDIR — the module global, pathlib.Path(__file__).resolve().parents[2], the real source tree — is what gets sourced.

Callers pass a fixture tree:

396:    out, rc = _sh(tmp_path, 'pgc_write_source_stamp "/proc/pgc-twin" "deadbeef"')
398:    ok, rc2 = _sh(tmp_path, f'pgc_write_source_stamp "{tmp_path}/s" "cafebabe"')
412:    a, _  = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[0]}"')
413:    b, _  = _sh(tmp_path, f'pgc_source_stamp_path /tree "{cfgs[1]}"')

So the call reads as "evaluate this expression against this tree", the docstring says so, and the tree is discarded. Whatever those four arms measure, it is not parameterised by the tree they are handed.

Why it is worth its own red test rather than a quiet deletion

The parameter is either load-bearing or it is not, and the file does not currently say which:

  • if those arms are meant to run against the fixture tree, they are measuring the wrong thing and the fix changes what they assert;
  • if they are meant to run against the real tree, the parameter is noise that makes four call sites read as something they are not, and removing it is the fix.

Either way the arm that distinguishes them does not exist, so a future reader cannot tell a deliberate choice from a slip. That is the same shape as a guard whose subject is not what its name says.

_sh_fp and _sh_fp_as (lines 486 and 538) build the same script from SRCDIR but take no tree parameter, so they are consistent with what they do and are not part of this.

How it surfaced

Found while verifying the debt inventory in #932, which cited this file as the example of code that does not touch the real test/lib.sh. It does, at three sites; that part is being corrected in #932 itself. The dead parameter is a separate thing and @jdatcmd asked for it to be filed rather than folded into the debt work, on the grounds that it deserves its own red test.

No suggested patch here on purpose: the right fix depends on which of the two readings above was intended, and whoever wrote the arms knows that better than I do.

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions