Spring Kick banish management

[ckb11]

No description provided.

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (07b8a84) 37.16% compared to head (612b3e0) 37.15%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2201      +/-   ##
============================================
- Coverage     37.16%   37.15%   -0.01%     
- Complexity    19586    19587       +1     
============================================
  Files          1106     1106              
  Lines        168884   168887       +3     
  Branches      35680    35681       +1     
============================================
- Hits          62765    62756       -9     
- Misses        96130    96146      +16     
+ Partials       9989     9985       -4     

Files	Coverage Δ
...et/sourceforge/kolmafia/session/BanishManager.java	95.62% <100.00%> (+0.01%)	⬆️
...net/sourceforge/kolmafia/request/FightRequest.java	35.46% <50.00%> (+<0.01%)	⬆️

... and 5 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07b8a84...612b3e0. Read the comment docs.

[jaadams5]

Like you, I agree that this looks like other banishers and should work.

Do you have any sense that this code works as expected (from local build and run)? Would you consider expanding your skill set and trying to write a test? That might be as simple as capturing HTML and doing a copy/past from some other test.

[ckb11]

I can give a test a try, but it might take me a long time. I know very little about how any of this works in detail.
I'll look through other commits and see if I can find something to copy maybe.

[jaadams5]

Sounds good. If you give up let me know and I will either try and help or just approve. But since I don't have the item you might have to be the one that captures fight html with a banish.

[ckb11]

I was hoping to find a similar test from a past banish commit, but it looks like all the recent additions (monkey slap. crimbo23 stuff, asol, Roar like a Lion) did not include any tests.
I can capture and share html if that helps.

[jaadams5]

The maximizer fails to equip is sucking up all of my cycles. Maybe someone else will chime in and assist with a test? Otherwise I'll get to it eventually or just submit. Thanks.

[jaadams5]

I look casually at FightRequestTest. commerceGhostIncrementsByOneOnFight seems to parse a combat and then check for a preference to change. You could capture HTML from a fight with a spring banish, parse it as part of the test and just verify that the preference changed to include the banished monster. I would expect that to work as a test although I am not in a position to try it for you.

[midgleyc]

look at e.g. FightRequestTest.canDetectEagleScreech. That's phyla instead of monster, but the idea is the same.

[ckb11]

I looked at that FightRequestTest... most of that went way over my head. I could spend some time trying to hack together a test, but I fear there is an equal change of getting a good test as there is at just messing up the code and breaking it.
It would probably be easier to try and figure out how to compile Mafia and run some test adventures, but I don't know how to do that either or if it counts.

[midgleyc]

no problem, if you're up for it I can make a test and add it to your branch myself, and we can join a video call over Discord so you can see what I do live, if you'd like.

But the long of it is:

• enter a fight with spring boots equipped
• command debug on
• use the banish
• command debug off
• you now have some HTML in the Mafia root directory. You're going to put this in test/root/request/ and make a test looking something like

  @Test
  public void canDetectSpringBoots() {
    var cleanups =
        new Cleanups(
            withFight(),
            withProperty("banishedMonsters"));

    try (cleanups) {
      parseCombatData(
          "request/test_fight_spring_boots_banish.html", "fight.php?action=skill&whichskill=7501");

      assertThat("banishedMonsters", hasStringValue(startsWith("fluffy bunny:Spring Kick:")));
    }
  }

at the bottom of FightRequestTest.java.

As to "how do I compile Mafia myself", I will assume you've already got the source downloaded and the jdk installed. From there, go to the root directory of the source, and run ./gradlew :shadowJar. This will make a jar file in dist/. Copy it to wherever or just run in straight from there.

[midgleyc]

Also, it probably /should/ be turn-free, because it doesn't end the combat. I think this is the first banish that's true for?

[ckb11]

@midgleyc That helps. I will capture some fight html and use your code snippet. I also need to install a JDK (I have JRE only now) to try that compile.

Do we need to check that spring shoes are equipped in the 'Cleanups' section?

When is this check done? Is this part of the PR check, or does that test happen at compile?

I am happy to jump on a call and learn more about this if you have time. I am on the west coast of the US, and you look like you are in the UK, so timing might be tricky.

As for "isTurnFree" I don't know what it should be as it works different than all the other banishes that end combat. I would expect this to be false based on the fact that it cannot act as a free runaway or delay burn, which was my line of thinking when I made this update.

[midgleyc]

Do we need to check that spring shoes are equipped in the 'Cleanups' section?

Not for this: the code runs when the skill is used, and no code requires that the shoes be equipped. Sometimes I like to add this to cleanups for in-game consistency, but it's not essential.

When is this check done? Is this part of the PR check, or does that test happen at compile?

Not sure which check you mean, but the tests run on GitHub actions when you do a PR (or run them manually)

I am on the west coast of the US, and you look like you are in the UK, so timing might be tricky.

I am indeed in the UK -- so yeah, 8 hours out.

isTurnFree is used in two places -- one when calculating the duration (which doesn't matter because it's an all-day banish) and once when calculating the turn the monster was banished on. The latter is why I think this should be "turn-free" -- because you're still in the combat when the banish happens (or so I assume anyway -- I guess the way it could work is if you banish then waffle twice you can still encounter the monster you just banished).

[ckb11]

Hey look! I added a test... which failed... I'm not sure if this is because the code is wrong or the test is wrong.

Edit: I tested a compiled version and got this:
Preference banishedMonsters changed from to fluffy bunny:Spring Kick:5390

[midgleyc]

Yeah, the test was wrong: I forgot that banishedMonsters needs special handling, because it reads from a static variable to populate it.

I've fixed it.

[midgleyc]

Thanks!