From ca4a64f398dd0b01fddcb5b6c2fc52e23890ec87 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 17 Jul 2016 21:12:01 +0200 Subject: [PATCH] #844 Run PurgeTask as repeating task and #784 write tests for PurgeTask --- .../authme/task/purge/PurgeExecutor.java | 2 +- .../xephi/authme/task/purge/PurgeService.java | 2 +- .../fr/xephi/authme/task/purge/PurgeTask.java | 4 +- .../fr/xephi/authme/util/BukkitService.java | 19 ++ .../authme/task/purge/PurgeServiceTest.java | 41 ++-- .../authme/task/purge/PurgeTaskTest.java | 221 ++++++++++++++++++ 6 files changed, 270 insertions(+), 19 deletions(-) create mode 100644 src/test/java/fr/xephi/authme/task/purge/PurgeTaskTest.java diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java b/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java index 9a822d74..a8805238 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeExecutor.java @@ -49,7 +49,7 @@ class PurgeExecutor { * players and names. * * @param players the players to purge - * @param names names to purge (lowercase) + * @param names names to purge */ public void executePurge(Collection players, Collection names) { // Purge other data diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeService.java b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java index 4b1bf40d..8f6f42f7 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeService.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeService.java @@ -91,7 +91,7 @@ public class PurgeService { isPurging = true; PurgeTask purgeTask = new PurgeTask(this, permissionsManager, sender, names, players); - bukkitService.runTaskAsynchronously(purgeTask); + bukkitService.runTaskTimer(purgeTask, 0, 1); } /** diff --git a/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java b/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java index fc879699..a69c5fee 100644 --- a/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java +++ b/src/main/java/fr/xephi/authme/task/purge/PurgeTask.java @@ -104,9 +104,9 @@ class PurgeTask extends BukkitRunnable { cancel(); // Show a status message - sendMessage(ChatColor.GREEN + "[AuthMe] Database has been purged correctly"); + sendMessage(ChatColor.GREEN + "[AuthMe] Database has been purged successfully"); - ConsoleLogger.info("Purge Finished!"); + ConsoleLogger.info("Purge finished!"); purgeService.setPurging(false); } diff --git a/src/main/java/fr/xephi/authme/util/BukkitService.java b/src/main/java/fr/xephi/authme/util/BukkitService.java index 6913c2af..12448809 100644 --- a/src/main/java/fr/xephi/authme/util/BukkitService.java +++ b/src/main/java/fr/xephi/authme/util/BukkitService.java @@ -9,6 +9,9 @@ import org.bukkit.OfflinePlayer; import org.bukkit.World; import org.bukkit.entity.Player; import org.bukkit.event.Event; +import org.bukkit.plugin.Plugin; +import org.bukkit.scheduler.BukkitRunnable; +import org.bukkit.scheduler.BukkitScheduler; import org.bukkit.scheduler.BukkitTask; import javax.inject.Inject; @@ -123,6 +126,22 @@ public class BukkitService { return Bukkit.getScheduler().runTaskLaterAsynchronously(authMe, task, delay); } + /** + * Schedules the given task to repeatedly run until cancelled, starting after the + * specified number of server ticks. + * + * @param task the task to schedule + * @param delay the ticks to wait before running the task + * @param period the ticks to wait between runs + * @return a BukkitTask that contains the id number + * @throws IllegalArgumentException if plugin is null + * @throws IllegalStateException if this was already scheduled + * @see BukkitScheduler#runTaskTimer(Plugin, Runnable, long, long) + */ + public BukkitTask runTaskTimer(BukkitRunnable task, long delay, long period) { + return task.runTaskTimer(authMe, period, delay); + } + /** * Broadcast a message to all players. * diff --git a/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java index c709d1f0..01f00b1f 100644 --- a/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java +++ b/src/test/java/fr/xephi/authme/task/purge/PurgeServiceTest.java @@ -4,9 +4,6 @@ import fr.xephi.authme.ReflectionTestUtils; import fr.xephi.authme.TestHelper; import fr.xephi.authme.datasource.DataSource; import fr.xephi.authme.permission.PermissionsManager; -import fr.xephi.authme.runner.BeforeInjecting; -import fr.xephi.authme.runner.DelayedInjectionRunner; -import fr.xephi.authme.runner.InjectDelayed; import fr.xephi.authme.settings.NewSetting; import fr.xephi.authme.settings.properties.PurgeSettings; import fr.xephi.authme.util.BukkitService; @@ -18,10 +15,14 @@ import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; +import org.mockito.InjectMocks; import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +import java.util.Arrays; import java.util.Calendar; import java.util.Collections; +import java.util.List; import java.util.Set; import java.util.UUID; @@ -36,6 +37,7 @@ import static org.mockito.BDDMockito.given; import static org.mockito.Matchers.anyLong; import static org.mockito.Matchers.anySet; import static org.mockito.Matchers.argThat; +import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -44,10 +46,10 @@ import static org.mockito.Mockito.verifyZeroInteractions; /** * Test for {@link PurgeService}. */ -@RunWith(DelayedInjectionRunner.class) +@RunWith(MockitoJUnitRunner.class) public class PurgeServiceTest { - @InjectDelayed + @InjectMocks private PurgeService purgeService; @Mock @@ -66,15 +68,11 @@ public class PurgeServiceTest { TestHelper.setupLogger(); } - @BeforeInjecting - public void initSettingDefaults() { - given(settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER)).willReturn(60); - } - @Test public void shouldNotRunAutoPurge() { // given given(settings.getProperty(PurgeSettings.USE_AUTO_PURGE)).willReturn(false); + given(settings.getProperty(PurgeSettings.DAYS_BEFORE_REMOVE_PLAYER)).willReturn(60); // when purgeService.runAutoPurge(); @@ -112,8 +110,8 @@ public class PurgeServiceTest { ArgumentCaptor captor = ArgumentCaptor.forClass(Long.class); verify(dataSource).getRecordsToPurge(captor.capture()); assertCorrectPurgeTimestamp(captor.getValue(), 60); - assertThat(Boolean.TRUE.equals( - ReflectionTestUtils.getFieldValue(PurgeService.class, purgeService, "isPurging")), equalTo(true)); + assertThat(Boolean.TRUE, equalTo( + ReflectionTestUtils.getFieldValue(PurgeService.class, purgeService, "isPurging"))); verifyScheduledPurgeTask(null, playerNames); } @@ -169,6 +167,19 @@ public class PurgeServiceTest { verifyZeroInteractions(bukkitService, dataSource, permissionsManager); } + @Test + public void shouldExecutePurgeActions() { + // given + List players = Arrays.asList(mockReturnedOfflinePlayers()); + List names = Arrays.asList("alpha", "bravo", "foxtrot"); + + // when + purgeService.executePurge(players, names); + + // then + verify(executor).executePurge(players, names); + } + /** * Returns mock OfflinePlayer objects with names corresponding to A - G of the NATO phonetic alphabet, * in various casing. @@ -198,14 +209,14 @@ public class PurgeServiceTest { } @SuppressWarnings("unchecked") - private void verifyScheduledPurgeTask(UUID uuid, Set names) { + private void verifyScheduledPurgeTask(UUID senderUuid, Set names) { ArgumentCaptor captor = ArgumentCaptor.forClass(PurgeTask.class); - verify(bukkitService).runTaskAsynchronously(captor.capture()); + verify(bukkitService).runTaskTimer(captor.capture(), eq(0L), eq(1L)); PurgeTask task = captor.getValue(); Object senderInTask = ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "sender"); Set namesInTask = (Set) ReflectionTestUtils.getFieldValue(PurgeTask.class, task, "toPurge"); - assertThat(senderInTask, Matchers.equalTo(uuid)); + assertThat(senderInTask, Matchers.equalTo(senderUuid)); assertThat(namesInTask, containsInAnyOrder(names.toArray())); } } diff --git a/src/test/java/fr/xephi/authme/task/purge/PurgeTaskTest.java b/src/test/java/fr/xephi/authme/task/purge/PurgeTaskTest.java new file mode 100644 index 00000000..2168756d --- /dev/null +++ b/src/test/java/fr/xephi/authme/task/purge/PurgeTaskTest.java @@ -0,0 +1,221 @@ +package fr.xephi.authme.task.purge; + +import fr.xephi.authme.ReflectionTestUtils; +import fr.xephi.authme.TestHelper; +import fr.xephi.authme.permission.PermissionNode; +import fr.xephi.authme.permission.PermissionsManager; +import fr.xephi.authme.permission.PlayerStatePermission; +import org.bukkit.Bukkit; +import org.bukkit.OfflinePlayer; +import org.bukkit.Server; +import org.bukkit.command.ConsoleCommandSender; +import org.bukkit.entity.Player; +import org.bukkit.scheduler.BukkitRunnable; +import org.bukkit.scheduler.BukkitScheduler; +import org.junit.BeforeClass; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.runners.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.UUID; + +import static com.google.common.collect.Sets.newHashSet; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.empty; +import static org.junit.Assert.assertThat; +import static org.mockito.BDDMockito.given; +import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; +import static org.mockito.Matchers.argThat; +import static org.mockito.Matchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; + +/** + * Test for {@link PurgeTask}. + */ +@RunWith(MockitoJUnitRunner.class) +public class PurgeTaskTest { + + private static final PermissionNode BYPASS_NODE = PlayerStatePermission.BYPASS_PURGE; + + private Map playerBypassAssignments = new HashMap<>(); + + @Mock + private PermissionsManager permissionsManager; + + @Mock + private PurgeService purgeService; + + @Captor + private ArgumentCaptor> playerCaptor; + + @Captor + private ArgumentCaptor> namesCaptor; + + @BeforeClass + public static void initLogger() { + TestHelper.setupLogger(); + } + + @Test + public void shouldRunTask() { + // given + Set names = + newHashSet("alpha", "bravo", "charlie", "delta", "echo", "foxtrot", "golf", "hotel", "india"); + // alpha and echo have bypass permission + // Foxtrot and india are not present as OfflinePlayer + // Additionally, BOGUS and 123456 are not present in the names list + OfflinePlayer[] players = asArray( + mockOfflinePlayer("Alpha", true), mockOfflinePlayer("BOGUS", false), mockOfflinePlayer("charlie", false), + mockOfflinePlayer("Delta", false), mockOfflinePlayer("BRAVO", false), mockOfflinePlayer("Echo", true), + mockOfflinePlayer("Golf", false), mockOfflinePlayer("123456", false), mockOfflinePlayer("HOTEL", false)); + reset(purgeService, permissionsManager); + setPermissionsBehavior(); + PurgeTask task = new PurgeTask(purgeService, permissionsManager, null, names, players); + + // when (1 - first run, 5 players per run) + task.run(); + + // then (1) + // In the first run, Alpha to BRAVO (see players list above) went through. One of those players is not present + // in the names list, so expect the permission manager to have been called four times + verify(permissionsManager, times(4)).hasPermissionOffline(any(OfflinePlayer.class), eq(BYPASS_NODE)); + // Alpha has the bypass permission, so we expect charlie, Delta and BRAVO to be purged + assertRanPurgeWithPlayers(players[2], players[3], players[4]); + + // when (2) + reset(purgeService, permissionsManager); + setPermissionsBehavior(); + task.run(); + + // then (2) + // Echo, Golf, HOTEL + verify(permissionsManager, times(3)).hasPermissionOffline(any(OfflinePlayer.class), eq(BYPASS_NODE)); + assertRanPurgeWithPlayers(players[6], players[8]); + + // given (3) + // Third round: no more OfflinePlayer objects, but some names remain + reset(purgeService, permissionsManager); + given(permissionsManager.hasPermissionOffline("india", BYPASS_NODE)).willReturn(true); + + // when (3) + task.run(); + + // then (3) + // We no longer have any OfflinePlayers, so lookup of permissions was done with the names + verify(permissionsManager, times(2)).hasPermissionOffline(anyString(), eq(BYPASS_NODE)); + verify(permissionsManager, never()).hasPermissionOffline(any(OfflinePlayer.class), any(PermissionNode.class)); + assertRanPurgeWithNames("foxtrot"); + } + + @Test + public void shouldStopTaskAndInformSenderUponCompletion() { + // given + Set names = newHashSet("name1", "name2"); + Player sender = mock(Player.class); + UUID uuid = UUID.randomUUID(); + given(sender.getUniqueId()).willReturn(uuid); + PurgeTask task = new PurgeTask(purgeService, permissionsManager, sender, names, new OfflinePlayer[0]); + + ReflectionTestUtils.setField(BukkitRunnable.class, task, "taskId", 10049); + Server server = mock(Server.class); + BukkitScheduler scheduler = mock(BukkitScheduler.class); + given(server.getScheduler()).willReturn(scheduler); + ReflectionTestUtils.setField(Bukkit.class, null, "server", server); + given(server.getPlayer(uuid)).willReturn(sender); + + task.run(); // Run for the first time -> results in empty names list + + // when + task.run(); + + // then + verify(scheduler).cancelTask(task.getTaskId()); + verify(sender).sendMessage(argThat(containsString("Database has been purged successfully"))); + } + + @Test + public void shouldStopTaskAndInformConsoleUser() { + // given + Set names = newHashSet("name1", "name2"); + PurgeTask task = new PurgeTask(purgeService, permissionsManager, null, names, new OfflinePlayer[0]); + + ReflectionTestUtils.setField(BukkitRunnable.class, task, "taskId", 10049); + Server server = mock(Server.class); + BukkitScheduler scheduler = mock(BukkitScheduler.class); + given(server.getScheduler()).willReturn(scheduler); + ReflectionTestUtils.setField(Bukkit.class, null, "server", server); + ConsoleCommandSender consoleSender = mock(ConsoleCommandSender.class); + given(server.getConsoleSender()).willReturn(consoleSender); + + task.run(); // Run for the first time -> results in empty names list + + // when + task.run(); + + // then + verify(scheduler).cancelTask(task.getTaskId()); + verify(consoleSender).sendMessage(argThat(containsString("Database has been purged successfully"))); + } + + + private OfflinePlayer mockOfflinePlayer(String name, boolean hasBypassPermission) { + OfflinePlayer player = mock(OfflinePlayer.class); + given(player.getName()).willReturn(name); + playerBypassAssignments.put(player, hasBypassPermission); + return player; + } + + private OfflinePlayer[] asArray(OfflinePlayer... players) { + return players; + } + + private void setPermissionsBehavior() { + given(permissionsManager.hasPermissionOffline(any(OfflinePlayer.class), eq(BYPASS_NODE))) + .willAnswer(new Answer() { + @Override + public Boolean answer(InvocationOnMock invocationOnMock) throws Throwable { + OfflinePlayer player = (OfflinePlayer) invocationOnMock.getArguments()[0]; + Boolean hasPermission = playerBypassAssignments.get(player); + if (hasPermission == null) { + throw new IllegalStateException("Unexpected check of '" + BYPASS_NODE + + "' with player = " + player); + } + return hasPermission; + } + }); + } + + private void assertRanPurgeWithPlayers(OfflinePlayer... players) { + List names = new ArrayList<>(players.length); + for (OfflinePlayer player : players) { + names.add(player.getName()); + } + verify(purgeService).executePurge(playerCaptor.capture(), namesCaptor.capture()); + assertThat(namesCaptor.getValue(), containsInAnyOrder(names.toArray())); + assertThat(playerCaptor.getValue(), containsInAnyOrder(players)); + } + + private void assertRanPurgeWithNames(String... names) { + verify(purgeService).executePurge(playerCaptor.capture(), namesCaptor.capture()); + assertThat(namesCaptor.getValue(), containsInAnyOrder(names)); + assertThat(playerCaptor.getValue(), empty()); + } + +}