Comfy-Org / Comfy-Org/comfy-cli
test: bug fixes are tested at the helper, not at the wiring that broke (8 closed bugs)
- Dominant language
- Python
- Stars
- 968
- Forks
- 151
- Avg merge
- 1d 9h
- Merged PRs (30d)
- 77
Description
## 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
- No assertion that any of these has *actually* regressed today. Each was verified present on `origin/main`; the claim is only that a future reversion is invisible.
- **#264** was investigated and **dismissed**: its fix (a `click<=8.1.8` pin, PR #265) was deliberately removed by `bc0d87f` when `typer` was bumped, and `uv.lock` pins `click 8.1.8` / `typer 0.21.1`, so there is no live exposure. Recording it so nobody re-files it. The one residual: there is no root `comfy --help` smoke test anywhere in `tests/` — every `--help` test targets a subcommand — and the root app was the surface #264 broke.
Filed by a closed-bug regression audit (Comfy-Org/comfy-cli, `origin/main` @ 3ff9f55, merge-base 62f35f2).
Contributor guide
Assessment
This issue has not been assessed yet.