Skip to content

Fix class name collision in CardBuilder DSL causing flaky specs - #513

Draft
radar with Copilot wants to merge 3 commits into
masterfrom
copilot/fix-flaky-spec-cards
Draft

Fix class name collision in CardBuilder DSL causing flaky specs#513
radar with Copilot wants to merge 3 commits into
masterfrom
copilot/fix-flaky-spec-cards

Conversation

Copilot AI commented Jan 6, 2026

Copy link
Copy Markdown
Contributor

Nested classes defined within CardBuilder DSL blocks were colliding at the Magic::Cards module level, causing test flakiness where one card's logic would override another's.

Root Cause

When using Creature("Name") do ... end, the block is evaluated via Class.new(base_class, &block). Ruby constant lookup rules place nested class definitions at the enclosing module level (Magic::Cards) rather than inside the anonymous class:

# Both define Magic::Cards::PowerAndToughnessModification
RighteousValkyrie = Creature("Righteous Valkyrie") do
  class PowerAndToughnessModification < Abilities::Static::PowerAndToughnessModification
    conditions do
      controller.life >= controller.starting_life + 7
    end
  end
end

RousingRead = Aura("Rousing Read") do
  class PowerAndToughnessModification < Abilities::Static::PowerAndToughnessModification
    # No conditions - should always apply
    # But inherits RighteousValkyrie's condition due to collision
  end
end

Load order determined which card's class definition "won", causing non-deterministic test failures.

Solution

Following the established pattern from cards like CanopyTactician, ElvishArchdruid, and GloriousAnthem, the fix separates the CardBuilder DSL from class definitions:

  1. Use CardBuilder DSL for basic card setup (cost, types, power/toughness, etc.)
  2. Reopen the class with explicit inheritance (e.g., class RighteousValkyrie < Creature)
  3. Define nested classes inside the reopened class

This ensures nested classes are properly scoped to Magic::Cards::RighteousValkyrie::PowerAndToughnessModification instead of Magic::Cards::PowerAndToughnessModification, preventing collisions while maintaining consistent naming conventions across the codebase.

Example:

RighteousValkyrie = Creature("Righteous Valkyrie") do
  cost generic: 2, white: 1
  # ... basic setup
end

class RighteousValkyrie < Creature
  class PowerAndToughnessModification < Abilities::Static::PowerAndToughnessModification
    # Properly scoped to Magic::Cards::RighteousValkyrie::PowerAndToughnessModification
  end
end
Original prompt

Work on TODO: Fix this flaky spec, it fails when all the tests are run. Why is that? (from spec/cards/righteous_valkyrie_spec.rb)

Created from VS Code via the GitHub Pull Request extension.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

…classes

The righteous_valkyrie_spec.rb test was failing when run with the full test suite due to class name collision. Both RighteousValkyrie and RousingRead defined nested classes named `PowerAndToughnessModification` within the CardBuilder DSL block. Since the block is evaluated at the Magic::Cards module level, these classes were overwriting each other, causing the second card loaded to use the conditions from the first card's class.

Fixed by renaming:
- PowerAndToughnessModification -> RighteousValkyriePowerAndToughnessModification in righteous_valkyrie.rb
- PowerAndToughnessModification -> RousingReadPowerAndToughnessModification in rousing_read.rb

Co-authored-by: radar <2687+radar@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix flaky spec in righteous valkyrie tests Fix class name collision in CardBuilder DSL causing flaky specs Jan 6, 2026
Copilot AI requested a review from radar January 6, 2026 08:47
@radar

radar commented Jan 6, 2026

Copy link
Copy Markdown
Owner

No, reopen the classes after the DSL part and keep the class names consistent across both.

@radar radar left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

No, reopen the classes after the DSL part and keep the class names consistent across both.

Following the pattern used in other cards like CanopyTactician and ElvishArchdruid:
- Use CardBuilder DSL for basic card setup
- Reopen the class with explicit inheritance (class CardName < BaseClass)
- Define nested classes inside the reopened class

This ensures nested classes are scoped to Magic::Cards::CardName::PowerAndToughnessModification instead of Magic::Cards::PowerAndToughnessModification, preventing collisions between cards.

Co-authored-by: radar <2687+radar@users.noreply.github.com>

Copilot AI commented Jan 6, 2026

Copy link
Copy Markdown
Contributor Author

No, reopen the classes after the DSL part and keep the class names consistent across both.

...

Done in bee757e. Both cards now reopen their classes after the DSL part, following the pattern used in CanopyTactician, ElvishArchdruid, and GloriousAnthem. The nested classes maintain their original names (PowerAndToughnessModification) and are properly scoped to Magic::Cards::CardName::PowerAndToughnessModification.

No, reopen the classes after the DSL part and keep the class names consistent across both.

Done in bee757e. Both cards now reopen their classes after the DSL part, following the pattern used in CanopyTactician, ElvishArchdruid, and GloriousAnthem. The nested classes maintain their original names (PowerAndToughnessModification) and are properly scoped to Magic::Cards::CardName::PowerAndToughnessModification.

Copilot AI requested a review from radar January 6, 2026 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants