Skip to content
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

Refactor the backstab unit test #6473

Merged
merged 1 commit into from Feb 2, 2022

Conversation

stevecotton
Copy link
Contributor

Based on part of Pentarctagon's #6308, and also improving
the encapsulation of the test to take a boolean value for whether
the ability should be active instead of the expected damage total.

@stevecotton stevecotton added Docs Documentation issues, both in-game and otherwise (e.g. manual and manpages). Unit Tests Issues involving Wesnoth's unit test suite. labels Jan 30, 2022
@stevecotton stevecotton self-assigned this Jan 30, 2022
@Wedge009
Copy link
Member

I'm not very familiar with WML tests but as a general rule I try to write self-descriptive names for the tests in preference to overly-long explanatory comments as the latter can become out of date as code/tests are updated. As I recall, back-stab doubles damage for the attacker when an attacker's ally is directly opposite the defender. But it looks like the test names are pre-existing and I assume they haven't been changed to avoid breaking tests.

It makes sense to me to have the test focus on the activation of the back-stab damage bonus vs checking arbitrary damage values in the parameters as the latter seems more prone to errors.

@stevecotton
Copy link
Contributor Author

It would be easy to change the tests' names too, please provide better suggestions.
The name just has to match in the test and in the wml_test_schedule file.

@Wedge009
Copy link
Member

Wedge009 commented Jan 30, 2022

I don't know what the convention is, but if I understood the tests correctly I was thinking along the lines of:

  • TEST_BACKSTAB "backstab_should_activate_with_attacker_ally_behind_defender"
  • TEST_BACKSTAB "backstab_should_not_activate_without_attacker_ally_behind_defender"
  • TEST_BACKSTAB "backstab_should_not_activate_with_disabled_attacker_ally_behind_defender"
  • TEST_BACKSTAB "backstab_should_not_activate_with_defender_ally_behind_defender"

A bit long, I know, but at least it should be clear to a newcomer what the tests are checking for.

@stevecotton
Copy link
Contributor Author

A bit long, I know, but at least it should be clear to a newcomer what the tests are checking for.

A bit shorter (haven't pushed it yet, still pondering):

  • backstab_active_with_accomplice_behind_bob
  • backstab_inactive_with_triangular_formation
  • backstab_inactive_with_statue_behind_bob
  • backstab_inactive_with_bobs_ally_behind_bob

@Wedge009
Copy link
Member

Wedge009 commented Feb 1, 2022

As long as people reading can quickly and easily understand the intent of the test solely by looking at the test name. I find that's generally easier to accomplish when being a bit more verbose but whatever works.

Based on Pentarctagon's documentation addition, and also improving
the encapsulation of the test to take a boolean value for whether
the ability should be active instead of the expected damage total.
@stevecotton
Copy link
Contributor Author

Are the ones that I posted clear to you, or not?

@Wedge009
Copy link
Member

Wedge009 commented Feb 1, 2022

It should be fine - definitely an improvement over the original anyway.

@stevecotton
Copy link
Contributor Author

@Pentarctagon, is this OK for you?

@stevecotton stevecotton merged commit 556c66e into wesnoth:master Feb 2, 2022
@stevecotton stevecotton deleted the refactor_backstab_test branch February 2, 2022 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs Documentation issues, both in-game and otherwise (e.g. manual and manpages). Unit Tests Issues involving Wesnoth's unit test suite.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants