Low/Medium: The new stub drift test can miss important signature drift
tests/test_stub_drift.py compares only parameter names:
runtime_params = list(runtime_sig.parameters.keys())
assert stub_params == runtime_params
It does not compare parameter kind, defaults, required/optional status, or annotations. It also silently skips a class when inspect.signature(cls) raises ValueError.
Impact: this test can pass even when the stub and runtime disagree in ways that matter to users. For example, the PR intentionally makes ComponentGraphConfig.__init__ keyword-only by adding * in both the .pyi and the #[pyo3(signature = (*, ...))] runtime signature. A future drift where the stub says keyword-only but the runtime accepts positional args, or the opposite, would not be detected by the current comparison because both signatures can have the same parameter names.
Suggested fix: compare at least:
[(name, param.kind, param.default is inspect.Parameter.empty)
for name, param in runtime_sig.parameters.items()]
against equivalent data parsed from the stub AST. For stubs, distinguish args.args from args.kwonlyargs, and check whether defaults exist. I would also avoid silently skipping ValueError for public classes, or make the skip explicit per-class with a strong reason, because otherwise a broken __text_signature__ can hide exactly the drift this test is meant to catch.
Originally posted by @llucax in #63 (comment)
Low/Medium: The new stub drift test can miss important signature drift
tests/test_stub_drift.pycompares only parameter names:It does not compare parameter kind, defaults, required/optional status, or annotations. It also silently skips a class when
inspect.signature(cls)raisesValueError.Impact: this test can pass even when the stub and runtime disagree in ways that matter to users. For example, the PR intentionally makes
ComponentGraphConfig.__init__keyword-only by adding*in both the.pyiand the#[pyo3(signature = (*, ...))]runtime signature. A future drift where the stub says keyword-only but the runtime accepts positional args, or the opposite, would not be detected by the current comparison because both signatures can have the same parameter names.Suggested fix: compare at least:
against equivalent data parsed from the stub AST. For stubs, distinguish
args.argsfromargs.kwonlyargs, and check whether defaults exist. I would also avoid silently skippingValueErrorfor public classes, or make the skip explicit per-class with a strong reason, because otherwise a broken__text_signature__can hide exactly the drift this test is meant to catch.Originally posted by @llucax in #63 (comment)