From 6585b68749b0cb843b57f81517e08aae3ebdd65b Mon Sep 17 00:00:00 2001 From: ljacqu Date: Thu, 30 Jun 2016 22:38:36 +0200 Subject: [PATCH] Fix TeleportationService tests + rename methods - Fix and supplement unit tests for TeleportationService - Rename methods as to avoid confusion (login vs. LoginEvent when player joins) - Add javadoc with note about Player#hasPlayedBefore always being false --- .../authme/listener/AuthMePlayerListener.java | 4 +- .../authme/util/TeleportationService.java | 57 ++++++++------ .../authme/util/TeleportationServiceTest.java | 75 ++++++++++++------- 3 files changed, 86 insertions(+), 50 deletions(-) diff --git a/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java b/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java index fc92e1e88..0e5915e1d 100644 --- a/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java +++ b/src/main/java/fr/xephi/authme/listener/AuthMePlayerListener.java @@ -194,7 +194,7 @@ public class AuthMePlayerListener implements Listener { @EventHandler(priority = EventPriority.LOW) public void onPlayerJoin(PlayerJoinEvent event) { final Player player = event.getPlayer(); - teleportationService.teleportOnJoin(player); + teleportationService.teleportNewPlayerToFirstSpawn(player); management.performJoin(player); } @@ -233,7 +233,7 @@ public class AuthMePlayerListener implements Listener { } antiBot.handlePlayerJoin(player); - teleportationService.teleportOnLoginEvent(player); + teleportationService.teleportOnJoin(player); } @EventHandler(priority = EventPriority.HIGHEST) diff --git a/src/main/java/fr/xephi/authme/util/TeleportationService.java b/src/main/java/fr/xephi/authme/util/TeleportationService.java index 53e77421b..a7cffb35e 100644 --- a/src/main/java/fr/xephi/authme/util/TeleportationService.java +++ b/src/main/java/fr/xephi/authme/util/TeleportationService.java @@ -44,10 +44,6 @@ public class TeleportationService implements Reloadable { TeleportationService() { } - private static boolean isEventValid(AbstractTeleportEvent event) { - return !event.isCancelled() && event.getTo() != null && event.getTo().getWorld() != null; - } - @PostConstruct @Override public void reload() { @@ -55,7 +51,18 @@ public class TeleportationService implements Reloadable { spawnOnLoginWorlds = new HashSet<>(settings.getProperty(RestrictionSettings.FORCE_SPAWN_ON_WORLDS)); } - public void teleportOnLoginEvent(final Player player) { + /** + * Teleports the player according to the settings when he joins. + *

