feat(dx): gate distrobox create/install/export on a cached image at runtime - #753
Conversation
…untime The three distrobox scenarios that cover the full value proposition — create a container, install a package in it, export the app to the host — landed tagged `@pending @requires_cached_image`. `@pending` is skipped by `tests/shared/quarantine.py` before anything else runs, so `@requires_cached_image` was inert: nothing in the repo read it. That is the masking trap `docs/skills/test-authoring/suite-map/SKILL.md` warns about, and it left projectbluefin#501 with no activation path other than a human hand-editing the feature file once projectbluefin/lab#621 lands — at which point the scenarios would run unguarded and a cold `distrobox create` would pull from the registry mid-scenario, which is the CI timeout the issue set out to avoid. Make the tag mean something. `tests/shared/image_cache.py` reads the image references out of a tagged scenario's own step text, probes each with `podman image exists` on the DUT, and skips with the missing refs named while any is absent. The DX `before_scenario` calls it right after `skip_quarantine`. `podman image exists` never contacts a registry — a probe that could pull would trigger the very timeout the gate prevents. The scenarios drop `@pending` and are now active behind that gate. Runtime behaviour on today's CI is unchanged (no cached fedora-toolbox, so they skip with an explicit reason instead of a placeholder one), but they activate on their own the moment lab#621 pre-pulls the image — no follow-up PR. This is the same contract as `@requires_bctl` / `@requires_toggle_action` in the common suite, so `@requires_cached_image` stays out of `_SKIP_TAGS`, `NON_RUNNABLE_TAGS`, and `BEHAVE_TAG_ARGS`. The probe deliberately does not reuse `run_ssh` from `tests/shared/ssh_steps.py`: importing that module registers its `@step` phrases, and `SSH command return code is "{code}"` and `Last command output contains "{text}"` are also defined by `tests/dx/features/steps/steps.py`, so a DX hook importing it would raise `AmbiguousStep` and take the suite down. It resolves connection details from `tests/shared/ssh_config.py` instead and leaves `context` untouched, since it runs before the per-scenario state reset. Unit coverage pins the parts that can regress silently: that every `@requires_cached_image` scenario in the tree names a registry-qualified image, that none is masked by a non-runnable tag, that the probe command is local-only, and that `image_cache.py` never imports the step library. Learning written back to `docs/skills/test-authoring/behave/SKILL.md` plus a new `references/cached-image-gate.md`, with the tag table, dx notes, and the projectbluefin#501 gap row updated in the suite map. Refs: projectbluefin#501, projectbluefin/lab#621 Assisted-by: Claude Opus 5 via Claude Code Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes projectbluefin#501 Adds 8 unit tests for the dx_distrobox_can_be_created step: - Happy path (cleanup → create → list with name in output) - Cleanup rc=1 acceptable (container not found) - Cleanup "No such container" text acceptable at any rc - Unexpected cleanup rc raises AssertionError - Create failure raises AssertionError - List failure raises AssertionError - Name absent from list raises AssertionError - Command shape: --name, --image, --yes flags verified All tests use the existing _FakeSSH sequential-call helper. Complementary to PR projectbluefin#753 which gates the feature scenarios at runtime via image_cache. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
hanthor
left a comment
There was a problem hiding this comment.
Reviewed the gating design and the diff in full.
This correctly fixes a real masking bug: @requires_cached_image was inert because @pending short-circuits before_scenario in skip_quarantine, so nothing ever read the capability tag. Making it load-bearing via skip_when_image_not_cached() and dropping @pending from the three DX scenarios is the right fix, and it's consistent with the existing @requires_bctl pattern.
Design choices that stood out as deliberately correct rather than incidental:
podman image existsis local-only and can't itself trigger the timeout it's guarding against — pinned bytest_probe_never_contacts_a_registry.- Reading the image ref out of the scenario's own step text (rather than a hardcoded constant) keeps the feature file as the single source of truth and is covered by
test_every_tagged_scenario_names_an_image, which runs against the real.featurefiles, not just fixtures. _looks_like_image_refdeliberately rejects barefedora:latest(no registry) — sound, since an unqualified ref's resolution depends on the DUT'sregistries.confand the probe could disagree with what the scenario would actually pull.- Avoiding
tests/shared/ssh_steps.pyin the probe to dodge theAmbiguousStepcollision withtests/dx/features/steps/steps.py's ownSSH command return code is "{code}"/Last command output contains "{text}"steps is correct — I independently confirmed those two phrases are in fact duplicated between the two files. test_tagged_scenarios_are_not_masked_by_a_non_runnable_tagis a genuinely useful regression guard against the exact bug this PR fixes recurring.
Coverage snapshot and skill-doc updates are consistent with the code change, and the file-disjointness accommodation for the concurrently open #740 (table row appended at the end, not inserted at #740's insertion point) is a good example of respecting the stated repo convention.
Same caveat as the other Danathar PRs in this batch: 0 GitHub Actions check runs currently registered (fork workflow approval likely needed) and a red ghost-lab status that fails uniformly across this whole batch of PRs regardless of content, which reads as infra flake rather than a real per-PR signal — worth confirming lint/pytest/dry-run actually go green before merge.
Generated by Claude Code
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…fin#501) (projectbluefin#755) * test(dx): add TestDistroboxCanBeCreated unit tests Closes projectbluefin#501 Adds 8 unit tests for the dx_distrobox_can_be_created step: - Happy path (cleanup → create → list with name in output) - Cleanup rc=1 acceptable (container not found) - Cleanup "No such container" text acceptable at any rc - Unexpected cleanup rc raises AssertionError - Create failure raises AssertionError - List failure raises AssertionError - Name absent from list raises AssertionError - Command shape: --name, --image, --yes flags verified All tests use the existing _FakeSSH sequential-call helper. Complementary to PR projectbluefin#753 which gates the feature scenarios at runtime via image_cache. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * docs(skills): document _FakeSSH multi-call SSH step pattern Adds a section to the unit-test-module-stubs skill reference describing how to use _FakeSSH to test steps that make multiple sequential _ssh() calls. Covers happy path, per-call failure, conditional acceptance, and command shape verification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #501.
What #501 asked for, and what was actually missing
The four scenarios the issue proposed already exist in
tests/dx/features/dx_tools.feature, with step implementations and unit coverage. What did not exist was any way for them to ever run.They were tagged
@pending @requires_cached_image.tests/shared/quarantine.pyskips@pendingfrombefore_scenarioand returns early, so@requires_cached_imagewas inert — nothing in the repo read it. That is precisely the masking trap this repo's own suite map warns about:The only activation path was a human hand-editing the feature file once
projectbluefin/lab#621lands. And at that moment the scenarios would run unguarded: a colddistrobox createpullsfedora-toolbox:latestfrom the registry inside the scenario and eats the CI timeout — the exact failure #501 was filed to prevent.What this PR does
Makes the tag load-bearing.
tests/shared/image_cache.pyexposesskip_when_image_not_cached(context, scenario), called from the DXbefore_scenarioright afterskip_quarantine. For a tagged scenario it:podman image exists <ref>on the DUT for each distinct reference;podman image existsis local-only and never contacts a registry. That is load-bearing, not incidental: a probe that could pull would trigger the timeout the gate exists to prevent, so a unit test pins the command shape.The three scenarios then drop
@pendingand become active behind the gate.@requires_cached_imageis a runtime capability gate in the family of@requires_bctland@requires_toggle_action, not a non-runnable tag — so it stays out of_SKIP_TAGS,NON_RUNNABLE_TAGS, andBEHAVE_TAG_ARGS.Runtime behaviour on today's CI is unchanged: no cached
fedora-toolbox:latestmeans the scenarios still skip, now with@requires_cached_image — not in the DUT's local podman store: …instead of a generic placeholder reason. Once lab#621 pre-pulls the image they activate on their own, with no feature-file edit and no follow-up PR.The
AmbiguousSteptrap this surfacedThe probe deliberately does not reuse
run_sshfromtests/shared/ssh_steps.py. Importing that module registers its@stepphrases into behave's global registry, and two of them —SSH command return code is "{code}"andLast command output contains "{text}"— are also defined bytests/dx/features/steps/steps.py. A DXbefore_scenarioimporting it would raiseAmbiguousStepand take the whole suite down.It resolves connection details from
tests/shared/ssh_config.py(the helper documented for hooks that probe the VM directly, carrying no step definitions) and shells out itself. It also leavescontext.command_stdout/context.ssh_rcuntouched, since it runs before the per-scenario state reset and would otherwise leak a probe result into the scenario's first assertion.Step-isolation is usually discussed in the feature→steps direction; this is the hook direction, and it is now written down.
Verification
ruff check tests/ --select E,F,W --ignore E501python3 -m pytest tests/unit/ -qbehave --dry-run tests/dx/featurespython3 scripts/validate_docs.pypython3 scripts/generate_skill_index.py --checkpython3 scripts/update_coverage_snapshot.pyTwo of the new unit tests are regression guards rather than coverage:
test_tagged_scenarios_are_not_masked_by_a_non_runnable_tag— fails againstmainas it stands today with@requires_cached_image is masked by ['pending']. Verified by stashing the feature-file change and re-running.test_every_tagged_scenario_names_an_image— a tagged scenario whose steps name no registry-qualified image would skip forever while looking like an infra gap.behave --dry-runcannot exercise the gate (it never callsbefore_scenario), which is why the behaviour is pinned in unit tests instead.Local runs used a stubbed
qecore—qecoreneedsgi/AT-SPI, which this environment does not have. The 7undefinedsteps in the dry-run output are the stubbed qecore GUI phrases (VS Code launch/close), not suite content; all 34 distrobox and SSH steps resolve.Note on the file-disjointness rule
docs/skills/test-authoring/suite-map/SKILL.mdis also touched by open PR #740. The overlap is unavoidable — the coverage snapshot in that file is generated and scenario counts changed here. To keep it conflict-free, the new tag-table row is appended at the end of the table, while #740 inserts after@hardware_emulation; the remaining edits (generated snapshot block, #501 gap row) sit ~15 and ~110 lines away from #740's single hunk. The detailed authoring guidance went todocs/skills/test-authoring/behave/instead, which no open PR touches.Scope left alone
distrobox enter works with default containerscenario stays@pending. It names barefedora:latest, which the gate deliberately does not recognise: without an explicit registry, whatpodman image existsresolves depends on the DUT'sregistries.confsearch list, so the probe could disagree with the pull the scenario would perform. Re-qualifying that image is a behaviour change to an unrelated pre-existing scenario.dx_distrobox_can_be_createdalready runsdistrobox rm --forcebefore create, so each run is deterministic; addingafter_scenariocleanup is a separate change and would widen this PR into the step definitions.🤖 Generated with Claude Code
🐝 Hive Agent:
contributor| SHA:f62cb6ae