mirror of
https://github.com/Comfy-Org/ComfyUI.git
synced 2026-08-25 18:32:35 +08:00
--feature-flag: strict bool coercion + drop invalid values
Per @rattus128 review on PR #13685: silently coercing typo'd bool values (e.g. `--feature-flag show_signin_button=ture`) to `False` was a confusing UX. Make bool coercion strict and drop unparseable flags entirely. - `_coerce_bool`: accept only `true`/`false` (case-insensitive); raise `ValueError` for anything else (`ture`, `yes`, `1`, ``). - `_coerce_flag_value`: no longer swallows exceptions; raises on bad coercion so the caller decides what to do. - `_parse_cli_feature_flags`: catches `ValueError`/`TypeError`, logs a warning ("dropping flag"), and omits the flag from the result. ComfyUI still starts; `SERVER_FEATURE_FLAGS` retains the registered default; other valid `--feature-flag` entries on the same command line are unaffected. Tests: - `test_bool_typo_raises`: `ture`/`yes`/`1`/`""` all raise ValueError. - `test_failed_int_coercion_raises`: replaces the old "falls back to string" test now that coercion failures propagate. - `test_invalid_bool_value_dropped`: parser drops the bad flag and logs a warning, while a valid sibling flag still parses. 19/19 unit tests pass. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019df5a8-36be-7107-a4af-c7e4f51687df
This commit is contained in:
@@ -1,5 +1,7 @@
|
||||
"""Tests for feature flags functionality."""
|
||||
|
||||
import pytest
|
||||
|
||||
from comfy_api.feature_flags import (
|
||||
get_connection_feature,
|
||||
supports_feature,
|
||||
@@ -116,14 +118,26 @@ class TestCoerceFlagValue:
|
||||
assert _coerce_flag_value("unknown_flag", "true") == "true"
|
||||
assert _coerce_flag_value("unknown_flag", "42") == "42"
|
||||
|
||||
def test_failed_coercion_falls_back_to_string(self, monkeypatch):
|
||||
"""Malformed values for typed flags must not crash; raw string is returned."""
|
||||
def test_bool_typo_raises(self):
|
||||
"""Strict bool: typos like 'ture' or 'yes' must raise so the flag can be dropped."""
|
||||
with pytest.raises(ValueError):
|
||||
_coerce_flag_value("show_signin_button", "ture")
|
||||
with pytest.raises(ValueError):
|
||||
_coerce_flag_value("show_signin_button", "yes")
|
||||
with pytest.raises(ValueError):
|
||||
_coerce_flag_value("show_signin_button", "1")
|
||||
with pytest.raises(ValueError):
|
||||
_coerce_flag_value("show_signin_button", "")
|
||||
|
||||
def test_failed_int_coercion_raises(self, monkeypatch):
|
||||
"""Malformed values for typed flags must raise; caller decides what to do."""
|
||||
monkeypatch.setitem(
|
||||
CLI_FEATURE_FLAG_REGISTRY,
|
||||
"test_int_flag",
|
||||
{"type": "int", "default": 0, "description": "test"},
|
||||
)
|
||||
assert _coerce_flag_value("test_int_flag", "not_a_number") == "not_a_number"
|
||||
with pytest.raises(ValueError):
|
||||
_coerce_flag_value("test_int_flag", "not_a_number")
|
||||
|
||||
|
||||
class TestParseCliFeatureFlags:
|
||||
@@ -145,6 +159,19 @@ class TestParseCliFeatureFlags:
|
||||
result = _parse_cli_feature_flags()
|
||||
assert result == {"valid": "1"}
|
||||
|
||||
def test_invalid_bool_value_dropped(self, monkeypatch, caplog):
|
||||
"""A typo'd bool value must be dropped entirely, not silently set to False
|
||||
and not stored as a raw string. A warning must be logged."""
|
||||
monkeypatch.setattr(
|
||||
"comfy_api.feature_flags.args",
|
||||
type("Args", (), {"feature_flag": ["show_signin_button=ture", "valid=1"]})(),
|
||||
)
|
||||
with caplog.at_level("WARNING"):
|
||||
result = _parse_cli_feature_flags()
|
||||
assert result == {"valid": "1"}
|
||||
assert "show_signin_button" not in result
|
||||
assert any("show_signin_button" in r.message and "drop" in r.message.lower() for r in caplog.records)
|
||||
|
||||
|
||||
class TestCliFeatureFlagRegistry:
|
||||
"""Test suite for the CLI feature flag registry."""
|
||||
|
||||
Reference in New Issue
Block a user