Skip to content

test(engine): Obuun land animation regression - #3152

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
kiannidev:fix/issue-2398-obuun-land-animation
Jun 13, 2026
Merged

test(engine): Obuun land animation regression#3152
matthewevans merged 2 commits into
phase-rs:mainfrom
kiannidev:fix/issue-2398-obuun-land-animation

Conversation

@kiannidev

Copy link
Copy Markdown
Contributor

Summary

  • Add integration regression test for Obuun, Mul Daya Ancestor's begin-combat land animation
  • Verifies the chosen land becomes X/X (Obuun's power) with trample and haste and stays on the battlefield

Test plan

  • cargo test -p engine --test integration issue_2398
  • cargo clippy -p engine -- -D warnings

Fixes #2398

Made with Cursor

Guard Obuun's begin-combat trigger so animated lands become X/X from the
source's power instead of dying as 0/0 creatures.

Fixes phase-rs#2398

Co-authored-by: Cursor <cursoragent@cursor.com>
@kiannidev
kiannidev requested a review from matthewevans as a code owner June 13, 2026 15:52

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds an integration test for Issue #2398 to verify that Obuun, Mul Daya Ancestor's begin-combat animation correctly sets the target land's power and toughness to Obuun's power. The review feedback suggests avoiding conditional actions in the integration test by asserting the expected game state directly to prevent masking potential bugs.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +33 to +42
if matches!(
runner.state().waiting_for,
engine::types::game_state::WaitingFor::TriggerTargetSelection { .. }
) {
runner
.act(GameAction::SelectTargets {
targets: vec![TargetRef::Object(forest)],
})
.expect("select land target for Obuun");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

[MEDIUM] Avoid conditional actions in integration tests. Using if matches!(...) can mask bugs where the trigger fails to fire entirely, leading to confusing failure messages later in the test. Asserting the expected state directly ensures the test fails immediately at the point of failure.

    assert!(matches!(
        runner.state().waiting_for,
        engine::types::game_state::WaitingFor::TriggerTargetSelection { .. }
    ));
    runner
        .act(GameAction::SelectTargets {
            targets: vec![TargetRef::Object(forest)],
        })
        .expect("select land target for Obuun");

@matthewevans matthewevans added the test Add tests label Jun 13, 2026
@matthewevans matthewevans self-assigned this Jun 13, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. This is test-only and covers the right production path: the regression advances to beginning of combat, responds through target selection, then checks the animated land survives as Obuun-power X/X with trample and haste. No engine logic is duplicated or bypassed. The focused issue_2398 integration test passed locally and GitHub CI is green.

For future PRs, please sync with origin/main before opening/updating and use the /engine-implementer skill for non-trivial parser or engine work so the plan, implementation, and review all stay aligned with the project architecture.

@matthewevans
matthewevans added this pull request to the merge queue Jun 13, 2026
@matthewevans matthewevans removed their assignment Jun 13, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Jun 13, 2026
@matthewevans matthewevans self-assigned this Jun 13, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed current head e8e88b9fb1a3 after the contributor merge-from-main commit. The PR delta remains test-only: the Obuun regression drives the production beginning-of-combat trigger, selects the land target, resolves the stack, and asserts the animated land stays on battlefield as 3/3 with trample/haste. That would fail if the source-power animation fix regressed back to 0/0.

@matthewevans
matthewevans enabled auto-merge June 13, 2026 16:42
@matthewevans matthewevans removed their assignment Jun 13, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jun 13, 2026
Merged via the queue into phase-rs:main with commit 4284775 Jun 13, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test Add tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Obuun, Mul Daya Ancestor: combat trigger animates land as 0/0 instead of X/X (Obuun's power)

2 participants