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

Battle Test Runner Failsafes/Improvements #5525

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghoulslash
Copy link
Collaborator

  1. Failsafe check on assigned move PP. If a mon has a moveset with a single move, PP in move slot 2+ will be 0, so some tests will fail with a hard-to-track issue
  2. TryMessage has been improved to be more similar to a StringCopy function/simpler to read. It also will skip over ext ctrl codes for colors. The loop will now basically skip over any space, \p, \n etc code as well as colors and just compare relevant characters.

@ghoulslash ghoulslash changed the title some battle test runner failsafe checks, simplify TryMessage Battle Test Runner Failsafes/Improvements Oct 15, 2024
@AlexOn1ine
Copy link
Collaborator

PR doesn't build

@ghoulslash
Copy link
Collaborator Author

ghoulslash commented Oct 16, 2024

PR doesn't build

looks like it was a weird CI issue. builds now

@hedara90 hedara90 added the category: battle-tests Related to the automated test environment label Oct 16, 2024
Comment on lines +1142 to +1146
#define IS_SKIPOVER_CHAR(char) (char == CHAR_SPACE \
|| char == CHAR_PROMPT_SCROLL \
|| char == CHAR_PROMPT_CLEAR \
|| char == CHAR_NEWLINE)

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should prefer static inline functions to macros wherever possible.

Suggested change
#define IS_SKIPOVER_CHAR(char) (char == CHAR_SPACE \
|| char == CHAR_PROMPT_SCROLL \
|| char == CHAR_PROMPT_CLEAR \
|| char == CHAR_NEWLINE)
static inline bool32 IsSkipoverChar(u32 char)
{
return char == CHAR_SPACE
|| char == CHAR_PROMPT_SCROLL
|| char == CHAR_PROMPT_CLEAR
|| char == CHAR_NEWLINE;
}

Comment on lines +1162 to +1169
if (IS_SKIPOVER_CHAR(event->pattern[k])) {
k++;
continue;
}
if (event->pattern[k] == EOS)
{
// Consume any trailing '\p'.
if (string[j] == CHAR_PROMPT_CLEAR)
j++;
if (IS_SKIPOVER_CHAR(string[j])) {
j++;
continue;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this essentially ignore all whitespace inside the pattern and output? e.g. "Hello World" would match "HelloWorld" and vice-versa?

The intent of the original code was that a space in the pattern would match any whitespace in the actual output, so I think this change should say something like "try to match 1 or more skipover characters in event->pattern, and if so match 1 or more skipover characters in string"?

Example incorrectly-passing test (no engine code modified):

WILD_BATTLE_TEST("Pokemon gain exp after catching a Pokemon")
{
    u8 level = 0;

    PARAMETRIZE { level = 50; }
    PARAMETRIZE { level = MAX_LEVEL; }

    GIVEN {
        PLAYER(SPECIES_WOBBUFFET) { Level(level); }
        OPPONENT(SPECIES_CATERPIE) { HP(1); }
    } WHEN {
        TURN { USE_ITEM(player, ITEM_ULTRA_BALL); }
    } SCENE {
-       MESSAGE("You used Ultra Ball!");
+       MESSAGE("YouusedUltraBall!");
        ANIMATION(ANIM_TYPE_SPECIAL, B_ANIM_BALL_THROW, player);
        if (level != MAX_LEVEL) {
            EXPERIENCE_BAR(player);
        }
    }
}

Example incorrectly passing code (no tests modified):

-static const u8 sText_PlayerUsedItem[] = _("You used\n{B_LAST_ITEM}!");
+static const u8 sText_PlayerUsedItem[] = _("Youused{B_LAST_ITEM}!");

}
if (string[j] != event->pattern[k])
{
break;
}
else if (string[j] == EOS)
if (string[j] == EOS || event->pattern[k] == EOS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think event->pattern[k] == EOS cannot occur, because the previous line is if (string[j] != event->pattern[k]) break;, so if string[k] != EOS && event->pattern[k] == EOS we should have already breaked?

If it could occur, I think this code would be wrong because we shouldn't exit this code with a success if we get to the end of event->pattern without also getting to the end of string.

SetMonData(DATA.currentMon, MON_DATA_MOVE1 + i, &data);
data = 20;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this choice arbitrary? If so I'd probably use 1 or 0x7F (max) to signal the arbitrariness more directly.

@hedara90
Copy link
Collaborator

From testing it with the cases I could remember that this would fix it works as advertised.
I don't think this is supposed to fix the various forced switch peculiarities, but I can't find any regressions with those at least.
This test fails without this PR and passes with it.

SINGLE_BATTLE_TEST("Species with only 1 learnable move can use more moves in tests")
{
    GIVEN {
        PLAYER(SPECIES_BELDUM);
        OPPONENT(SPECIES_WOBBUFFET);
    } WHEN {
        TURN { MOVE(player, MOVE_TACKLE); }
        TURN { MOVE(player, MOVE_GROWL); }
        TURN { MOVE(player, MOVE_SURF); }
        TURN { MOVE(player, MOVE_STRENGTH); }
    } SCENE {
        MESSAGE("Beldum used Tackle!");
        MESSAGE("Beldum used Growl!");
        MESSAGE("Beldum used Surf!");
        MESSAGE("Beldum used Strength!");
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: battle-tests Related to the automated test environment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants