test(engine): discriminating Devour post-replacement regression tests (Fixes #575)

[kiannidev]

Summary

• Replace the non-discriminating Dralnu integration smoke test with unit tests that call apply_post_replacement_effect directly.
• Assert DealtDamage + Sacrifice { Typed } injects the source as a pre-selected target (Dralnu path).
• Assert Moved + Sacrifice { Typed } suppresses source injection and opens EffectZoneChoice (Devour path).

Fixes #575

Test plan

• cargo test -p engine --lib issue_575

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[matthewevans]

/review-impl — PR #2795 (Devour post-replacement regression tests)

Verdict: APPROVE. This is a genuine test-quality improvement, correctly labeled test(...). No production behavior changes.

(a) What the engine_replacement.rs change does

Despite touching a production file, the only change there is inside mod tests (single hunk @@ -1847,6 +1847,120 @@ mod tests). I diffed against origin/main (ad3e8d2b): the production guard is_as_enters_sacrifice_scope_replacement and its event == Some(ReplacementEvent::Moved) gating are unchanged — they already exist on main. The PR adds two unit tests of that existing guard via apply_post_replacement_effect. The test(...) label is accurate.

(b) Does the -233 deletion lose coverage? Are the new tests discriminating?

The deleted dralnu_dealt_damage_sacrifice.rs self-documented as "a SHAPE + SMOKE regression test, not a fail-first discriminating test" — it cast Bolt at Dralnu and asserted Dralnu dies to lethal-damage SBA (CR 704.5g), which happens regardless of the guard. It never exercised the chooser-vs-injection branch the guard controls. So nothing discriminating was lost.

The two replacements are genuinely discriminating, traced against the live code paths:

• issue_575_dealt_damage_sacrifice_injects_source_target: event=DealtDamage → guard false → [Object(dralnu)] injected → sacrifice::resolve takes the targeted_objects non-empty branch → Dralnu→Graveyard. Fails if the guard re-broadens (drop the Moved check): DealtDamage+Typed would then suppress injection → EffectZoneChoice → Dralnu stays. Confirmed discriminating.
• issue_575_moved_sacrifice_typed_opens_chooser_not_source_injection: event=Moved → guard true → targets [] → eligible pool {devourer,fodder_a,fodder_b}, count 1 < 3 → EffectZoneChoice, devourer not auto-sacrificed. Fails if the guard is removed entirely (devourer would be injected and auto-sacrificed). Confirmed discriminating against the opposite regression.

Together the pair pins both directions of the guard — exactly the fail-first coverage the deleted test lacked.

Findings

[LOW] No integration test now drives a DealtDamage + Sacrifice{Typed} replacement through the real cast→damage→SBA pipeline. Evidence: after this PR grep -rln "ReplacementEvent::DealtDamage" crates/engine/tests/ returns nothing; the new Test 1 calls apply_post_replacement_effect directly rather than via a GameAction. Why it matters: per the repo's runtime-tests-must-drive-pipeline principle, the new tests are behavioral but lower-altitude — they prove the guard produces the right result given the call, not that the cast/damage pipeline reaches it with the right (event, object_id). Net it's a wash-to-improvement (the deleted pipeline test was non-discriminating, and these are discriminating), so this is informational, not blocking. Devour itself still has real-pipeline coverage in devour_co_entry_regression and devour_intellect_treasure_rider. Optional follow-up: keep one GameScenario/GameRunner Dralnu smoke test alongside the new unit pair to retain end-to-end reach.

[LOW] Outfitted Jouster (DamageDone + Sacrifice{Typed(Equipment)}) is now uncited in tests. Evidence: the deleted file's doc-comment named it as a guard-relevant card, but only in prose — it was never actually built or asserted, so no executable coverage is lost. Worth a tracked follow-up only if you want explicit DamageDone coverage.

Idiomatic / seam checks (pass)

• New tests reuse existing in-module helpers/imports (create_object, AbilityDefinition::new(AbilityKind::Spell, Effect::…), QuantityExpr::Fixed { value }), matching the sibling tests at lines 1338/1694/1750. Signatures and constructors verified.
• CR annotations: no new CR numbers introduced (test-only). The guard's existing Moved-gating doc-comment is unchanged and accurate.
• main.rs mod-list deletion correctly drops mod dralnu_dealt_damage_sacrifice; in lockstep with the file removal.

Note: I could not get PR-specific Tilt test-engine/clippy signal — the working tree is on main with other agents' uncommitted changes, so I reviewed via git show pr2795-review rather than checking out. Compile-correctness verified by tracing every import/constructor/signature; assertions verified against the live sacrifice::resolve and apply_post_replacement_effect code paths.

[natefinch]

/review-impl — PR #2795 (Devour post-replacement regression tests)

Reviewed the API diff against PR head 3bbc7aa4 in an isolated worktree.

Seam / scope: The only engine_replacement.rs change is inside mod tests (single hunk @@ -1847,6 +1847,120 @@). The production guard is_as_enters_sacrifice_scope_replacement (gated on event == Some(ReplacementEvent::Moved)) and apply_post_replacement_effect's injection-suppression logic are unchanged. Tests live in the same module as the guard and drive it through apply_post_replacement_effect — correct seam, accurate test(...) label, no production behavior change.

Discrimination (both directions pinned):

• issue_575_dealt_damage_sacrifice_injects_source_target: event=DealtDamage → guard false → [Object(dralnu)] injected → dralnu→Graveyard. Fails if the guard re-broadens (drop the Moved check), which would suppress injection → EffectZoneChoice and leave dralnu alive.
• issue_575_moved_sacrifice_typed_opens_chooser_not_source_injection: event=Moved → guard true → empty targets → chooser pool {devourer, fodder_a, fodder_b}, count 1 < pool → EffectZoneChoice, devourer not auto-sacrificed. Fails if the guard is removed entirely (devourer would be injected + auto-sacrificed).

The deleted dralnu_dealt_damage_sacrifice.rs self-documented as "a SHAPE + SMOKE regression test, not a fail-first discriminating test" — its only behavioral assertion (Dralnu dies to lethal-damage SBA) fires regardless of the guard. So the -233 deletion loses no discriminating coverage; the new pair is a strict upgrade. CI is fully green (Rust test + coverage-gate), so the deletion doesn't trip the coverage regression check and the new tests compile/pass. The main.rs mod-list deletion is in lockstep with the file removal.

Findings (both informational, non-blocking — folding in prior reviewer's points):

[LOW] No integration test now drives a DealtDamage + Sacrifice{Typed} replacement through the real cast→damage→SBA pipeline. Evidence: grep -rln "ReplacementEvent::DealtDamage" crates/engine/tests/ returns nothing post-PR; the new Test 1 calls apply_post_replacement_effect directly rather than via a GameAction. Why it matters: the new tests prove the guard yields the right result given the call, not that the cast/damage pipeline reaches it with the right (event, object_id). Net it's wash-to-improvement (deleted pipeline test was non-discriminating); Devour retains real-pipeline coverage in devour_co_entry_regression / devour_intellect_treasure_rider. Suggested fix (optional): keep one GameScenario Dralnu smoke test alongside the new unit pair.

[LOW] Outfitted Jouster (DamageDone + Sacrifice{Typed(Equipment)}) is no longer cited in tests. Evidence: it appeared only in the deleted file's prose, never as executable assertions, so no runnable coverage is lost. Suggested fix (optional): add explicit DamageDone coverage if desired.

VERDICT: approve

[matthewevans]

Maintainer sign-off: strict test-quality upgrade — replaces a self-documented non-discriminating smoke test with a discriminating pair pinning both directions of the Moved-gated sacrifice-scope guard. Two independent review-impl approvals; only optional LOW follow-ups. CI green.