Roll rework / refactor Fixes #5023

Changelog.txt

@@ -1,3 +1,13 @@
+Version 2.2.020
+    (Codebase) Reworked Roll implementation (See notes)
+    (Codebase) Added unit test coverage for Roll
+    Fixed a bug where Roll was modifying damage unnecessarily
+
+    NOTES:
+    The code for Roll was a bit of a mess, I've rewritten a good chunk of it and added some unit test coverage.
+    I will likely put out another update for Acrobatics in general, as the code for Acrobatics is whack.
+    This would be a good time to suggest changes to Acrobatics on discord.
+
 Version 2.2.019
     Optimized Alchemy code (thanks MrPowerGamerBR)
     Fixed an exception that could occur when shooting entities through worlds (thanks Wariorrrr)

pom.xml

@@ -2,7 +2,7 @@
     <modelVersion>4.0.0</modelVersion>
     <groupId>com.gmail.nossr50.mcMMO</groupId>
     <artifactId>mcMMO</artifactId>
-    <version>2.2.019</version>
+    <version>2.2.020-SNAPSHOT</version>
     <name>mcMMO</name>
     <url>https://github.com/mcMMO-Dev/mcMMO</url>
     <scm>

src/main/java/com/gmail/nossr50/datatypes/skills/subskills/acrobatics/Roll.java

@@ -11,13 +11,9 @@ import com.gmail.nossr50.mcMMO;
 import com.gmail.nossr50.util.EventUtils;
 import com.gmail.nossr50.util.ItemUtils;
 import com.gmail.nossr50.util.Permissions;
-import com.gmail.nossr50.util.player.NotificationManager;
 import com.gmail.nossr50.util.random.Probability;
 import com.gmail.nossr50.util.random.ProbabilityUtil;
-import com.gmail.nossr50.util.skills.PerksUtils;
 import com.gmail.nossr50.util.skills.RankUtils;
-import com.gmail.nossr50.util.skills.SkillUtils;
-import com.gmail.nossr50.util.sounds.SoundManager;
 import com.gmail.nossr50.util.sounds.SoundType;
 import net.kyori.adventure.text.Component;
 import net.kyori.adventure.text.TextComponent;
@@ -34,9 +30,17 @@ import org.jetbrains.annotations.VisibleForTesting;
 
 import java.util.Locale;
 
+import static com.gmail.nossr50.util.player.NotificationManager.sendPlayerInformation;
+import static com.gmail.nossr50.util.random.ProbabilityUtil.getSubSkillProbability;
+import static com.gmail.nossr50.util.skills.SkillUtils.applyXpGain;
+import static com.gmail.nossr50.util.sounds.SoundManager.sendCategorizedSound;
+
 public class Roll extends AcrobaticsSubSkill {
 
 
+    public static final String GRACEFUL_ROLL_ACTIVATED_LOCALE_STR_KEY = "Acrobatics.Ability.Proc";
+    public static final String ROLL_ACTIVATED_LOCALE_KEY = "Acrobatics.Roll.Text";
+
     public Roll() {
         super("Roll", EventPriority.HIGHEST, SubSkillType.ACROBATICS_ROLL);
     }
@@ -49,23 +53,14 @@ public class Roll extends AcrobaticsSubSkill {
      */
     @Override
     public boolean doInteraction(Event event, mcMMO plugin) {
-        //TODO: Go through and API this up
-
-        /*
-         * Roll is a SubSkill which allows players to negate fall damage from certain heights with sufficient Acrobatics skill and luck
-         * Roll is activated when a player takes damage from a fall
-         * If a player holds shift, they double their odds at a successful roll and upon success are told they did a graceful roll.
-         */
-
-        //Casting
-        EntityDamageEvent entityDamageEvent = (EntityDamageEvent) event;
+        final EntityDamageEvent entityDamageEvent = (EntityDamageEvent) event;
 
         //Make sure a real player was damaged in this event
         if (!EventUtils.isRealPlayerDamaged(entityDamageEvent))
             return false;
 
-        if (entityDamageEvent.getCause() == EntityDamageEvent.DamageCause.FALL) {//Grab the player
-            McMMOPlayer mmoPlayer = EventUtils.getMcMMOPlayer(entityDamageEvent.getEntity());
+        if (entityDamageEvent.getCause() == EntityDamageEvent.DamageCause.FALL) {
+            final McMMOPlayer mmoPlayer = EventUtils.getMcMMOPlayer(entityDamageEvent.getEntity());
 
             if (mmoPlayer == null)
                 return false;
@@ -75,16 +70,38 @@ public class Roll extends AcrobaticsSubSkill {
              */
             
             if (canRoll(mmoPlayer)) {
-                entityDamageEvent.setDamage(
-                        rollCheck(mmoPlayer, entityDamageEvent.getFinalDamage(), mmoPlayer.getPlayer().isSneaking()));
+                final RollResult rollResult
+                        = rollCheck(mmoPlayer, entityDamageEvent);
+                if (rollResult == null) {
+                    // no-op - fall was fatal or otherwise did not get processed
+                    return false;
+                }
+                entityDamageEvent.setDamage(rollResult.getModifiedDamage());
 
                 if (entityDamageEvent.getFinalDamage() == 0) {
                     entityDamageEvent.setCancelled(true);
-                    return true;
                 }
+
+                // Roll happened, send messages and XP
+                if (rollResult.isRollSuccess()) {
+                    final String key
+                            = rollResult.isGraceful() ? GRACEFUL_ROLL_ACTIVATED_LOCALE_STR_KEY : ROLL_ACTIVATED_LOCALE_KEY;
+                    sendPlayerInformation(mmoPlayer.getPlayer(), NotificationType.SUBSKILL_MESSAGE, key);
+                    sendCategorizedSound(mmoPlayer.getPlayer(), mmoPlayer.getPlayer().getLocation(),
+                            SoundType.ROLL_ACTIVATED, SoundCategory.PLAYERS,0.5F);
+                }
+
+                if (!rollResult.isExploiting() && rollResult.getXpGain() > 0) {
+                    applyXpGain(mmoPlayer, getPrimarySkill(), rollResult.getXpGain(), XPGainReason.PVE);
+                }
+
+                // Player didn't die, so add the location to the list
+                addFallLocation(mmoPlayer);
+                return true;
+            // We give Acrobatics XP for fall damage even if they haven't unlocked roll
             } else if (mcMMO.p.getSkillTools().doesPlayerHaveSkillPermission(mmoPlayer.getPlayer(), PrimarySkillType.ACROBATICS)) {
-                //Give XP Anyways
-                SkillUtils.applyXpGain(mmoPlayer, getPrimarySkill(), calculateRollXP(mmoPlayer, ((EntityDamageEvent) event).getFinalDamage(), false), XPGainReason.PVE);
+                //Give XP
+                applyXpGain(mmoPlayer, getPrimarySkill(), calculateRollXP(mmoPlayer, ((EntityDamageEvent) event).getFinalDamage(), false), XPGainReason.PVE);
             }
         }
 
@@ -167,7 +184,7 @@ public class Roll extends AcrobaticsSubSkill {
 
     @NotNull
     private Probability getRollProbability(McMMOPlayer mmoPlayer) {
-        return ProbabilityUtil.getSubSkillProbability(SubSkillType.ACROBATICS_ROLL, mmoPlayer);
+        return getSubSkillProbability(SubSkillType.ACROBATICS_ROLL, mmoPlayer);
     }
 
     @Override
@@ -185,54 +202,67 @@ public class Roll extends AcrobaticsSubSkill {
         return true;
     }
 
-    private boolean canRoll(McMMOPlayer mmoPlayer) {
+    @VisibleForTesting
+    public boolean canRoll(McMMOPlayer mmoPlayer) {
         return RankUtils.hasUnlockedSubskill(mmoPlayer.getPlayer(), SubSkillType.ACROBATICS_ROLL)
                 && Permissions.isSubSkillEnabled(mmoPlayer.getPlayer(), SubSkillType.ACROBATICS_ROLL);
     }
 
-    private int getActivationChance(McMMOPlayer mmoPlayer) {
-        return PerksUtils.handleLuckyPerks(mmoPlayer, getPrimarySkill());
-    }
-
     /**
      * Handle the damage reduction and XP gain from the Roll / Graceful Roll ability
      *
-     * @param damage The amount of damage initially dealt by the event
+     * @param entityDamageEvent the event to modify in the event of roll success
      * @return the modified event damage if the ability was successful, the original event damage otherwise
      */
-    private double rollCheck(McMMOPlayer mmoPlayer, double damage, boolean isGracefulRoll) {
+    @VisibleForTesting
+    public RollResult rollCheck(McMMOPlayer mmoPlayer, EntityDamageEvent entityDamageEvent) {
+        double baseDamage = entityDamageEvent.getDamage();
+        final boolean isGraceful = mmoPlayer.getPlayer().isSneaking();
+        final RollResult.Builder rollResultBuilder
+                = new RollResult.Builder(entityDamageEvent, isGraceful);
         final Probability probability
-                = isGracefulRoll ? getGracefulProbability(mmoPlayer) : getNonGracefulProbability(mmoPlayer);
-        double modifiedDamage = calculateModifiedRollDamage(damage,
+                = isGraceful ? getGracefulProbability(mmoPlayer) : getNonGracefulProbability(mmoPlayer);
+
+        double modifiedDamage = calculateModifiedRollDamage(baseDamage,
                 mcMMO.p.getAdvancedConfig().getRollDamageThreshold() * 2);
+        rollResultBuilder.modifiedDamage(modifiedDamage);
 
+        boolean isExploiting = isPlayerExploitingAcrobatics(mmoPlayer);
+        rollResultBuilder.exploiting(isExploiting);
+        // They Rolled
         if (!isFatal(mmoPlayer, modifiedDamage)
                 && ProbabilityUtil.isStaticSkillRNGSuccessful(PrimarySkillType.ACROBATICS, mmoPlayer, probability)) {
-            NotificationManager.sendPlayerInformation(mmoPlayer.getPlayer(), NotificationType.SUBSKILL_MESSAGE, "Acrobatics.Ability.Proc");
-            SoundManager.sendCategorizedSound(mmoPlayer.getPlayer(), mmoPlayer.getPlayer().getLocation(), SoundType.ROLL_ACTIVATED, SoundCategory.PLAYERS,0.5F);
-            if (!isExploiting(mmoPlayer) && mmoPlayer.getAcrobaticsManager().canGainRollXP())
-                SkillUtils.applyXpGain(mmoPlayer, getPrimarySkill(), calculateRollXP(mmoPlayer, damage, true), XPGainReason.PVE);
-
-            addFallLocation(mmoPlayer);
-            return modifiedDamage;
-        } else if (!isFatal(mmoPlayer, damage)) {
-            if (!isExploiting(mmoPlayer) && mmoPlayer.getAcrobaticsManager().canGainRollXP())
-                SkillUtils.applyXpGain(mmoPlayer, getPrimarySkill(), calculateRollXP(mmoPlayer, damage, false), XPGainReason.PVE);
-            
-            addFallLocation(mmoPlayer);
-        }
+            rollResultBuilder.rollSuccess(true);
+            rollResultBuilder.exploiting(isExploiting);
+            final boolean canGainXp = mmoPlayer.getAcrobaticsManager().canGainRollXP();
 
-        return damage;
+            if (!isExploiting && canGainXp) {
+                rollResultBuilder.xpGain((int) calculateRollXP(mmoPlayer, baseDamage, true));
+            }
+
+            return rollResultBuilder.build();
+        // They did not roll, but they also did not die so reward XP as appropriate
+        } else if (!isFatal(mmoPlayer, baseDamage)) {
+            rollResultBuilder.rollSuccess(false);
+            final boolean canGainXp = mmoPlayer.getAcrobaticsManager().canGainRollXP();
+            if (!isExploiting && canGainXp) {
+                rollResultBuilder.xpGain((int) calculateRollXP(mmoPlayer, baseDamage, false));
+            }
+
+            return rollResultBuilder.build();
+        }
+        // Fall was fatal return null
+        return null;
     }
 
     @NotNull
     public static Probability getGracefulProbability(McMMOPlayer mmoPlayer) {
-        double gracefulOdds = ProbabilityUtil.getSubSkillProbability(SubSkillType.ACROBATICS_ROLL, mmoPlayer).getValue() * 2;
+        double gracefulOdds = getSubSkillProbability(SubSkillType.ACROBATICS_ROLL, mmoPlayer).getValue() * 2;
         return Probability.ofValue(gracefulOdds);
     }
 
     public static Probability getNonGracefulProbability(McMMOPlayer mmoPlayer) {
-        double gracefulOdds = ProbabilityUtil.getSubSkillProbability(SubSkillType.ACROBATICS_ROLL, mmoPlayer).getValue();
+        double gracefulOdds = getSubSkillProbability(SubSkillType.ACROBATICS_ROLL, mmoPlayer).getValue();
         return Probability.ofValue(gracefulOdds);
     }
 
@@ -242,7 +272,7 @@ public class Roll extends AcrobaticsSubSkill {
      *
      * @return true if exploits are detected, false otherwise
      */
-    private boolean isExploiting(McMMOPlayer mmoPlayer) {
+    private boolean isPlayerExploitingAcrobatics(McMMOPlayer mmoPlayer) {
         if (!ExperienceConfig.getInstance().isAcrobaticsExploitingPrevented()) {
             return false;
         }
@@ -331,11 +361,9 @@ public class Roll extends AcrobaticsSubSkill {
      */
     @Override
     public Double[] getStats(McMMOPlayer mmoPlayer) {
-        double playerChanceRoll = ProbabilityUtil.getSubSkillProbability(subSkillType, mmoPlayer).getValue();
+        double playerChanceRoll = getSubSkillProbability(subSkillType, mmoPlayer).getValue();
         double playerChanceGrace = playerChanceRoll * 2;
 
-        double gracefulOdds = ProbabilityUtil.getSubSkillProbability(subSkillType, mmoPlayer).getValue() * 2;
-
         return new Double[]{ playerChanceRoll, playerChanceGrace };
     }
 

src/main/java/com/gmail/nossr50/datatypes/skills/subskills/acrobatics/RollResult.java

@@ -0,0 +1,115 @@
+package com.gmail.nossr50.datatypes.skills.subskills.acrobatics;
+
+import org.bukkit.event.entity.EntityDamageEvent;
+
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Immutable class representing the result of a roll action in acrobatics.
+ */
+public class RollResult {
+    private final boolean rollSuccess;
+    private final boolean isGraceful;
+    private final double eventDamage;
+    private final double modifiedDamage;
+    private final boolean isFatal;
+    private final boolean isExploiting;
+    private final float xpGain;
+
+    private RollResult(Builder builder) {
+        this.rollSuccess = builder.rollSuccess;
+        this.isGraceful = builder.isGraceful;
+        this.eventDamage = builder.eventDamage;
+        this.modifiedDamage = builder.modifiedDamage;
+        this.isFatal = builder.isFatal;
+        this.isExploiting = builder.isExploiting;
+        this.xpGain = builder.xpGain;
+    }
+
+    public boolean isRollSuccess() {
+        return rollSuccess;
+    }
+
+    public boolean isGraceful() {
+        return isGraceful;
+    }
+
+    public double getEventDamage() {
+        return eventDamage;
+    }
+
+    public double getModifiedDamage() {
+        return modifiedDamage;
+    }
+
+    public boolean isFatal() {
+        return isFatal;
+    }
+
+    public boolean isExploiting() {
+        return isExploiting;
+    }
+
+    public float getXpGain() {
+        return xpGain;
+    }
+
+    /**
+     * Builder class for constructing {@code RollResult} instances.
+     */
+    public static class Builder {
+        private final boolean isGraceful;
+        private final double eventDamage;
+        private double modifiedDamage;
+        private boolean isFatal;
+        private boolean rollSuccess;
+        private boolean isExploiting;
+        private float xpGain;
+
+        /**
+         * Constructs a new {@code Builder} with required parameters.
+         *
+         * @param entityDamageEvent the damage event, must not be null
+         * @param isGracefulRoll    whether the roll is graceful
+         */
+        public Builder(EntityDamageEvent entityDamageEvent, boolean isGracefulRoll) {
+            requireNonNull(entityDamageEvent, "EntityDamageEvent cannot be null");
+            this.eventDamage = entityDamageEvent.getDamage();
+            this.isGraceful = isGracefulRoll;
+        }
+
+        public Builder modifiedDamage(double modifiedDamage) {
+            this.modifiedDamage = modifiedDamage;
+            return this;
+        }
+
+        public Builder fatal(boolean isFatal) {
+            this.isFatal = isFatal;
+            return this;
+        }
+
+        public Builder rollSuccess(boolean rollSuccess) {
+            this.rollSuccess = rollSuccess;
+            return this;
+        }
+
+        public Builder exploiting(boolean isExploiting) {
+            this.isExploiting = isExploiting;
+            return this;
+        }
+
+        public Builder xpGain(float xpGain) {
+            this.xpGain = xpGain;
+            return this;
+        }
+
+        /**
+         * Builds and returns a {@code RollResult} instance.
+         *
+         * @return a new {@code RollResult}
+         */
+        public RollResult build() {
+            return new RollResult(this);
+        }
+    }
+}

src/test/java/com/gmail/nossr50/MMOTestEnvironment.java

@@ -15,11 +15,16 @@ import com.gmail.nossr50.util.*;
 import com.gmail.nossr50.util.blockmeta.ChunkManager;
 import com.gmail.nossr50.util.compat.CompatibilityManager;
 import com.gmail.nossr50.util.platform.MinecraftGameVersion;
+import com.gmail.nossr50.util.player.NotificationManager;
 import com.gmail.nossr50.util.player.UserManager;
 import com.gmail.nossr50.util.skills.RankUtils;
 import com.gmail.nossr50.util.skills.SkillTools;
+import com.gmail.nossr50.util.sounds.SoundManager;
 import org.bukkit.*;
+import org.bukkit.attribute.Attribute;
+import org.bukkit.block.Block;
 import org.bukkit.entity.Player;
+import org.bukkit.event.Event;
 import org.bukkit.inventory.ItemFactory;
 import org.bukkit.inventory.ItemStack;
 import org.bukkit.inventory.PlayerInventory;
@@ -44,6 +49,8 @@ public abstract class MMOTestEnvironment {
     protected MockedStatic<Misc> mockedMisc;
     protected MockedStatic<SkillTools> mockedSkillTools;
     protected MockedStatic<EventUtils> mockedEventUtils;
+    protected MockedStatic<NotificationManager> notificationManager;
+    protected MockedStatic<SoundManager> mockedSoundManager;
     protected TransientEntityTracker transientEntityTracker;
     protected AdvancedConfig advancedConfig;
     protected PartyConfig partyConfig;
@@ -63,7 +70,6 @@ public abstract class MMOTestEnvironment {
     protected PlayerInventory playerInventory;
     protected PlayerProfile playerProfile;
     protected McMMOPlayer mmoPlayer;
-    protected String playerName = "testPlayer";
     protected ItemFactory itemFactory;
 
     protected ChunkManager chunkManager;
@@ -124,14 +130,25 @@ public abstract class MMOTestEnvironment {
         this.server = mock(Server.class);
         when(mcMMO.p.getServer()).thenReturn(server);
 
+        // wire Bukkit
         mockedBukkit = mockStatic(Bukkit.class);
         when(Bukkit.getItemFactory()).thenReturn(itemFactory);
         itemFactory = mock(ItemFactory.class);
-        // when(itemFactory.getItemMeta(any())).thenReturn(mock(ItemMeta.class));
+
+        // wire Bukkit call to get server
+        when(Bukkit.getServer()).thenReturn(server);
 
         // wire plugin manager
         this.pluginManager = mock(PluginManager.class);
+        // wire server -> plugin manager
         when(server.getPluginManager()).thenReturn(pluginManager);
+        // wire Bukkit -> plugin manager
+        when(Bukkit.getPluginManager()).thenReturn(pluginManager);
+        // return the argument provided when call event is invoked on plugin manager mock
+        doAnswer(invocation -> {
+            Object[] args = invocation.getArguments();
+            return args[0];
+        }).when(pluginManager).callEvent(any(Event.class));
 
         // wire world
         this.world = mock(World.class);
@@ -143,10 +160,19 @@ public abstract class MMOTestEnvironment {
         // setup player and player related mocks after everything else
         this.player = mock(Player.class);
         when(player.getUniqueId()).thenReturn(playerUUID);
-
+        when(player.isValid()).thenReturn(true);
+        when(player.isOnline()).thenReturn(true);
+        // health
+        when(player.getHealth()).thenReturn(20D);
         // wire inventory
         this.playerInventory = mock(PlayerInventory.class);
         when(player.getInventory()).thenReturn(playerInventory);
+        // player location
+        Location playerLocation = mock(Location.class);
+        Block playerLocationBlock = mock(Block.class);
+        when(player.getLocation()).thenReturn(playerLocation);
+        when(playerLocation.getBlock()).thenReturn(playerLocationBlock);
+        // when(playerLocationBlock.getType()).thenReturn(Material.AIR);
 
         // PlayerProfile and McMMOPlayer are partially mocked
         playerProfile = new PlayerProfile("testPlayer", player.getUniqueId(), 0);
@@ -158,6 +184,12 @@ public abstract class MMOTestEnvironment {
 
         this.materialMapStore = new MaterialMapStore();
         when(mcMMO.getMaterialMapStore()).thenReturn(materialMapStore);
+
+        // wire notification manager
+        notificationManager = mockStatic(NotificationManager.class);
+
+        // wire sound manager
+        mockedSoundManager = mockStatic(SoundManager.class);
     }
 
     private void mockPermissions() {
@@ -201,7 +233,7 @@ public abstract class MMOTestEnvironment {
         when(ExperienceConfig.getInstance().getCombatXP("Cow")).thenReturn(1D);
     }
 
-    protected void cleanupBaseEnvironment() {
+    protected void cleanUpStaticMocks() {
         // Clean up resources here if needed.
         if (mockedMcMMO != null) {
             mockedMcMMO.close();
@@ -230,5 +262,11 @@ public abstract class MMOTestEnvironment {
         if (mockedBukkit != null) {
             mockedBukkit.close();
         }
+        if (notificationManager != null) {
+            notificationManager.close();
+        }
+        if (mockedSoundManager != null) {
+            mockedSoundManager.close();
+        }
     }
 }

src/test/java/com/gmail/nossr50/party/PartyManagerTest.java

@@ -12,12 +12,13 @@ import org.mockito.Mockito;
 import java.util.UUID;
 import java.util.logging.Logger;
 
+import static java.util.logging.Logger.getLogger;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
 class PartyManagerTest extends MMOTestEnvironment {
-    private static final Logger logger = Logger.getLogger(PartyManagerTest.class.getName());
+    private static final Logger logger = getLogger(PartyManagerTest.class.getName());
 
     @BeforeEach
     public void setUp() {
@@ -29,7 +30,7 @@ class PartyManagerTest extends MMOTestEnvironment {
 
     @AfterEach
     public void tearDown() {
-        cleanupBaseEnvironment();
+        cleanUpStaticMocks();
 
         // disable parties in config for other tests
         Mockito.when(partyConfig.isPartyEnabled()).thenReturn(false);

src/test/java/com/gmail/nossr50/skills/acrobatics/AcrobaticsTest.java

@@ -0,0 +1,101 @@
+package com.gmail.nossr50.skills.acrobatics;
+
+import com.gmail.nossr50.MMOTestEnvironment;
+import com.gmail.nossr50.api.exceptions.InvalidSkillException;
+import com.gmail.nossr50.datatypes.skills.PrimarySkillType;
+import com.gmail.nossr50.datatypes.skills.SubSkillType;
+import com.gmail.nossr50.datatypes.skills.subskills.AbstractSubSkill;
+import com.gmail.nossr50.datatypes.skills.subskills.acrobatics.Roll;
+import com.gmail.nossr50.mcMMO;
+import com.gmail.nossr50.util.skills.RankUtils;
+import org.bukkit.entity.Player;
+import org.bukkit.event.entity.EntityDamageEvent;
+import org.jetbrains.annotations.NotNull;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.util.logging.Logger;
+
+import static java.util.logging.Logger.getLogger;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.*;
+
+class AcrobaticsTest extends MMOTestEnvironment {
+    private static final Logger logger = getLogger(AcrobaticsTest.class.getName());
+
+    @BeforeEach
+    void setUp() throws InvalidSkillException {
+        mockBaseEnvironment(logger);
+        when(rankConfig.getSubSkillUnlockLevel(SubSkillType.ACROBATICS_ROLL, 1)).thenReturn(1);
+        when(rankConfig.getSubSkillUnlockLevel(SubSkillType.ACROBATICS_DODGE, 1)).thenReturn(1);
+
+        // wire advanced config
+        when(advancedConfig.getMaximumProbability(SubSkillType.ACROBATICS_ROLL)).thenReturn(100D);
+        when(advancedConfig.getMaxBonusLevel(SubSkillType.ACROBATICS_ROLL)).thenReturn(1000);
+        when(advancedConfig.getRollDamageThreshold()).thenReturn(7D);
+
+        Mockito.when(RankUtils.getRankUnlockLevel(SubSkillType.ACROBATICS_ROLL, 1)).thenReturn(1); // needed?
+        Mockito.when(RankUtils.getRankUnlockLevel(SubSkillType.ACROBATICS_DODGE, 1)).thenReturn(1000); // needed?
+
+        when(RankUtils.getRankUnlockLevel(SubSkillType.ACROBATICS_ROLL, 1)).thenReturn(1); // needed?
+        when(RankUtils.hasReachedRank(eq(1), any(Player.class), eq(SubSkillType.ACROBATICS_ROLL))).thenReturn(true);
+        when(RankUtils.hasReachedRank(eq(1), any(Player.class), any(AbstractSubSkill.class))).thenReturn(true);
+    }
+
+    @AfterEach
+    void tearDown() {
+        cleanUpStaticMocks();
+    }
+
+    @SuppressWarnings("deprecation")
+    @Test
+    public void rollShouldLowerDamage() {
+        // Given
+        final Roll roll = new Roll();
+        final double damage = 2D;
+        final EntityDamageEvent mockEvent = mockEntityDamageEvent(damage);
+        mmoPlayer.modifySkill(PrimarySkillType.ACROBATICS, 1000);
+        when(roll.canRoll(mmoPlayer)).thenReturn(true);
+        assertThat(roll.canRoll(mmoPlayer)).isTrue();
+
+        // When
+        roll.doInteraction(mockEvent, mcMMO.p);
+
+        // Then
+        verify(mockEvent, atLeastOnce()).setDamage(0);
+    }
+
+    @SuppressWarnings("deprecation")
+    @Test
+    public void rollShouldNotLowerDamage() {
+        // Given
+        final Roll roll = new Roll();
+        final double damage = 100D;
+        final EntityDamageEvent mockEvent = mockEntityDamageEvent(damage);
+        mmoPlayer.modifySkill(PrimarySkillType.ACROBATICS, 0);
+        when(roll.canRoll(mmoPlayer)).thenReturn(true);
+        assertThat(roll.canRoll(mmoPlayer)).isTrue();
+
+        // When
+        roll.doInteraction(mockEvent, mcMMO.p);
+
+        // Then
+        assertThat(roll.canRoll(mmoPlayer)).isTrue();
+        verify(mockEvent, Mockito.never()).setDamage(any(Double.class));
+    }
+
+    private @NotNull EntityDamageEvent mockEntityDamageEvent(double damage) {
+        final EntityDamageEvent mockEvent = mock(EntityDamageEvent.class);
+        when(mockEvent.getCause()).thenReturn(EntityDamageEvent.DamageCause.FALL);
+        when(mockEvent.getFinalDamage()).thenReturn(damage);
+        when(mockEvent.getDamage(any(EntityDamageEvent.DamageModifier.class))).thenReturn(damage);
+        when(mockEvent.getDamage()).thenReturn(damage);
+        when(mockEvent.isCancelled()).thenReturn(false);
+        when(mockEvent.getEntity()).thenReturn(player);
+        return mockEvent;
+    }
+}

src/test/java/com/gmail/nossr50/skills/excavation/ExcavationTest.java

@@ -13,7 +13,6 @@ import org.bukkit.block.BlockState;
 import org.bukkit.block.data.BlockData;
 import org.bukkit.entity.Player;
 import org.bukkit.inventory.ItemStack;
-import org.bukkit.inventory.PlayerInventory;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -29,7 +28,6 @@ import static org.mockito.Mockito.*;
 class ExcavationTest extends MMOTestEnvironment {
     private static final java.util.logging.Logger logger = java.util.logging.Logger.getLogger(ExcavationTest.class.getName());
 
-
     @BeforeEach
     void setUp() throws InvalidSkillException {
         mockBaseEnvironment(logger);
@@ -48,18 +46,13 @@ class ExcavationTest extends MMOTestEnvironment {
         when(player.getUniqueId()).thenReturn(playerUUID);
 
         // wire inventory
-        this.playerInventory = Mockito.mock(PlayerInventory.class);
         this.itemInMainHand = new ItemStack(Material.DIAMOND_SHOVEL);
-        when(player.getInventory()).thenReturn(playerInventory);
         when(playerInventory.getItemInMainHand()).thenReturn(itemInMainHand);
-
-        // Set up spy for Excavation Manager
-
     }
 
     @AfterEach
     void tearDown() {
-        cleanupBaseEnvironment();
+        cleanUpStaticMocks();
     }
 
     @Test

src/test/java/com/gmail/nossr50/skills/tridents/TridentsTest.java

@@ -10,8 +10,12 @@ import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.mockito.Mockito;
 
+import java.util.logging.Logger;
+
+import static java.util.logging.Logger.getLogger;
+
 class TridentsTest extends MMOTestEnvironment {
-    private static final java.util.logging.Logger logger = java.util.logging.Logger.getLogger(TridentsTest.class.getName());
+    private static final Logger logger = getLogger(TridentsTest.class.getName());
 
     TridentsManager tridentsManager;
     ItemStack trident;
@@ -34,6 +38,6 @@ class TridentsTest extends MMOTestEnvironment {
 
     @AfterEach
     void tearDown() {
-        cleanupBaseEnvironment();
+        cleanUpStaticMocks();
     }
 }

src/test/java/com/gmail/nossr50/skills/woodcutting/WoodcuttingTest.java

@@ -11,21 +11,23 @@ import org.bukkit.block.Block;
 import org.bukkit.block.BlockState;
 import org.bukkit.entity.Player;
 import org.bukkit.inventory.ItemStack;
-import org.bukkit.inventory.PlayerInventory;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mockito;
 
 import java.util.Collections;
+import java.util.logging.Logger;
 
+import static java.util.logging.Logger.getLogger;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 
 class WoodcuttingTest extends MMOTestEnvironment {
-    private static final java.util.logging.Logger logger = java.util.logging.Logger.getLogger(WoodcuttingTest.class.getName());
+    private static final Logger logger = getLogger(WoodcuttingTest.class.getName());
+
+    private WoodcuttingManager woodcuttingManager;
 
-    WoodcuttingManager woodcuttingManager;
     @BeforeEach
     void setUp() throws InvalidSkillException {
         mockBaseEnvironment(logger);
@@ -42,12 +44,7 @@ class WoodcuttingTest extends MMOTestEnvironment {
         Mockito.when(RankUtils.hasReachedRank(eq(1), any(Player.class), eq(SubSkillType.WOODCUTTING_HARVEST_LUMBER))).thenReturn(true);
         Mockito.when(RankUtils.hasReachedRank(eq(1), any(Player.class), eq(SubSkillType.WOODCUTTING_CLEAN_CUTS))).thenReturn(true);
 
-        // setup player and player related mocks after everything else
-        this.player = Mockito.mock(Player.class);
-        Mockito.when(player.getUniqueId()).thenReturn(playerUUID);
-
         // wire inventory
-        this.playerInventory = Mockito.mock(PlayerInventory.class);
         this.itemInMainHand = new ItemStack(Material.DIAMOND_AXE);
         Mockito.when(player.getInventory()).thenReturn(playerInventory);
         Mockito.when(playerInventory.getItemInMainHand()).thenReturn(itemInMainHand);
@@ -58,7 +55,7 @@ class WoodcuttingTest extends MMOTestEnvironment {
 
     @AfterEach
     void tearDown() {
-        cleanupBaseEnvironment();
+        cleanUpStaticMocks();
     }
 
     @Test

src/test/java/com/gmail/nossr50/util/random/ProbabilityUtilTest.java

@@ -38,7 +38,7 @@ class ProbabilityUtilTest extends MMOTestEnvironment {
 
     @AfterEach
     public void tearDown() {
-        cleanupBaseEnvironment();
+        cleanUpStaticMocks();
     }
 
     private static Stream<Arguments> staticChanceSkills() {