-
Notifications
You must be signed in to change notification settings - Fork 839
Fix Brutal Cathar trigger timing and add regression canaries(#9484) #9669
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| game.getPhaseHandler().devModeSet(PhaseType.MAIN1, catharController); | ||
| SpellAbility catharSa = brutalCathar.getFirstSpellAbility(); | ||
| AssertJUnit.assertNotNull(catharSa); | ||
| catharSa.setTargetCard(specialist); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmn thanks, but you picked a pretty difficult problem for your first PR...
anyway I thought I already fixed the trigger handling (and only left the issue open for the replacement effect part):
while I see this test fails without the engine change I suspect that's only because you're not providing the LastStateBattlefield correctly when destroying 🤷♂️
also here a target is set for the permanent spell which makes no sense (it's possible the AI targets the trigger by itself later though)
this kind of leads me to believe this is generated by an AI once again fooling itself? 👎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback, absolutely my mistake.
I think that the tests should be valid now. At the very least, I think I've fixed the two issues that you've pointed out. Dropping the guard I added causes the following tests to fail:
testBanisherPriestReturnDoesNotSeePastOpponentDeath
testBrutalCatharDoesNotTriggerReturnedSlaughterSpecialistFromItsDeath
testBrutalCatharReturnDoesNotSeeSimultaneousOtherCreatureDeaths
If I’m still missing something and the tests are still wrong, that’s on me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the most terrible thought. What if I misread/misremembered the card?
In a case of tragic serendipity, the test was right for the wrong reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Started with the test, got the test wrong. The issue was always solved. Sorry for wasting time.

Fixes ChangesZone trigger timing for battlefield-origin events so newly-entered objects don’t retroactively observe older leaves/dies events (Brutal Cathar / Slaughter Specialist case).
Also adds focused regression canaries in GameSimulationTest for:
No functional changes outside trigger timing guard + tests.