Tests: fix choice on notTarget wrongly not checking Filter #12350
Conversation
| @@ -4143,7 +4143,7 @@ public boolean choose(Outcome outcome, Cards cards, TargetCard target, Ability s | |||
| continue; | |||
| } | |||
| if (hasObjectTargetNameOrAlias(card, targetName)) { | |||
| if (target.isNotTarget() || target.canTarget(card.getId(), source, game)) { | |||
| if (target.canTarget(getId(), card.getId(), source, game)) { | |||
There was a problem hiding this comment.
There are three places with such code, not one.
I think it can be used to setup invalid targets for some reasons? If true then that's wrong usage and must be fixed in broken tests.
There was a problem hiding this comment.
It's looks like a complex "problem" -- isNotTarget || canTarget used all around xmage:
There was a problem hiding this comment.
I don't think there is an issue for real gameplay (we have choices using target code with the notTarget set to true, and that's important to be allowed to sacrifice a shroud creature for instance).
The thing with the test is that we were wrongly accepting some invalid choices. I'm running all the tests with the others 2 instances in TestPlayer without the call to isNotTarget(). I do believe this was wrong usage there and skipped filter validation.
There was a problem hiding this comment.
Game engine code is fine -- it's used for checking replacement effects (only real target 🙃 can be restricted by effects):
I think TestPlayer must search targets same way. Example:
There was a problem hiding this comment.
That's protection effects can be broken in current test framework (it allow to add restricted target):
Fix an issue with
setChoiceof non-target choices not validating that the choice matched the target'sFilter(also was not using the one with a 4-argumentFilter::match).Of note, we don't have a way to check that is choice is invalid through the checkXXX methods. It would be nice to add at some point. The try/catch way is the current workaround.