+ * Note: this is triggered by Bukkit's PlayerLoginEvent, during which you cannot use + * {@link Player#hasPlayedBefore()}: it always returns {@code false}. We trigger teleportation + * from the PlayerLoginEvent and not the PlayerJoinEvent to ensure that the location is overridden + * as fast as possible (cf. AuthMe #682). + * + * @param player the player to process + * @see BUKKIT-3521: Player.hasPlayedBefore() always false + */ + public void teleportOnJoin(final Player player) { if (settings.getProperty(RestrictionSettings.NO_TELEPORT)) { return; } @@ -65,13 +72,30 @@ public class TeleportationService implements Reloadable { } } - public void teleportOnJoin(final Player player) { - if (settings.getProperty(RestrictionSettings.NO_TELEPORT)) { + /** + * Teleports the player to the first spawn if he is new and the first spawn is configured. + * + * @param player the player to process + */ + public void teleportNewPlayerToFirstSpawn(final Player player) { + if (settings.getProperty(RestrictionSettings.NO_TELEPORT) || player.hasPlayedBefore()) { return; } - teleportToFirstSpawn(player); + Location firstSpawn = spawnLoader.getFirstSpawn(); + if (firstSpawn == null) { + return; + } + + performTeleportation(player, new FirstSpawnTeleportEvent(player, firstSpawn)); } + /** + * Teleports the player according to the settings after having successfully logged in. + * + * @param player the player + * @param auth corresponding PlayerAuth object + * @param limbo corresponding LimboPlayer object + */ public void teleportOnLogin(final Player player, PlayerAuth auth, LimboPlayer limbo) { if (settings.getProperty(RestrictionSettings.NO_TELEPORT)) { return; @@ -104,19 +128,6 @@ public class TeleportationService implements Reloadable { return new Location(world, auth.getQuitLocX(), auth.getQuitLocY(), auth.getQuitLocZ()); } - private boolean teleportToFirstSpawn(final Player player) { - if (player.hasPlayedBefore()) { - return false; - } - Location firstSpawn = spawnLoader.getFirstSpawn(); - if (firstSpawn == null) { - return false; - } - - performTeleportation(player, new FirstSpawnTeleportEvent(player, firstSpawn)); - return true; - } - private void teleportBackFromSpawn(final Player player, final Location location) { performTeleportation(player, new AuthMeTeleportEvent(player, location)); } @@ -144,4 +155,8 @@ public class TeleportationService implements Reloadable { } }); } + + private static boolean isEventValid(AbstractTeleportEvent event) { + return !event.isCancelled() && event.getTo() != null && event.getTo().getWorld() != null; + } } diff --git a/src/test/java/fr/xephi/authme/util/TeleportationServiceTest.java b/src/test/java/fr/xephi/authme/util/TeleportationServiceTest.java index de8d286fa..84e1a1de0 100644 --- a/src/test/java/fr/xephi/authme/util/TeleportationServiceTest.java +++ b/src/test/java/fr/xephi/authme/util/TeleportationServiceTest.java @@ -38,7 +38,6 @@ import static org.mockito.Mockito.verifyZeroInteractions; /** * Test for {@link TeleportationService}. */ -// TODO: Correct me! @RunWith(MockitoJUnitRunner.class) public class TeleportationServiceTest { @@ -57,20 +56,6 @@ public class TeleportationServiceTest { @Mock private PlayerCache playerCache; - // We check that the World in Location is set, this method creates a mock World in Location for us - private static Location mockLocation() { - Location location = mock(Location.class); - given(location.getWorld()).willReturn(mock(World.class)); - return location; - } - - private static PlayerAuth createAuthWithLocation() { - return PlayerAuth.builder() - .name("bobby") - .locX(123.45).locY(23.4).locZ(-4.567) - .build(); - } - @Before public void setUpForcedWorlds() { given(settings.getProperty(RestrictionSettings.FORCE_SPAWN_ON_WORLDS)) @@ -107,7 +92,7 @@ public class TeleportationServiceTest { given(spawnLoader.getFirstSpawn()).willReturn(firstSpawn); // when - teleportationService.teleportOnJoin(player); + teleportationService.teleportNewPlayerToFirstSpawn(player); runSyncDelayedTask(bukkitService); // then @@ -122,13 +107,12 @@ public class TeleportationServiceTest { // given given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true); Player player = mock(Player.class); - given(player.hasPlayedBefore()).willReturn(true); given(player.isOnline()).willReturn(true); Location spawn = mockLocation(); given(spawnLoader.getSpawnLocation(player)).willReturn(spawn); // when - teleportationService.teleportOnLoginEvent(player); + teleportationService.teleportOnJoin(player); runSyncDelayedTask(bukkitService); // then @@ -150,8 +134,7 @@ public class TeleportationServiceTest { given(spawnLoader.getFirstSpawn()).willReturn(null); // when - teleportationService.teleportOnLoginEvent(player); - teleportationService.teleportOnJoin(player); + teleportationService.teleportNewPlayerToFirstSpawn(player); // then verify(player, never()).teleport(any(Location.class)); @@ -161,10 +144,39 @@ public class TeleportationServiceTest { } @Test - public void shouldTeleportPlayerDueToForcedWorld() { + public void shouldNotTeleportPlayerToFirstSpawnIfNoTeleportEnabled() { + // given + Player player = mock(Player.class); + given(player.hasPlayedBefore()).willReturn(false); + given(settings.getProperty(RestrictionSettings.NO_TELEPORT)).willReturn(true); + + // when + teleportationService.teleportNewPlayerToFirstSpawn(player); + + // then + verify(player, never()).teleport(any(Location.class)); + verifyZeroInteractions(bukkitService); + } + + @Test + public void shouldNotTeleportNotNewPlayerToFirstSpawn() { // given Player player = mock(Player.class); given(player.hasPlayedBefore()).willReturn(true); + given(settings.getProperty(RestrictionSettings.NO_TELEPORT)).willReturn(false); + + // when + teleportationService.teleportNewPlayerToFirstSpawn(player); + + // then + verify(player, never()).teleport(any(Location.class)); + verifyZeroInteractions(bukkitService); + } + + @Test + public void shouldTeleportPlayerDueToForcedWorld() { + // given + Player player = mock(Player.class); given(player.isOnline()).willReturn(true); World playerWorld = mock(World.class); @@ -177,7 +189,6 @@ public class TeleportationServiceTest { given(spawnLoader.getSpawnLocation(player)).willReturn(spawn); // when - teleportationService.teleportOnLoginEvent(player); teleportationService.teleportOnJoin(player); runSyncDelayedTask(bukkitService); @@ -191,7 +202,6 @@ public class TeleportationServiceTest { public void shouldNotTeleportPlayerForRemovedLocationInEvent() { // given final Player player = mock(Player.class); - given(player.hasPlayedBefore()).willReturn(true); Location spawn = mockLocation(); given(spawnLoader.getSpawnLocation(player)).willReturn(spawn); given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true); @@ -206,7 +216,6 @@ public class TeleportationServiceTest { }).when(bukkitService).callEvent(any(SpawnTeleportEvent.class)); // when - teleportationService.teleportOnLoginEvent(player); teleportationService.teleportOnJoin(player); runSyncDelayedTask(bukkitService); @@ -219,7 +228,6 @@ public class TeleportationServiceTest { public void shouldNotTeleportPlayerForCanceledEvent() { // given final Player player = mock(Player.class); - given(player.hasPlayedBefore()).willReturn(true); Location spawn = mockLocation(); given(spawnLoader.getSpawnLocation(player)).willReturn(spawn); given(settings.getProperty(RestrictionSettings.TELEPORT_UNAUTHED_TO_SPAWN)).willReturn(true); @@ -234,7 +242,6 @@ public class TeleportationServiceTest { }).when(bukkitService).callEvent(any(SpawnTeleportEvent.class)); // when - teleportationService.teleportOnLoginEvent(player); teleportationService.teleportOnJoin(player); runSyncDelayedTask(bukkitService); @@ -415,11 +422,25 @@ public class TeleportationServiceTest { verify(player).teleport(location); } - private void assertCorrectLocation(Location location, PlayerAuth auth, World world) { + private static void assertCorrectLocation(Location location, PlayerAuth auth, World world) { assertThat(location.getX(), equalTo(auth.getQuitLocX())); assertThat(location.getY(), equalTo(auth.getQuitLocY())); assertThat(location.getZ(), equalTo(auth.getQuitLocZ())); assertThat(location.getWorld(), equalTo(world)); } + // We check that the World in Location is set, this method creates a mock World in Location for us + private static Location mockLocation() { + Location location = mock(Location.class); + given(location.getWorld()).willReturn(mock(World.class)); + return location; + } + + private static PlayerAuth createAuthWithLocation() { + return PlayerAuth.builder() + .name("bobby") + .locX(123.45).locY(23.4).locZ(-4.567) + .build(); + } + }