From 4717dc148c3a005800291965187439ee3143ae8f Mon Sep 17 00:00:00 2001 From: ljacqu Date: Tue, 21 Nov 2017 23:48:15 +0100 Subject: [PATCH] #1413 Don't run onUnregister command in async --- .../unregister/AsynchronousUnregister.java | 3 +- src/test/java/fr/xephi/authme/TestHelper.java | 19 +++---- .../authme/RegisterAdminCommandTest.java | 4 +- .../AsynchronousUnregisterTest.java | 31 +++++++++--- .../service/TeleportationServiceTest.java | 49 ++++++++----------- 5 files changed, 58 insertions(+), 48 deletions(-) diff --git a/src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java b/src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java index 70a3d1611..7291be685 100644 --- a/src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java +++ b/src/main/java/fr/xephi/authme/process/unregister/AsynchronousUnregister.java @@ -120,7 +120,8 @@ public class AsynchronousUnregister implements AsynchronousProcess { if (player == null || !player.isOnline()) { return; } - commandManager.runCommandsOnUnregister(player); + bukkitService.scheduleSyncTaskFromOptionallyAsyncTask(() -> + commandManager.runCommandsOnUnregister(player)); if (service.getProperty(RegistrationSettings.FORCE)) { teleportationService.teleportOnJoin(player); diff --git a/src/test/java/fr/xephi/authme/TestHelper.java b/src/test/java/fr/xephi/authme/TestHelper.java index 5a60bc1e6..2f388034d 100644 --- a/src/test/java/fr/xephi/authme/TestHelper.java +++ b/src/test/java/fr/xephi/authme/TestHelper.java @@ -23,6 +23,7 @@ import java.util.logging.Logger; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -134,17 +135,17 @@ public final class TestHelper { } /** - * Execute a {@link Runnable} passed to a mock's {@link BukkitService#scheduleSyncTaskFromOptionallyAsyncTask} - * method. Note that calling this method expects that there be a runnable sent to the method and will fail - * otherwise. + * Sets a BukkitService mock to run any Runnable it is passed to its method + * {@link BukkitService#scheduleSyncTaskFromOptionallyAsyncTask}. * - * @param service The mock service + * @param bukkitService the mock to set behavior on */ - public static void runSyncTaskFromOptionallyAsyncTask(BukkitService service) { - ArgumentCaptor captor = ArgumentCaptor.forClass(Runnable.class); - verify(service).scheduleSyncTaskFromOptionallyAsyncTask(captor.capture()); - Runnable runnable = captor.getValue(); - runnable.run(); + public static void setBukkitServiceToRunOptionallyAsyncTasks(BukkitService bukkitService) { + doAnswer(invocation -> { + Runnable runnable = invocation.getArgument(0); + runnable.run(); + return null; + }).when(bukkitService).scheduleSyncTaskFromOptionallyAsyncTask(any(Runnable.class)); } /** diff --git a/src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java b/src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java index bacbeb7b8..fd30dc92a 100644 --- a/src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java +++ b/src/test/java/fr/xephi/authme/command/executable/authme/RegisterAdminCommandTest.java @@ -22,7 +22,7 @@ import org.mockito.junit.MockitoJUnitRunner; import java.util.Arrays; -import static fr.xephi.authme.TestHelper.runSyncTaskFromOptionallyAsyncTask; +import static fr.xephi.authme.TestHelper.setBukkitServiceToRunOptionallyAsyncTasks; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; import static org.mockito.ArgumentMatchers.any; @@ -161,11 +161,11 @@ public class RegisterAdminCommandTest { String kickForAdminRegister = "Admin registered you -- log in again"; given(commandService.retrieveSingleMessage(MessageKey.KICK_FOR_ADMIN_REGISTER)).willReturn(kickForAdminRegister); CommandSender sender = mock(CommandSender.class); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when command.executeCommand(sender, Arrays.asList(user, password)); TestHelper.runOptionallyAsyncTask(bukkitService); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then verify(validationService).validatePassword(password, user); diff --git a/src/test/java/fr/xephi/authme/process/unregister/AsynchronousUnregisterTest.java b/src/test/java/fr/xephi/authme/process/unregister/AsynchronousUnregisterTest.java index 61bba4227..c15c8e279 100644 --- a/src/test/java/fr/xephi/authme/process/unregister/AsynchronousUnregisterTest.java +++ b/src/test/java/fr/xephi/authme/process/unregister/AsynchronousUnregisterTest.java @@ -13,10 +13,14 @@ import fr.xephi.authme.service.BukkitService; import fr.xephi.authme.service.CommonService; import fr.xephi.authme.service.TeleportationService; import fr.xephi.authme.service.bungeecord.BungeeSender; +import fr.xephi.authme.service.bungeecord.MessageType; import fr.xephi.authme.settings.commandconfig.CommandManager; import fr.xephi.authme.settings.properties.RegistrationSettings; +import fr.xephi.authme.settings.properties.RestrictionSettings; import org.bukkit.command.CommandSender; import org.bukkit.entity.Player; +import org.bukkit.potion.PotionEffect; +import org.bukkit.potion.PotionEffectType; import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; @@ -27,6 +31,7 @@ import org.mockito.junit.MockitoJUnitRunner; import java.util.function.Function; +import static fr.xephi.authme.TestHelper.setBukkitServiceToRunOptionallyAsyncTasks; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; import static org.mockito.ArgumentMatchers.any; @@ -89,7 +94,7 @@ public class AsynchronousUnregisterTest { // then verify(service).send(player, MessageKey.WRONG_PASSWORD); verify(passwordSecurity).comparePassword(userPassword, password, name); - verifyZeroInteractions(dataSource, limboService, teleportationService, bukkitService); + verifyZeroInteractions(dataSource, limboService, teleportationService, bukkitService, bungeeSender); verify(player, only()).getName(); } @@ -108,6 +113,9 @@ public class AsynchronousUnregisterTest { given(passwordSecurity.comparePassword(userPassword, password, name)).willReturn(true); given(dataSource.removeAuth(name)).willReturn(true); given(service.getProperty(RegistrationSettings.FORCE)).willReturn(true); + given(service.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)).willReturn(true); + given(service.getProperty(RestrictionSettings.TIMEOUT)).willReturn(21); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when asynchronousUnregister.unregister(player, userPassword); @@ -118,9 +126,10 @@ public class AsynchronousUnregisterTest { verify(dataSource).removeAuth(name); verify(playerCache).removePlayer(name); verify(teleportationService).teleportOnJoin(player); - verify(bukkitService).scheduleSyncTaskFromOptionallyAsyncTask(any(Runnable.class)); verifyCalledUnregisterEventFor(player); verify(commandManager).runCommandsOnUnregister(player); + verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name); + verify(player).addPotionEffect(new PotionEffect(PotionEffectType.BLINDNESS, 21 * 20, 2)); } @Test @@ -138,6 +147,8 @@ public class AsynchronousUnregisterTest { given(passwordSecurity.comparePassword(userPassword, password, name)).willReturn(true); given(dataSource.removeAuth(name)).willReturn(true); given(service.getProperty(RegistrationSettings.FORCE)).willReturn(true); + given(service.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)).willReturn(false); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when asynchronousUnregister.unregister(player, userPassword); @@ -148,9 +159,10 @@ public class AsynchronousUnregisterTest { verify(dataSource).removeAuth(name); verify(playerCache).removePlayer(name); verify(teleportationService).teleportOnJoin(player); - verify(bukkitService).scheduleSyncTaskFromOptionallyAsyncTask(any(Runnable.class)); verifyCalledUnregisterEventFor(player); verify(commandManager).runCommandsOnUnregister(player); + verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name); + verify(player, never()).addPotionEffect(any(PotionEffect.class)); } @Test @@ -167,6 +179,7 @@ public class AsynchronousUnregisterTest { given(passwordSecurity.comparePassword(userPassword, password, name)).willReturn(true); given(dataSource.removeAuth(name)).willReturn(true); given(service.getProperty(RegistrationSettings.FORCE)).willReturn(false); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when asynchronousUnregister.unregister(player, userPassword); @@ -177,8 +190,8 @@ public class AsynchronousUnregisterTest { verify(dataSource).removeAuth(name); verify(playerCache).removePlayer(name); verifyZeroInteractions(teleportationService, limboService); - verify(bukkitService, never()).runTask(any(Runnable.class)); verifyCalledUnregisterEventFor(player); + verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name); verify(commandManager).runCommandsOnUnregister(player); } @@ -203,7 +216,7 @@ public class AsynchronousUnregisterTest { verify(passwordSecurity).comparePassword(userPassword, password, name); verify(dataSource).removeAuth(name); verify(service).send(player, MessageKey.ERROR); - verifyZeroInteractions(teleportationService, bukkitService); + verifyZeroInteractions(teleportationService, bukkitService, bungeeSender); } @Test @@ -230,6 +243,7 @@ public class AsynchronousUnregisterTest { verify(playerCache).removePlayer(name); verifyZeroInteractions(teleportationService); verifyCalledUnregisterEventFor(player); + verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name); } // Initiator known and Player object available @@ -241,7 +255,9 @@ public class AsynchronousUnregisterTest { given(player.isOnline()).willReturn(true); given(dataSource.removeAuth(name)).willReturn(true); given(service.getProperty(RegistrationSettings.FORCE)).willReturn(true); + given(service.getProperty(RegistrationSettings.APPLY_BLIND_EFFECT)).willReturn(false); CommandSender initiator = mock(CommandSender.class); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when asynchronousUnregister.adminUnregister(initiator, name, player); @@ -252,9 +268,9 @@ public class AsynchronousUnregisterTest { verify(dataSource).removeAuth(name); verify(playerCache).removePlayer(name); verify(teleportationService).teleportOnJoin(player); - verify(bukkitService).scheduleSyncTaskFromOptionallyAsyncTask(any(Runnable.class)); verifyCalledUnregisterEventFor(player); verify(commandManager).runCommandsOnUnregister(player); + verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name); } @Test @@ -271,6 +287,7 @@ public class AsynchronousUnregisterTest { verify(playerCache).removePlayer(name); verifyZeroInteractions(teleportationService); verifyCalledUnregisterEventFor(null); + verify(bungeeSender).sendAuthMeBungeecordMessage(MessageType.UNREGISTER, name); } @Test @@ -286,7 +303,7 @@ public class AsynchronousUnregisterTest { // then verify(dataSource).removeAuth(name); verify(service).send(initiator, MessageKey.ERROR); - verifyZeroInteractions(playerCache, teleportationService, bukkitService); + verifyZeroInteractions(playerCache, teleportationService, bukkitService, bungeeSender); } @SuppressWarnings("unchecked") diff --git a/src/test/java/fr/xephi/authme/service/TeleportationServiceTest.java b/src/test/java/fr/xephi/authme/service/TeleportationServiceTest.java index 64f7ce462..38540fa9c 100644 --- a/src/test/java/fr/xephi/authme/service/TeleportationServiceTest.java +++ b/src/test/java/fr/xephi/authme/service/TeleportationServiceTest.java @@ -17,14 +17,11 @@ import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; import org.mockito.InjectMocks; import org.mockito.Mock; -import org.mockito.invocation.InvocationOnMock; import org.mockito.junit.MockitoJUnitRunner; -import org.mockito.stubbing.Answer; import java.util.Arrays; -import static fr.xephi.authme.TestHelper.runSyncDelayedTask; -import static fr.xephi.authme.TestHelper.runSyncTaskFromOptionallyAsyncTask; +import static fr.xephi.authme.TestHelper.setBukkitServiceToRunOptionallyAsyncTasks; import static org.hamcrest.Matchers.equalTo; import static org.junit.Assert.assertThat; import static org.mockito.ArgumentMatchers.any; @@ -92,10 +89,10 @@ public class TeleportationServiceTest { given(player.isOnline()).willReturn(true); Location firstSpawn = mockLocation(); given(spawnLoader.getFirstSpawn()).willReturn(firstSpawn); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportNewPlayerToFirstSpawn(player); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then verify(player).teleport(firstSpawn); @@ -112,10 +109,10 @@ public class TeleportationServiceTest { given(player.isOnline()).willReturn(true); Location spawn = mockLocation(); given(spawnLoader.getSpawnLocation(player)).willReturn(spawn); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportOnJoin(player); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then verify(player).teleport(spawn); @@ -175,19 +172,16 @@ public class TeleportationServiceTest { Location spawn = mockLocation(); given(spawnLoader.getSpawnLocation(player)).willReturn(spawn); given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true); - doAnswer(new Answer() { - @Override - public Void answer(InvocationOnMock invocation) throws Throwable { - SpawnTeleportEvent event = (SpawnTeleportEvent) invocation.getArguments()[0]; - assertThat(event.getPlayer(), equalTo(player)); - event.setTo(null); - return null; - } + doAnswer(invocation -> { + SpawnTeleportEvent event = (SpawnTeleportEvent) invocation.getArguments()[0]; + assertThat(event.getPlayer(), equalTo(player)); + event.setTo(null); + return null; }).when(bukkitService).callEvent(any(SpawnTeleportEvent.class)); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportOnJoin(player); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then verify(bukkitService).callEvent(any(SpawnTeleportEvent.class)); @@ -201,19 +195,16 @@ public class TeleportationServiceTest { Location spawn = mockLocation(); given(spawnLoader.getSpawnLocation(player)).willReturn(spawn); given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true); - doAnswer(new Answer() { - @Override - public Void answer(InvocationOnMock invocation) throws Throwable { - SpawnTeleportEvent event = (SpawnTeleportEvent) invocation.getArguments()[0]; - assertThat(event.getPlayer(), equalTo(player)); - event.setCancelled(true); - return null; - } + doAnswer(invocation -> { + SpawnTeleportEvent event = (SpawnTeleportEvent) invocation.getArguments()[0]; + assertThat(event.getPlayer(), equalTo(player)); + event.setCancelled(true); + return null; }).when(bukkitService).callEvent(any(SpawnTeleportEvent.class)); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportOnJoin(player); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then verify(bukkitService).callEvent(any(SpawnTeleportEvent.class)); @@ -251,10 +242,10 @@ public class TeleportationServiceTest { Location limboLocation = mockLocation(); given(limboLocation.getWorld().getName()).willReturn("forced1"); given(limbo.getLocation()).willReturn(limboLocation); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportOnLogin(player, auth, limbo); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then verify(player).teleport(spawn); @@ -298,10 +289,10 @@ public class TeleportationServiceTest { LimboPlayer limbo = mock(LimboPlayer.class); Location limboLocation = mockLocation(); given(limbo.getLocation()).willReturn(limboLocation); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportOnLogin(player, auth, limbo); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then ArgumentCaptor locationCaptor = ArgumentCaptor.forClass(Location.class); @@ -326,10 +317,10 @@ public class TeleportationServiceTest { LimboPlayer limbo = mock(LimboPlayer.class); Location limboLocation = mockLocation(); given(limbo.getLocation()).willReturn(limboLocation); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportOnLogin(player, auth, limbo); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then ArgumentCaptor locationCaptor = ArgumentCaptor.forClass(Location.class); @@ -351,10 +342,10 @@ public class TeleportationServiceTest { LimboPlayer limbo = mock(LimboPlayer.class); Location location = mockLocation(); given(limbo.getLocation()).willReturn(location); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportOnLogin(player, auth, limbo); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then verify(player).teleport(location); @@ -373,10 +364,10 @@ public class TeleportationServiceTest { LimboPlayer limbo = mock(LimboPlayer.class); Location location = mockLocation(); given(limbo.getLocation()).willReturn(location); + setBukkitServiceToRunOptionallyAsyncTasks(bukkitService); // when teleportationService.teleportOnLogin(player, auth, limbo); - runSyncTaskFromOptionallyAsyncTask(bukkitService); // then verify(player).teleport(location);