Five review findings of the P3 phase, each fixed at its root:
1. (medium) The P3.7 metadata test imported `tomllib` at module level: the
**3.10 CI job** (the declared `requires-python >=3.10` minimum) died
during collection, so the registry-metadata assertions never ran on 3.10.
The parser import is now guarded (`tomllib` -> `tomli`, the package the
3.10 job installs); without any parser the module *skips* instead of
dying. `pyproject.toml` gained the test extra
`tomli>=2.0; python_version < '3.11'`.
Proof: a 3.10 simulation (the stdlib `tomllib` refused for the test
module, `tomli` on sys.path) -> 6 passed; the control without `tomli`
-> 6 skipped (no collection error).
2. (medium) The P3.5 "primary-root-first multi-root" resolution was not
wired into the production load path: `resolve_model_file` (the only
`get_full_path_or_raise` caller) was used by tests only, so a weight in
a secondary model root was downloaded into the primary root again.
`src/keep_paths.py` now has `find_model_file()` (the multi-root lookup
through `folder_paths.get_full_path_or_raise`) and `resolve_model_root()`
(the root the weights really live in); `load_file_from_url_comfy()` - the
production path of `src/keep_model_loader.py:107` - uses the lookup, and
the facelib root follows the weights (the facelib helper builds its
paths from a single root). New tests prove that the production path
uses a weight of an extra model root and that the primary root wins.
The `models_dir` grep guard is no longer a blanket exemption: only the
registration in `src/keep_paths.py` may use the attribute.
3. (low) `[tool.comfy].Icon` was an empty string and the test only checked
that the key exists. The preview asset **is** committed
(`git cat-file -s HEAD:assets/keep-preview.png` -> 93279 bytes); only the
working copy was missing (an uncommitted local deletion, restored from
HEAD - no asset change is in this commit). The key now points at
`assets/keep-preview.png` and the test asserts that the value is not
empty and names a file that is really in the repository.
4. (low) `tests/test_package_entrypoint.py` hard-coded the V1 state and
would have gone red (not xfail) when P4.4 flips the root `__init__.py`
to `comfy_entrypoint`. The assertions now follow the entry point that is
really there: exactly **one** mapping style (V1 XOR V3). The mocked
smoke test of `tests/test_package_import.py` got the same treatment; the
headless variant is P4.4's job (its test list names it).
5. (low) The plan's §3 rule ("`comfy.*` imports inside `src/` are lazy
(function-level) so the pack imports without a ComfyUI server") was
violated and unguarded. The `comfy` imports of `src/keep_model_loader.py`
and `src/keep_processor.py` are function-level now; the vendored facelib
helper (which imports `comfy` itself) is imported lazily through a module
`__getattr__` (PEP 562), so `loader.FaceRestoreHelper` still resolves to
the vendored class. Structure **guard (f)** (`tests/test_structure.py`)
forbids a module-level `comfy`/`comfy_api` import in `src/**` (with the
self-test the guards file demands), and the new
`tests/test_lazy_imports.py` proves at runtime that the engines import
while every `comfy` import is blocked.
Gate after the commit:
python.exe -m pytest -q tests -m "unit or mock_integration"
-> 153 passed, 1 skipped, 5 deselected, 3 xfailed
COMFYUI_ROOT=C:/_Dev/ComfyUI_dev/ComfyUI-source/ComfyUI python.exe
-m pytest -q tests -m "unit or mock_integration or headless"
-> 158 passed, 1 skipped, 3 xfailed
Details: docs/2026-10-23-p3-review-findings.md
152 lines
4.9 KiB
Python
152 lines
4.9 KiB
Python
"""P3.2 / P3.6 - the package imports.
|
|
|
|
The package is imported the way ComfyUI imports it (the root ``__init__.py``
|
|
is executed under an alias) - once against the **real** ComfyUI source tree
|
|
(the ``headless`` tier, P3.2) and once against the **mocks** of the root
|
|
conftest (the ``mock_integration`` tier, P3.6).
|
|
|
|
The headless variant needs the ComfyUI source tree: without ``COMFYUI_ROOT``
|
|
the test **skips** with a message that names the variable (the acceptance
|
|
gate runs without environment variables).
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import importlib
|
|
import json
|
|
import os
|
|
import subprocess
|
|
import sys
|
|
from pathlib import Path
|
|
|
|
import pytest
|
|
|
|
REPO_ROOT = Path(__file__).resolve().parent.parent
|
|
HEADLESS_HELPER = REPO_ROOT / "tests" / "_headless.py"
|
|
|
|
#: The environment variable that points at the ComfyUI source tree.
|
|
COMFYUI_ROOT_ENV = "COMFYUI_ROOT"
|
|
|
|
#: The node ids of the pack.
|
|
PACK_NODE_IDS = [
|
|
"KEEP_FaceUpscaleImage",
|
|
"KEEP_ModelLoader",
|
|
"KEEP_ProcessImageSequence",
|
|
]
|
|
|
|
#: The probe: it registers the package the way ComfyUI does (the root
|
|
#: ``__init__.py`` is executed under an alias) and reports the result.
|
|
_HEADLESS_PROBE = """
|
|
import json, os, sys
|
|
|
|
comfyui_root, repo_root = sys.argv[1], sys.argv[2]
|
|
sys.path.insert(0, comfyui_root)
|
|
sys.path.insert(0, repo_root)
|
|
sys.path.insert(0, os.path.join(repo_root, "tests"))
|
|
|
|
import _headless
|
|
|
|
module = _headless.register_package(
|
|
repo_root, alias="ComfyUI_KEEP_headless", execute=True
|
|
)
|
|
print(
|
|
"KEEP_IMPORT_REPORT:"
|
|
+ json.dumps(
|
|
{
|
|
"node_ids": sorted(getattr(module, "NODE_CLASS_MAPPINGS", {})),
|
|
"display_names": dict(
|
|
getattr(module, "NODE_DISPLAY_NAME_MAPPINGS", {})
|
|
),
|
|
"has_logger": hasattr(module, "logger"),
|
|
}
|
|
)
|
|
)
|
|
"""
|
|
|
|
|
|
def _comfyui_root() -> str:
|
|
"""The ComfyUI source tree - or a skip that names the variable."""
|
|
value = (os.environ.get(COMFYUI_ROOT_ENV) or "").strip()
|
|
if not value:
|
|
pytest.skip(
|
|
f"{COMFYUI_ROOT_ENV} is not set - the headless import needs the "
|
|
"ComfyUI source tree on sys.path"
|
|
)
|
|
root = Path(value).expanduser()
|
|
if not (root / "folder_paths.py").is_file():
|
|
pytest.skip(
|
|
f"{COMFYUI_ROOT_ENV}={value!r} is not a ComfyUI source tree "
|
|
"(folder_paths.py is missing)"
|
|
)
|
|
return str(root.resolve())
|
|
|
|
|
|
@pytest.mark.mock_integration
|
|
def test_the_package_imports_for_real_under_the_mocks():
|
|
"""P3.6: the package imports for real under the mocks - no exception.
|
|
|
|
This is the strict version of the import smoke test: the root
|
|
``__init__.py`` is executed and the three node mappings are exported.
|
|
The test follows the entry point that is really there (V1 today, the V3
|
|
``comfy_entrypoint`` from P4.4 on).
|
|
"""
|
|
conftest = importlib.import_module("conftest")
|
|
package = conftest.execute_package_init()
|
|
if hasattr(package, "comfy_entrypoint"):
|
|
# V3 (P4.4): the entry point is the only registration surface
|
|
assert callable(package.comfy_entrypoint)
|
|
return
|
|
assert sorted(package.NODE_CLASS_MAPPINGS) == PACK_NODE_IDS
|
|
assert sorted(package.NODE_DISPLAY_NAME_MAPPINGS) == PACK_NODE_IDS
|
|
|
|
|
|
@pytest.mark.headless
|
|
def test_the_package_imports_against_the_real_comfyui_tree():
|
|
"""P3.2: the whole package imports against the real ComfyUI source tree."""
|
|
comfyui_root = _comfyui_root()
|
|
result = subprocess.run(
|
|
[sys.executable, "-c", _HEADLESS_PROBE, comfyui_root, str(REPO_ROOT)],
|
|
cwd=str(REPO_ROOT),
|
|
capture_output=True,
|
|
text=True,
|
|
timeout=600,
|
|
)
|
|
assert result.returncode == 0, (
|
|
f"the package did not import\nstdout:\n{result.stdout}\n"
|
|
f"stderr:\n{result.stderr}"
|
|
)
|
|
sentinel = [
|
|
line for line in result.stdout.splitlines() if line.startswith("KEEP_IMPORT_REPORT:")
|
|
]
|
|
assert sentinel, (
|
|
f"the probe produced no report\nstdout:\n{result.stdout}\n"
|
|
f"stderr:\n{result.stderr}"
|
|
)
|
|
report = json.loads(sentinel[-1].split(":", 1)[1])
|
|
assert report["node_ids"] == PACK_NODE_IDS, report
|
|
assert report["display_names"]["KEEP_ModelLoader"] == "Load KEEP Models", report
|
|
assert report["has_logger"] is True, report
|
|
|
|
|
|
@pytest.mark.headless
|
|
def test_the_headless_helper_knows_the_package():
|
|
"""The P1.4 helper resolves the package directory of this checkout."""
|
|
comfyui_root = _comfyui_root()
|
|
result = subprocess.run(
|
|
[
|
|
sys.executable,
|
|
"-c",
|
|
"import sys; sys.path.insert(0, sys.argv[2]); import _headless; "
|
|
"print('KEEP_ALIAS', _headless.alias_for(sys.argv[1]))",
|
|
str(REPO_ROOT),
|
|
str(REPO_ROOT / "tests"),
|
|
],
|
|
cwd=str(REPO_ROOT),
|
|
capture_output=True,
|
|
text=True,
|
|
timeout=600,
|
|
)
|
|
assert result.returncode == 0, result.stdout + result.stderr
|
|
assert "KEEP_ALIAS ComfyUI_KEEP" in result.stdout, result.stdout
|
|
assert Path(comfyui_root).is_dir()
|