From ab85ef4ca934552d2e69a13e037ff22207f1e4d2 Mon Sep 17 00:00:00 2001 From: digital-garbage <133010426+digital-garbage@users.noreply.github.com> Date: Mon, 3 Aug 2026 11:48:58 +0300 Subject: [PATCH] =?UTF-8?q?test:=20kill=20the=2022=20chronic=20failures=20?= =?UTF-8?q?=E2=80=94=20they=20were=20stub=20collisions,=20not=20redundant?= =?UTF-8?q?=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every one of them was a real test of a live feature. 18 failed only in a full-suite run and passed alone, 4 asserted behaviour that was deliberately changed and never got updated. Ordering (18): each test module built its own partial `comfy` stub and registered it with `sys.modules.setdefault(...)`, so the first module to import won and every later module's richer stub was silently discarded — the scene chain sampler then died on `comfy` having no `.sample`, and the mask-safe attention install bailed at its first guard because `import comfy.ldm.modules.attention` resolves through the PARENT's attribute, which one module set and another did not. tests/_comfy_stubs.py now builds that package tree once, wiring every parent attribute, and test modules attach their fakes as attributes on the shared modules (per-test, via fixtures, where the fake is only theirs — the idiom test_detailing.py already used). Same file also stubs `server.PromptServer`, whose absence made every module that imports templates.py (i.e. anything importing conditioning) fail on `No module named 'server'`. Stale assertions (4): * the "every learning rating" loop excluded non-learning ratings by hardcoded LABEL, so the later-added `Wrong appearance|loved` and the two control sentinels were asserted to train phrase memory. Now filtered by profile key, with the reason per key. * three split_by_transitions tests still used "scene ten" / "cut to" as cut markers. The unified splitter cuts on a real transition trigger or the generic `scene ` label, on purpose — an incidental phrase must never split a prompt. Rewritten on the current contract, and they now also pin that the delimiter is dropped rather than encoded. Verified: 584 pass, and every file passes alone, in reverse order, and with movie_editor's suite standalone. Mutation-checked the rewritten split tests (disabling the scene-label pattern fails 4 of 5) so they are not passing vacuously. Co-Authored-By: Claude Opus 5 --- tests/_comfy_stubs.py | 148 ++++++++++++++++++++++++++++ tests/conftest.py | 12 ++- tests/test_mask_safe_attention.py | 33 +++---- tests/test_scene_chain_sampler.py | 60 +++++------ tests/test_v2_category_alignment.py | 29 +++++- 5 files changed, 226 insertions(+), 56 deletions(-) create mode 100644 tests/_comfy_stubs.py diff --git a/tests/_comfy_stubs.py b/tests/_comfy_stubs.py new file mode 100644 index 00000000..9b83607a --- /dev/null +++ b/tests/_comfy_stubs.py @@ -0,0 +1,148 @@ +"""Stand-ins for the ComfyUI runtime, installed once for the whole suite. + +FunPack's modules are ComfyUI custom nodes: importing one pulls in `folder_paths`, `comfy.*` +and (through `templates.py`, which registers its HTTP routes at module scope) `server`. None +of those exist outside a running ComfyUI, so they have to be in `sys.modules` BEFORE the +first test module imports — a conftest is the only hook early enough. + +**Why this file exists at all.** Every test module used to build its own partial `comfy` +stub and register it with `sys.modules.setdefault(...)`. First file wins, so in a full-suite +run a later file's richer stub was silently discarded and its tests failed on a missing +attribute — while passing when run alone. Two shapes of the same bug: + +* `import comfy.sample` resolves through the PARENT's attribute, so a stub that sets + `sys.modules["comfy.sample"]` without also setting `comfy.sample` leaves the import + reading whatever the first file wired (or nothing). +* the same for `comfy.ldm.modules.attention`, which several files replace wholesale. + +So the package tree is built ONCE here, with every parent attribute wired, and test modules +attach their own fakes as ATTRIBUTES on these shared module objects instead of replacing +them. Anything a single file needs only for its own tests belongs in a fixture that restores +the previous value (see `test_detailing.py` / `test_minimax_h3_sampler.py`), because these +objects are shared for the whole session. +""" +import sys +import types + + +def install_module(name, **attrs): + """Register `name` in sys.modules AND on its parent, then set `attrs` on it. + + Returns the module (existing one if already registered, so callers merge rather than + replace). Wiring the parent attribute is the part that is easy to forget and the whole + reason imports used to resolve to a stale stub. + """ + mod = sys.modules.get(name) + if mod is None: + mod = types.ModuleType(name) + sys.modules[name] = mod + if "." in name: + parent_name, _, leaf = name.rpartition(".") + parent = install_module(parent_name) + setattr(parent, leaf, mod) + for key, value in attrs.items(): + setattr(mod, key, value) + return mod + + +def _install_folder_paths(): + install_module( + "folder_paths", + models_dir="", + get_input_directory=lambda: "", + get_output_directory=lambda: "", + get_temp_directory=lambda: "", + get_folder_paths=lambda _name: [], + get_filename_list=lambda _name: [], + ) + + +def _install_server(): + """`server.PromptServer` — route decorators that register nothing and return the fn. + + templates.py decorates ~20 handlers with `@PromptServer.instance.routes.get(...)` at + import time. The handlers are exercised through the Movie Editor's own aiohttp tests, so + the decorator only has to be a no-op that leaves the function callable. + """ + if "server" in sys.modules: + return + + class _Routes: + """aiohttp's RouteTableDef surface, for any verb. + + Generated rather than listed: movie_editor/server.py alone uses get/post/put/patch/ + delete, and a missing verb fails at IMPORT time (the decorators run at module scope), + which reads as a mysterious collection error rather than a missing stub. + """ + + def __init__(self): + self.registered = [] + + def _register(self, method, path): + def decorator(fn): + self.registered.append((method, path, fn)) + return fn + return decorator + + def __getattr__(self, method): + if method.startswith("_"): + raise AttributeError(method) + if method in ("static", "view"): + return lambda *_a, **_k: None + return lambda path, **_kw: self._register(method.upper(), path) + + class _PromptServer: + instance = None + + def __init__(self): + self.routes = _Routes() + self.app = types.SimpleNamespace(router=self.routes) + + def send_sync(self, *_a, **_k): + return None + + _PromptServer.instance = _PromptServer() + install_module("server", PromptServer=_PromptServer) + + +def _install_comfy(): + """The comfy package tree the suite imports, wired parent-to-child. + + Defaults are deliberately inert — a test that needs real behaviour installs it (see the + module docstring). What matters here is that the tree EXISTS and is consistent, so no + file can lose a race to register it. + """ + # NOT installed here: comfy.context_windows and comfy.patcher_extension. The sampler + # feature-detects both, and their tests swap whole modules in per test — pre-creating + # them would leave a parent attribute that shadows the swap (the same resolution rule + # this file exists to fix, pointed the other way). + install_module("comfy") + install_module("comfy.k_diffusion") + install_module("comfy.k_diffusion.sampling") + install_module("comfy.model_sampling") + install_module("comfy.samplers") + install_module("comfy.ldm") + install_module("comfy.ldm.modules") + install_module("comfy.ldm.modules.attention") + install_module("comfy.nested_tensor", NestedTensor=object) + install_module( + "comfy.sample", + prepare_noise=lambda samples, seed, noise_inds=None: None, + sample_custom=lambda *a, **k: None, + ) + install_module( + "comfy.utils", + ProgressBar=lambda total: types.SimpleNamespace(update_absolute=lambda *a, **k: None), + common_upscale=lambda samples, width, height, method, crop: samples, + ) + install_module( + "comfy.model_management", + get_torch_device=lambda: "cpu", + intermediate_device=lambda: "cpu", + ) + + +def install_all(): + _install_folder_paths() + _install_server() + _install_comfy() diff --git a/tests/conftest.py b/tests/conftest.py index 4ec86a22..5e822f63 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,4 +1,10 @@ -import sys -import types +"""Install the ComfyUI stand-ins before any test module imports. -sys.modules.setdefault("folder_paths", types.SimpleNamespace(models_dir="")) +The stubs themselves live in `_comfy_stubs.py` rather than here: test modules need +`install_module` too, and `import conftest` is ambiguous the moment a second conftest.py +exists in the run (movie_editor/tests has one), so they import a uniquely-named module +instead. See `_comfy_stubs` for what is stubbed and why. +""" +import _comfy_stubs + +_comfy_stubs.install_all() diff --git a/tests/test_mask_safe_attention.py b/tests/test_mask_safe_attention.py index 0def0c0f..50554ad9 100644 --- a/tests/test_mask_safe_attention.py +++ b/tests/test_mask_safe_attention.py @@ -13,37 +13,34 @@ import sys import types from pathlib import Path -sys.path.insert(0, str(Path(__file__).resolve().parents[1])) +import _comfy_stubs -# Minimal comfy stubs so `import samplers` works without a full ComfyUI env. -for _name in ( - "comfy", "comfy.k_diffusion", "comfy.k_diffusion.sampling", - "comfy.model_sampling", "comfy.nested_tensor", "comfy.sample", - "comfy.samplers", "comfy.utils", -): - sys.modules.setdefault(_name, types.ModuleType(_name)) -sys.modules["comfy.nested_tensor"].NestedTensor = object +sys.path.insert(0, str(Path(__file__).resolve().parents[1])) import samplers # noqa: E402 def _install_attn_module(*, backend="xformers", with_pytorch_sentinel=True): - """Register a fake comfy.ldm.modules.attention with selectable active backend.""" - am = types.ModuleType("comfy.ldm.modules.attention") + """Point comfy.ldm.modules.attention at a fake with a selectable active backend. + Installed through _comfy_stubs.install_module so the PARENT attribute moves too. Registering + only the sys.modules entry is not enough: `import comfy.ldm.modules.attention as _am` + (what samplers.py does) resolves through `comfy.ldm.modules.attention` the attribute, so + a stub left behind by another test module kept winning here and the install under test + bailed out at its first guard. + """ def attention_xformers(*a, **kw): return "xformers" def attention_pytorch(*a, **kw): return "pytorch" - am.attention_xformers = attention_xformers - am.attention_pytorch = attention_pytorch - am.optimized_attention = attention_xformers if backend == "xformers" else attention_pytorch - for parent in ("comfy", "comfy.ldm", "comfy.ldm.modules", "comfy.ldm.modules.attention"): - sys.modules.setdefault(parent, types.ModuleType(parent)) - sys.modules["comfy.ldm.modules.attention"] = am - return am + return _comfy_stubs.install_module( + "comfy.ldm.modules.attention", + attention_xformers=attention_xformers, + attention_pytorch=attention_pytorch, + optimized_attention=attention_xformers if backend == "xformers" else attention_pytorch, + ) def _fake_model(): diff --git a/tests/test_scene_chain_sampler.py b/tests/test_scene_chain_sampler.py index 45e23a4a..5e6c8440 100644 --- a/tests/test_scene_chain_sampler.py +++ b/tests/test_scene_chain_sampler.py @@ -1,10 +1,11 @@ import sys -import types from pathlib import Path import pytest import torch +import _comfy_stubs + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) @@ -86,16 +87,6 @@ def _sample_snapshot(value): return value.detach().clone() -comfy_mod = types.ModuleType("comfy") -comfy_kd_mod = types.ModuleType("comfy.k_diffusion") -comfy_kd_sampling_mod = types.ModuleType("comfy.k_diffusion.sampling") -comfy_model_sampling_mod = types.ModuleType("comfy.model_sampling") -comfy_nested_mod = types.ModuleType("comfy.nested_tensor") -comfy_sample_mod = types.ModuleType("comfy.sample") -comfy_samplers_mod = types.ModuleType("comfy.samplers") -comfy_utils_mod = types.ModuleType("comfy.utils") - - class FakeProgressBar: """Records what the sampler declared as the run's total step count.""" last = None @@ -109,27 +100,36 @@ class FakeProgressBar: self.value = int(value) -comfy_utils_mod.ProgressBar = FakeProgressBar -comfy_nested_mod.NestedTensor = FakeNestedTensor -comfy_sample_mod.prepare_noise = fake_prepare_noise -comfy_sample_mod.sample_custom = fake_sample_custom +# These fakes ARE the test: a full sample() run is driven through them. They attach to the +# shared comfy stubs conftest built rather than registering modules of their own — the old +# `sys.modules.setdefault(...)` lost the race whenever another test module imported first, +# and every test here then died on a missing attribute in a full-suite run while passing +# alone. Only this file and test_minimax_h3_sampler.py touch the sampling entry points, and +# that one installs inert lambdas, so owning them for the session is safe. +_comfy_stubs.install_module("comfy.utils", ProgressBar=FakeProgressBar) +_comfy_stubs.install_module("comfy.nested_tensor", NestedTensor=FakeNestedTensor) +_comfy_stubs.install_module("comfy.sample", + prepare_noise=fake_prepare_noise, sample_custom=fake_sample_custom) -comfy_mod.k_diffusion = comfy_kd_mod -comfy_kd_mod.sampling = comfy_kd_sampling_mod -comfy_mod.model_sampling = comfy_model_sampling_mod -comfy_mod.nested_tensor = comfy_nested_mod -comfy_mod.sample = comfy_sample_mod -comfy_mod.samplers = comfy_samplers_mod -comfy_mod.utils = comfy_utils_mod -sys.modules.setdefault("comfy", comfy_mod) -sys.modules.setdefault("comfy.k_diffusion", comfy_kd_mod) -sys.modules.setdefault("comfy.k_diffusion.sampling", comfy_kd_sampling_mod) -sys.modules.setdefault("comfy.model_sampling", comfy_model_sampling_mod) -sys.modules.setdefault("comfy.nested_tensor", comfy_nested_mod) -sys.modules.setdefault("comfy.sample", comfy_sample_mod) -sys.modules.setdefault("comfy.samplers", comfy_samplers_mod) -sys.modules.setdefault("comfy.utils", comfy_utils_mod) +@pytest.fixture(autouse=True) +def _own_comfy_stubs(monkeypatch): + """Re-pin this file's fakes around every test. + + Module-level installation is not enough: several other test modules assign + `comfy.nested_tensor.NestedTensor = object` on the same shared module at COLLECTION + time, so whichever is collected last wins and a run() here builds `object()` latents. + Re-pinning per test makes these assertions about the sampler instead of about + collection order.""" + monkeypatch.setattr(sys.modules["comfy.nested_tensor"], "NestedTensor", + FakeNestedTensor, raising=False) + monkeypatch.setattr(sys.modules["comfy.utils"], "ProgressBar", + FakeProgressBar, raising=False) + monkeypatch.setattr(sys.modules["comfy.sample"], "prepare_noise", + fake_prepare_noise, raising=False) + monkeypatch.setattr(sys.modules["comfy.sample"], "sample_custom", + fake_sample_custom, raising=False) + import run_phase # noqa: E402 from samplers import FunPackLTXAVSceneChainSampler # noqa: E402 diff --git a/tests/test_v2_category_alignment.py b/tests/test_v2_category_alignment.py index e0341c35..56c110db 100644 --- a/tests/test_v2_category_alignment.py +++ b/tests/test_v2_category_alignment.py @@ -236,12 +236,22 @@ def test_awful_lora_feedback_reduces_before_missing_axis_boosts(): assert suggestion["model_weight"] < 1.0 +# Ratings that deliberately do NOT train phrase memory, by profile key rather than by +# label text — the label list grows (the "|loved" variants, the control sentinels) and a +# hardcoded exclusion list silently starts asserting the wrong thing about the new ones. +# forget / continue / fresh_prompt — control actions, not ratings at all +# wrong_appearance — a character-consistency signal, handled by the +# appearance anchor, never by phrase categories +NON_LEARNING_RATING_KEYS = {"forget", "wrong_appearance", "continue", "fresh_prompt"} + + def test_category_weights_are_recorded_for_every_learning_rating(): refiner = FunPackVideoRefinerV2() learning_labels = [ label for label in V2_RATING_LABELS - if label not in {"-Just forget it-", "Wrong appearance"} + if normalize_refiner_v2_rating(label).get("key") not in NON_LEARNING_RATING_KEYS ] + assert len(learning_labels) > 10 # the filter must not quietly empty the loop for label in learning_labels: entry, _, _ = train_phrase(refiner, f"running test {label}", label) @@ -1781,8 +1791,11 @@ def test_split_by_transitions_shows_scenes_in_encoded_prompts(tmp_path): state_path = tmp_path / "state.json" refiner._v2_state_path = lambda refinement_key: str(state_path) + # `scene ` is the canonical split delimiter (split_scenes): a real transition trigger + # from the user's DB, or this generic label. Word-numbers ("scene ten") and bare prose + # ("cut to") deliberately do NOT cut — an incidental phrase must never split a prompt. cond, status, _, _, encoded_prompts, _, _ = refiner.refine_v2( - "a woman in a red dress, scene ten she runs, cut to she stops", + "a woman in a red dress, scene 1 she runs, scene 2 she stops", FakeClip(), "Perfect", "transition-test", @@ -1792,9 +1805,15 @@ def test_split_by_transitions_shows_scenes_in_encoded_prompts(tmp_path): assert len(cond) == 2 assert cond[0][1]["funpack_scene_index"] == 0 assert cond[0][1]["funpack_scene_count"] == 2 + # the leading run is the anchor and folds into every scene; each scene keeps its own text assert "a woman in a red dress" in cond[0][1]["funpack_scene_text"] - assert "scene ten she runs" in cond[0][1]["funpack_scene_text"] + assert "a woman in a red dress" in cond[1][1]["funpack_scene_text"] + assert "she runs" in cond[0][1]["funpack_scene_text"] assert "she stops" in cond[1][1]["funpack_scene_text"] + assert "she stops" not in cond[0][1]["funpack_scene_text"] + # the delimiter is a boundary, never content — encoding it would pollute the prompt + assert "scene 1" not in cond[0][1]["funpack_scene_text"] + assert "scene 2" not in cond[1][1]["funpack_scene_text"] assert "Scene chain mode" in status assert "Transition split" in status assert "Detected scenes" in encoded_prompts @@ -2049,7 +2068,7 @@ def test_split_by_transitions_attaches_scene_seeds(tmp_path): refiner._v2_state_path = lambda refinement_key: str(state_path) cond, _, _, _, _, _, _ = refiner.refine_v2( - "anchor, scene one she walks, cut to she turns", + "anchor, scene 1 she walks, scene 2 she turns", FakeClip(), "Perfect", "split-seed-test", @@ -2068,7 +2087,7 @@ def test_split_by_transitions_uses_provided_scene_seeds(tmp_path): refiner._v2_state_path = lambda refinement_key: str(state_path) cond, _, _, _, _, _, _ = refiner.refine_v2( - "anchor, scene one she walks, cut to she turns", + "anchor, scene 1 she walks, scene 2 she turns", FakeClip(), "Perfect", "split-seed-test",