[1.16] Unit tests for [drains], [poison] and [slows], including apply_to=opponent

[stevecotton]

Here apply_to=opponent means that the weapon special gives the opponent the
ability, the unit that should get poisoned or slowed is the unit that has the
weapon special.

There's a known bug in 1.16, that apply_to=opponent check the wrong unit to
see it it's unpoisonable, undrainable etc. It also checks the wrong unit to
see if it's already poisoned or slowed, so a battle between two units that both
have reverse-poison results in at most one being poisoned.

As 1.16 has already been released, to avoid OOS the test is checking that the
current behavior's known bug is preserved. For the 1.17 branch the four tests
labelled preserving known bug will be changed to test the reverse.

Most of the credit for this is Newfrenchy's, as he's already written a fix
and a WML based test. This commit uses a Lua test instead to test more
combinations of statuses.

This adds a COMMON_KEEP_A_B_UNIT_TEST macro, which is a counterpart to the
GENERIC_UNIT_TEST macro that starts the leaders next to each other, ready
to attack. The A_B is because I'm planning a multiple-side variant too.

There's no test for [petrify], as simulate_combat doesn't provide a stat for it.

This tests only 3 of the 6 abilities whose behavior will change in 1.17's
equivalent of 1.16's 7b39b65. That's sufficient to prevent any accidental
copy of the 1.17 fix to 1.16, and my thoughts on testing the others are:

• [firststrike]'s test is in 7b39b65. It crashed, so is fixed in 1.16.
• [drains], [poison] and [slow] are tested here.
• [petrify] ends combat, it's also not exposed in simulate_combat's stats.
• [plague] triggers after combat ends.

[CelticMinstrel]

Why randomly indenting stuff? (Or was this caused by wmlindent? In which case the question would be, why isn't the change already in master and why didn't the CI fail because it's not?)

[stevecotton]

It was caused by wmlindent. There's a CI exclusion in 599504c, for which the GNA bug became Github's #1397.

[stevecotton]

Revised plan - I won't change wml_unit_test_macros.cfg in this PR.

Instead of adding the COMMON_KEEP_A_B_UNIT_TEST to wml_unit_test_macros.cfg, I want to put it in a separate file. There will then be a separate PR for master (possibly backported too) that moves GENERIC_UNIT_TEST to its own file and extracts COMMON_KEEP_A_B_C_D_UNIT_TEST from the 4 side setup in firststrike_and_laststrike.

The main reason for having these in separate files is Git's fuzzy-patch matching when merging and rebasing. Having large blocks of identical text in wml_unit_test_macros.cfg can mean the wrong section gets patched.

[stevecotton]

If there's no more coments then I'll use similar code for drains and slows. The test for firststrike / laststrike ended up looking different.

[stevecotton]

It now tests slows, poison and drains. The slows and poison tests are just a find-and-replace of each other, but drains is a separate test.

I've called it reflexive_drains but it's also the only unit test for the drains ability.

[CelticMinstrel]

There's no test for [petrify], as simulate_combat doesn't provide a stat for it.

Obviously not for 1.16, but this could (and probably should) be added.

[stevecotton]

There's no test for [petrify], as simulate_combat doesn't provide a stat for it.

Obviously not for 1.16, but this could (and probably should) be added.

We should, but even for 1.17 I think that's going to be a separate feature. At the moment, attack prediction for petrify works by running a combat simulation with damage set to the potentially petrified unit's max hp, and includes a FIXME comment about drain.

wesnoth/src/attack_prediction.cpp

Lines 2076 to 2085 in c1d7e65

	// To simulate stoning, we set to amount which kills, and re-adjust after.
	/** @todo FIXME: This doesn't work for rolling calculations, just first battle.
	It also does not work if combined with (percentage) drain. */
	if(stats.petrifies) {
	a_damage = a_slow_damage = opp_stats.max_hp;
	}
	if(opp_stats.petrifies) {
	b_damage = b_slow_damage = stats.max_hp;
	}

[stevecotton]

@CelticMinstrel okay to merge?

[stevecotton]

Going to rebase and force-push for a trivial fixup to the commit message - there's now seven asserts for preserving the known status, not four. (Noticed now because I'm rebasing the 1.17 version).

Edit: seven asserts but only five lines. The single line in reflexive_drains tests once for each expected hp value.

[CelticMinstrel]

okay to merge?

I'm not quite sure why you're asking me, but yeah, it seems fine.