Skip to content

test: bug fixes are tested at the helper, not at the wiring that broke (8 closed bugs) #750

Description

@christian-byrne

Pattern

An audit of all 74 closed-and-completed bugs in this repo found one shape repeating across at least eight of them:

The fix is split into a pure helper plus the wiring that calls it. The helper gets thorough tests. The wiring — the default, the call site, the flag — is the part that actually produced the user-visible bug, and it has no test.

Revert the wiring in each case below and the entire suite stays green while the original issue reproduces verbatim. These are not "no tests were written" bugs; every one of them shipped tests. They are tests aimed one layer below the defect.

This is filed as one issue rather than eight, because the fix is a review convention plus a handful of small tests, not eight separate investigations.

Instances

Each row: the fix that landed, the part that is covered, and the exact line whose reversion is invisible to CI.

Closed bug Covered Uncovered wiring — revert this and CI stays green
#322 CivitAI token broken check_civitai_url parsing, ~20 cases at tests/comfy_cli/command/models/test_models.py:142-235 comfy_cli/command/models/models.py:417 builds Authorization: Bearer on the foreground path and forwards it at :421/:433. Zero Bearer assertions exist under tests/comfy_cli/command/models/. The titled failure mode — the token not being sent — is untested.
#397 cu126 installed where cu130 needed resolve_cuda_wheel (tests/comfy_cli/test_cuda_detect.py:267-292) and install.execute(cuda_tag=…) comfy_cli/cmdline.py:383 _resolve_cuda, called at :610. git grep _resolve_cuda origin/main -- tests/ = 0 files (control: 1 prod file). Tests pass cuda_tag in directly, bypassing the resolver that was the defect.
#472 outdated ROCm ROCm URL construction, parametrized at tests/comfy_cli/test_install_python_resolution.py:283-293 The defaults at comfy_cli/cmdline.py:465, install.py:50, install.py:173. Every ROCm test passes rocm_version explicitly, so reverting all three defaults passes.
#311 default_launch_extras ignored The resolver, tests/comfy_cli/test_workspace_manager.py:326,340,385 comfy_cli/command/launch.py:428-434, which applies the extras. The only launch_extras hit in tests/ is test_config_manager.py:163, a config-read test.
#271 comfy node bisect broke parse_cm_output, 11 assertions at tests/comfy_cli/command/test_bisect_parse.py The call site comfy_cli/command/custom_nodes/bisect_custom_nodes.py:164. Reverting start() to the old inline comprehension leaves all 11 parser assertions green.
#205 set-default / --workspace The marker-fallback branch, tests/comfy_cli/test_workspace_manager.py:188 The git-success branch comfy_cli/workspace_manager.py:109 (return True, str(repo.working_dir)) — the other half of PR #401's return-type change. Only reachable via tests/e2e/test_e2e.py:83, which is skipped unless TEST_E2E=true.
#430 visual glitches + error The error half — markup metachars, exit code, no traceback The glitch half: transient=True at comfy_cli/ui.py:38 and comfy_cli/file_utils.py:349 has no assertion. (The 11 transient hits in tests/ are all the word "transient network" in oauth/cloud tests, not this flag.)
#403 cm-cli not found The FileNotFoundError arm, tests/comfy_cli/test_cmdline_python_resolution.py:37-40 The subprocess.CalledProcessError arm of the same except at comfy_cli/cmdline.py:691, and the sibling guard at install.py:315-318, which has no test at all.

Why this is worth a convention, not eight tickets

The repo's test suite is large and mostly good — 201 test files, ~11,181 assertions on origin/main. The failure is not effort or skill; it is that "did you add a test?" is satisfiable by testing the helper, and a helper test is easier to write than a wiring test. So the easy test displaces the load-bearing one.

Two cheap guards would catch most of it:

  1. Review checklist item: for a bug fix, name the line whose reversion the new test detects. If that line is not the one in the bug report's stack/repro, the test is aimed at the wrong layer.
  2. Prefer one call-site test over N helper tests. In several rows above the helper already had coverage before the fix; the fix added more helper cases.

Related, same family, different mechanism: #344 has no test on its branch at all (filed separately as #749), and #342's redactor is applied at one of several sinks (filed as #748).

Not claimed

Filed by a closed-bug regression audit (Comfy-Org/comfy-cli, origin/main @ 3ff9f55, merge-base 62f35f2).

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions