Skip to content

Commit 7552cc3

Browse files
committed
test: cover the raising-renderer path and pin resolve_feature parity (os-016)
Review follow-ups on the resolve_or_raise refactor: - resolve_feature now degrades a raising evaluate_and_render into ResolvedFeature.error instead of propagating it. That path is covered, including its fail-closed empty candidate list and the scope callout. - New parity tests pin the one converged call site that does not delegate: resolve_feature projects exactly the message and candidates that resolve_or_raise raises, across all three failure kinds. - test_raises_for_every_failure_kind compares against render_resolution_failure over an independent evaluate() pass instead of the helper's own return value. - Dropped two isinstance assertions that could never fail. Docs: identify_seam.py records that evaluate_or_raise delegates to resolve_or_raise and stays the seam resolution tests target, systemPatterns.md names the shared helper in the resolve step, and the resolve_feature design note is rewrapped.
1 parent e1bfcba commit 7552cc3

4 files changed

Lines changed: 186 additions & 19 deletions

File tree

memory-bank/systemPatterns.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ flowchart LR
4848
2. **Parse**: `FeatureChainParser.parse_name` (`feature_chain_parser.py`) returns a frozen `ParsedFeatureName` (`parsed_feature_name.py`) recording exactly what `re` found. `operation_part` is the raw suffix, never a fabricated token.
4949
3. **Bind** (`feature_chain_parser.py`): `bind_name_captures` binds named captures exclusively by name into an effective `Options`; a captureless pattern is a recognition predicate that identifies the group and binds nothing. A present option value always wins over a name-derived one. A transitional positional fallback (`_legacy_operation_config`) still reverse-looks-up single-capture legacy patterns, pending downstream migration (mloda-registry#327).
5050
4. **Match**: `match_configuration_feature_chain_parser` validates present values (a bad value raises `PropertyValueRejection`, a `ValueError` verdict rather than a crash) and enforces required presence on the string-named path.
51-
5. **Resolve** (`prepare/identify_feature_group.py`): `IdentifyFeatureGroupClass.evaluate` runs one non-raising resolution pass, recording per-candidate elimination facts (`EvaluationResult`, `Elimination`) so a failure explains which gate each near-miss failed. Class-definition-time checks reject order-dependent bindings and install guards enforcing `required_when` and name-path required presence; an all-optional matcher that would match any name emits a definition-time warning unless the class sets `ALLOW_UNIVERSAL_MATCHER`.
51+
5. **Resolve** (`prepare/identify_feature_group.py`): the engine and the resolution test seam both enter through `resolve_or_raise` (the shared evaluate-render-raise helper, built on `evaluate_and_render`), under which `IdentifyFeatureGroupClass.evaluate` remains the non-raising matcher: it runs one resolution pass, recording per-candidate elimination facts (`EvaluationResult`, `Elimination`) so a failure explains which gate each near-miss failed. Class-definition-time checks reject order-dependent bindings and install guards enforcing `required_when` and name-path required presence; an all-optional matcher that would match any name emits a definition-time warning unless the class sets `ALLOW_UNIVERSAL_MATCHER`.
5252
6. **Materialize defaults at intake, then compute** (os-008): `Engine.add_feature_to_collection` rebinds each resolved feature's options through `options_with_defaults()` as it enters the plan, so default-equivalent twins (same name, one key explicitly set to its declared default) merge through the standard duplicate path with uuid remapping. `ComputeFramework.run_calculate_feature` (`@final`) still calls `FeatureSet.materialize_option_defaults` as an idempotent safety net for direct API use; its twin-collapse ValueError is unreachable from engine-driven requests. `options_with_defaults()` fills only absent keys that declare a concrete default; `NO_DEFAULT` and a declared `None` fill nothing. Presence honors the explicit-`None` policy: a present `None` counts as set only when the spec sets `allow_explicit_none=True`.
5353

5454
Which lifecycle stages observe which options view:

mloda/core/api/plugin_docs.py

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -345,11 +345,10 @@ def resolve_feature(
345345
Signature misuse (options/feature_group alongside a Feature) is a programmer error and raises TypeError.
346346
347347
Design note: resolve_feature is a thin adapter. It only normalizes the standalone request, builds the
348-
canonical accessible-plugins environment once, delegates one evaluation to
349-
evaluate_and_render, and projects the result. ANY environment-build failure (including
350-
redefinition conflicts) is projected fail-closed from the failure itself into ``error`` with no
351-
candidates and no re-matching; the seam owns name/domain/scope/abstract/subclass filtering, the
352-
winner, candidates, and the failure texts.
348+
canonical accessible-plugins environment once, delegates one evaluation to evaluate_and_render, and
349+
projects the result. ANY environment-build failure (including redefinition conflicts) is projected
350+
fail-closed from the failure itself into ``error`` with no candidates and no re-matching; the seam owns
351+
name/domain/scope/abstract/subclass filtering, the winner, candidates, and the failure texts.
353352
354353
Engine inputs now covered: name, options, domain and compute-framework pin (carried on the Feature),
355354
scope (via the Feature's feature_group_scope or the feature_group argument for the string form), and

tests/test_core/test_prepare/identify_seam.py

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
- EvaluationResult fields (identified, criteria_matched, abstract_matched, candidate_frameworks,
88
eliminations, facts) and the derived failure_kind property, for structured assertions.
99
10+
evaluate_or_raise delegates to identify_feature_group.resolve_or_raise; target this seam, not the helper directly.
11+
1012
Exact failure-message wording is out of scope: it is inherently wording-coupled, so prefer asserting on
1113
structured facts and reserve exact-string checks for tests whose contract is the wording itself.
1214
"""

tests/test_core/test_prepare/test_resolve_or_raise.py

Lines changed: 179 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,10 @@
1313
``ComputeFrameworkPinError`` is a misuse validated before matching, so it escapes both helpers unconverted, and
1414
the error ``resolve_or_raise`` raises must be equivalent to the one the os-014 seam raises for the same feature.
1515
16+
The third converged call site, ``resolve_feature``, does not delegate to the helper: it projects the pair's
17+
result into a ``ResolvedFeature`` behind a never-raise guard that now covers rendering as well as evaluation.
18+
Both halves of that are pinned here, against the helper itself.
19+
1620
All fixture names carry an ``016`` suffix: test feature groups become global subclasses and the suite runs in
1721
parallel, so a shared name would leak into another module's candidate universe.
1822
"""
@@ -26,10 +30,15 @@
2630
from mloda.core.abstract_plugins.components.domain import Domain
2731
from mloda.core.abstract_plugins.components.feature import Feature
2832
from mloda.core.abstract_plugins.components.feature_name import FeatureName
33+
from mloda.core.abstract_plugins.components.link import Link
2934
from mloda.core.abstract_plugins.components.options import Options
35+
from mloda.core.abstract_plugins.components.plugin_option.plugin_collector import PluginCollector
3036
from mloda.core.abstract_plugins.compute_framework import ComputeFramework
3137
from mloda.core.abstract_plugins.feature_group import FeatureGroup
32-
from mloda.core.prepare.accessible_plugins import FeatureGroupEnvironmentMapping
38+
from mloda.core.api import plugin_docs
39+
from mloda.core.api.plugin_docs import resolve_feature
40+
from mloda.core.api.plugin_info import ResolvedFeature
41+
from mloda.core.prepare.accessible_plugins import FeatureGroupEnvironmentMapping, PreFilterPlugins
3342
from mloda.core.prepare.identify_feature_group import (
3443
PARTIAL_RECORDS_CAP,
3544
ComputeFrameworkPinError,
@@ -40,6 +49,7 @@
4049
evaluate_and_render,
4150
render_resolution_failure,
4251
resolve_or_raise,
52+
scope_callout,
4353
)
4454
from tests.test_core.test_prepare.identify_seam import evaluate_or_raise
4555

@@ -167,6 +177,35 @@ def _plugins_for(feature_name: str) -> FeatureGroupEnvironmentMapping:
167177
return _match_plugins()
168178

169179

180+
def _frameworks_016() -> set[type[ComputeFramework]]:
181+
"""A fresh compute-framework restriction, the one resolve_feature accepts as a keyword."""
182+
return {ResolveOrRaiseFw016}
183+
184+
185+
def _collector_for(feature_name: str) -> PluginCollector:
186+
"""A collector restricting the universe to exactly the fixture groups behind one failure kind."""
187+
if feature_name == RESOLVE_MULTIPLE_FEATURE_016:
188+
return PluginCollector.enabled_feature_groups({ResolveOrRaiseSiblingAFG016, ResolveOrRaiseSiblingBFG016})
189+
if feature_name == RESOLVE_ABSTRACT_FEATURE_016:
190+
return PluginCollector.enabled_feature_groups({ResolveOrRaiseAbstractFG016})
191+
return PluginCollector.enabled_feature_groups({ResolveOrRaiseMatchFG016})
192+
193+
194+
def _built_plugins_for(feature_name: str) -> FeatureGroupEnvironmentMapping:
195+
"""The environment resolve_feature builds for that fixture, via the same PreFilterPlugins path."""
196+
return PreFilterPlugins(_frameworks_016(), _collector_for(feature_name)).get_accessible_plugins()
197+
198+
199+
def _resolve_feature_016(feature_name: str, scope: Optional[type[FeatureGroup]] = None) -> ResolvedFeature:
200+
"""Run resolve_feature over the fixture universe of one failure kind."""
201+
return resolve_feature(
202+
feature_name,
203+
feature_group=scope,
204+
plugin_collector=_collector_for(feature_name),
205+
compute_frameworks=_frameworks_016(),
206+
)
207+
208+
170209
def _empty_result() -> EvaluationResult:
171210
"""A minimal EvaluationResult for cheap ResolutionRecord construction."""
172211
return EvaluationResult(identified={})
@@ -204,6 +243,19 @@ def _raised_with_records(records: list[ResolutionRecord]) -> FeatureResolutionEr
204243
]
205244

206245

246+
RENDER_EXPLOSION_016 = "resolve_or_raise_016 render step exploded"
247+
248+
249+
def _exploding_evaluate_and_render(
250+
feature: Feature,
251+
accessible_plugins: FeatureGroupEnvironmentMapping,
252+
links: Optional[set[Link]] = None,
253+
data_access_collection: Optional[DataAccessCollection] = None,
254+
) -> tuple[EvaluationResult, str | None]:
255+
"""Stand-in for the helper pair that always raises, standing for a renderer that blows up."""
256+
raise RuntimeError(RENDER_EXPLOSION_016)
257+
258+
207259
class TestEvaluateAndRenderSuccess:
208260
"""On a resolvable feature the pair returns evaluate()'s result and no message."""
209261

@@ -360,7 +412,7 @@ def test_error_result_carries_the_same_evaluation(self) -> None:
360412

361413
@pytest.mark.parametrize(("failure_kind", "feature_name"), FAILURE_CASES)
362414
def test_raises_for_every_failure_kind(self, failure_kind: str, feature_name: str) -> None:
363-
"""Each of the three failure kinds raises, and the message matches the pair's rendered one."""
415+
"""Each of the three failure kinds raises with the renderer's own message over the same evaluation."""
364416
feature = Feature(feature_name)
365417
accessible_plugins = _plugins_for(feature_name)
366418

@@ -371,15 +423,12 @@ def test_raises_for_every_failure_kind(self, failure_kind: str, feature_name: st
371423
links=None,
372424
data_access_collection=None,
373425
)
374-
_, message = evaluate_and_render(
375-
feature=feature,
376-
accessible_plugins=_plugins_for(feature_name),
377-
links=None,
378-
data_access_collection=None,
379-
)
426+
direct = IdentifyFeatureGroupClass.evaluate(feature, _plugins_for(feature_name), links=None)
427+
expected = render_resolution_failure(direct, feature)
380428

381429
assert exc_info.value.result.failure_kind == failure_kind
382-
assert str(exc_info.value) == message
430+
assert expected is not None
431+
assert str(exc_info.value) == expected
383432

384433

385434
class TestResolveOrRaisePartialRecords:
@@ -437,16 +486,14 @@ class TestComputeFrameworkPinErrorEscapesBothHelpers:
437486

438487
def test_evaluate_and_render_propagates_the_pin_error(self) -> None:
439488
"""evaluate_and_render lets ComputeFrameworkPinError out instead of rendering a message."""
440-
with pytest.raises(ComputeFrameworkPinError) as exc_info:
489+
with pytest.raises(ComputeFrameworkPinError):
441490
evaluate_and_render(
442491
feature=_pinned_feature(RESOLVE_NO_MATCH_FEATURE_016),
443492
accessible_plugins=_match_plugins(),
444493
links=None,
445494
data_access_collection=None,
446495
)
447496

448-
assert not isinstance(exc_info.value, FeatureResolutionError)
449-
450497
def test_resolve_or_raise_propagates_the_pin_error(self) -> None:
451498
"""resolve_or_raise lets the same pin error out, unconverted, with its own wording."""
452499
feature = _pinned_feature(RESOLVE_NO_MATCH_FEATURE_016)
@@ -459,7 +506,6 @@ def test_resolve_or_raise_propagates_the_pin_error(self) -> None:
459506
data_access_collection=None,
460507
)
461508

462-
assert not isinstance(exc_info.value, FeatureResolutionError)
463509
assert ResolveOrRaiseFw016.get_class_name() in str(exc_info.value)
464510
assert ResolveOrRaiseFwBeta016.get_class_name() in str(exc_info.value)
465511

@@ -501,3 +547,123 @@ def test_helper_and_seam_errors_are_equivalent(self, failure_kind: str, feature_
501547
assert helper_info.value.feature_name == seam_info.value.feature_name
502548
assert helper_info.value.result.failure_kind == failure_kind
503549
assert seam_info.value.result.failure_kind == failure_kind
550+
551+
552+
class TestResolveFeatureCallSiteParity:
553+
"""resolve_feature projects exactly what resolve_or_raise raises for the same environment.
554+
555+
The seam above delegates to resolve_or_raise, so it re-enters the same function; resolve_feature does
556+
not, which makes it the one call site whose agreement can actually break. It takes no accessible-plugins
557+
mapping, only a collector and a compute-framework restriction, so the environment is rebuilt through the
558+
PreFilterPlugins path it uses internally instead of the hand-built fixture mappings. All three failure
559+
kinds are expressible that way, which the environment test below pins.
560+
561+
Complements test_single_pass_exactly_once.py::TestEngineAndResolveFeatureAgree, whose reference is the
562+
engine seam and whose subject is the per-attempt hook budget; here the reference is the helper itself.
563+
"""
564+
565+
def test_the_rebuilt_environment_is_the_fixture_universe(self) -> None:
566+
"""The collector projection yields exactly the fixture groups, so both call sites see one universe."""
567+
assert _built_plugins_for(RESOLVE_NO_MATCH_FEATURE_016) == _match_plugins()
568+
assert _built_plugins_for(RESOLVE_ABSTRACT_FEATURE_016) == _abstract_plugins()
569+
assert set(_built_plugins_for(RESOLVE_MULTIPLE_FEATURE_016)) == {
570+
ResolveOrRaiseSiblingAFG016,
571+
ResolveOrRaiseSiblingBFG016,
572+
}
573+
574+
@pytest.mark.parametrize(("failure_kind", "feature_name"), FAILURE_CASES)
575+
def test_resolve_feature_error_is_the_error_the_helper_raises(self, failure_kind: str, feature_name: str) -> None:
576+
"""Same string on both call sites, for all three failure kinds."""
577+
with pytest.raises(FeatureResolutionError) as exc_info:
578+
resolve_or_raise(
579+
feature=Feature(feature_name),
580+
accessible_plugins=_built_plugins_for(feature_name),
581+
links=None,
582+
data_access_collection=None,
583+
)
584+
resolved = _resolve_feature_016(feature_name)
585+
586+
assert exc_info.value.result.failure_kind == failure_kind
587+
assert resolved.feature_group is None
588+
assert resolved.error == str(exc_info.value)
589+
590+
@pytest.mark.parametrize(("failure_kind", "feature_name"), FAILURE_CASES)
591+
def test_resolve_feature_candidates_are_that_error_criteria_matched(
592+
self, failure_kind: str, feature_name: str
593+
) -> None:
594+
"""The projected candidates are the failing evaluation's criteria-matched groups, sorted by name."""
595+
with pytest.raises(FeatureResolutionError) as exc_info:
596+
resolve_or_raise(
597+
feature=Feature(feature_name),
598+
accessible_plugins=_built_plugins_for(feature_name),
599+
links=None,
600+
data_access_collection=None,
601+
)
602+
resolved = _resolve_feature_016(feature_name)
603+
604+
expected = sorted(candidate.get_class_name() for candidate in exc_info.value.result.criteria_matched)
605+
assert [candidate.get_class_name() for candidate in resolved.candidates] == expected
606+
607+
def test_a_resolvable_feature_agrees_on_the_winner(self) -> None:
608+
"""Success side of the same parity: the helper's identified group is the ResolvedFeature's winner."""
609+
result = resolve_or_raise(
610+
feature=Feature(RESOLVE_MATCH_FEATURE_016),
611+
accessible_plugins=_built_plugins_for(RESOLVE_MATCH_FEATURE_016),
612+
links=None,
613+
data_access_collection=None,
614+
)
615+
resolved = _resolve_feature_016(RESOLVE_MATCH_FEATURE_016)
616+
617+
assert resolved.error is None
618+
assert resolved.feature_group is next(iter(result.identified))
619+
assert resolved.feature_group is ResolveOrRaiseMatchFG016
620+
621+
622+
class TestResolveFeatureDegradesARaisingHelper:
623+
"""resolve_feature guards evaluation AND rendering, so a raising helper becomes an error result.
624+
625+
The one intentional behaviour delta of the convergence: the never-raise guard now wraps the whole
626+
evaluate-plus-render pair, so a renderer that blows up can no longer escape the debug API. The
627+
degraded result is fail-closed exactly like the pre-existing raising-evaluate path: no candidates, and
628+
the scope callout appended by resolve_feature because no rendered message carried one.
629+
"""
630+
631+
def test_a_raising_helper_becomes_the_error_instead_of_escaping(self, monkeypatch: pytest.MonkeyPatch) -> None:
632+
"""The call returns a ResolvedFeature carrying the raised message rather than propagating it."""
633+
monkeypatch.setattr(plugin_docs, "evaluate_and_render", _exploding_evaluate_and_render)
634+
635+
resolved = _resolve_feature_016(RESOLVE_MATCH_FEATURE_016)
636+
637+
assert isinstance(resolved, ResolvedFeature)
638+
assert resolved.feature_name == RESOLVE_MATCH_FEATURE_016
639+
assert resolved.feature_group is None
640+
assert resolved.error is not None
641+
assert RENDER_EXPLOSION_016 in resolved.error
642+
643+
def test_the_degraded_path_reports_no_candidates(self, monkeypatch: pytest.MonkeyPatch) -> None:
644+
"""Fail-closed: nothing is re-matched to fill in candidates behind the failure."""
645+
monkeypatch.setattr(plugin_docs, "evaluate_and_render", _exploding_evaluate_and_render)
646+
647+
resolved = _resolve_feature_016(RESOLVE_MATCH_FEATURE_016)
648+
649+
assert resolved.candidates == []
650+
651+
def test_the_degraded_path_appends_the_scope_callout(self, monkeypatch: pytest.MonkeyPatch) -> None:
652+
"""A scoped request still names its scope once, as on the raising-evaluate path."""
653+
monkeypatch.setattr(plugin_docs, "evaluate_and_render", _exploding_evaluate_and_render)
654+
655+
resolved = _resolve_feature_016(RESOLVE_MATCH_FEATURE_016, scope=ResolveOrRaiseMatchFG016)
656+
657+
callout = scope_callout(ResolveOrRaiseMatchFG016)
658+
assert callout is not None
659+
assert resolved.error is not None
660+
assert RENDER_EXPLOSION_016 in resolved.error
661+
assert resolved.error.endswith(callout)
662+
assert resolved.error.count(callout) == 1
663+
664+
def test_the_same_call_resolves_without_the_raising_helper(self) -> None:
665+
"""Control: the degradation above is caused by the raise, not by the fixture environment."""
666+
resolved = _resolve_feature_016(RESOLVE_MATCH_FEATURE_016)
667+
668+
assert resolved.error is None
669+
assert resolved.feature_group is ResolveOrRaiseMatchFG016

0 commit comments

Comments
 (0)