From 7c652feac2d65fbd036d0b4d268f33ffca823cd2 Mon Sep 17 00:00:00 2001 From: ljacqu Date: Sun, 29 Nov 2015 10:56:01 +0100 Subject: [PATCH] Fix failing command tests - Detailed description missing period - Fix twice defined /authme delete - Add executable command to HelpSyntaxHelperTest initializations - Remove unneeded constructors in CommandDescription --- .../authme/command/CommandDescription.java | 32 --------- .../xephi/authme/command/CommandManager.java | 4 +- .../authme/command/CommandManagerTest.java | 70 ++++++++++++++++--- .../command/help/HelpSyntaxHelperTest.java | 11 ++- 4 files changed, 65 insertions(+), 52 deletions(-) diff --git a/src/main/java/fr/xephi/authme/command/CommandDescription.java b/src/main/java/fr/xephi/authme/command/CommandDescription.java index fc5a7e4c..3a534246 100644 --- a/src/main/java/fr/xephi/authme/command/CommandDescription.java +++ b/src/main/java/fr/xephi/authme/command/CommandDescription.java @@ -59,19 +59,6 @@ public class CommandDescription { */ private CommandPermissions permissions; - /** - * Constructor. - * - * @param executableCommand The executable command, or null. - * @param label Command label. - * @param description Command description. - * @param detailedDescription Detailed comment description. - * @param parent Parent command. - */ - public CommandDescription(ExecutableCommand executableCommand, String label, String description, String detailedDescription, CommandDescription parent) { - this(executableCommand, label, description, parent, detailedDescription, null); - } - /** * Constructor. * @@ -85,25 +72,6 @@ public class CommandDescription { this(executableCommand, labels, description, detailedDescription, parent, null); } - /** - * Constructor. - * - * @param executableCommand The executable command, or null. - * @param label Command label. - * @param description Command description. - * @param parent Parent command. - * @param detailedDescription Detailed comment description. - * @param arguments Command arguments. - */ - public CommandDescription(ExecutableCommand executableCommand, String label, String description, CommandDescription parent, String detailedDescription, List arguments) { - setExecutableCommand(executableCommand); - this.labels = Collections.singletonList(label); - this.description = description; - this.detailedDescription = detailedDescription; - setParent(parent); - setArguments(arguments); - } - /** * Constructor. * diff --git a/src/main/java/fr/xephi/authme/command/CommandManager.java b/src/main/java/fr/xephi/authme/command/CommandManager.java index 08310a7d..cc335561 100644 --- a/src/main/java/fr/xephi/authme/command/CommandManager.java +++ b/src/main/java/fr/xephi/authme/command/CommandManager.java @@ -81,7 +81,7 @@ public class CommandManager { // Register the unregister command CommandDescription unregisterCommand = CommandDescription.builder() .executableCommand(new UnregisterCommand()) - .labels("unregister", "unreg", "unr", "delete", "del") + .labels("unregister", "unreg", "unr") .description("Unregister a player") .detailedDescription("Unregister the specified player.") .parent(authMeBaseCommand) @@ -117,7 +117,7 @@ public class CommandManager { .executableCommand(new LastLoginCommand()) .labels("lastlogin", "ll") .description("Player's last login") - .detailedDescription("View the date of the specified players last login") + .detailedDescription("View the date of the specified players last login.") .parent(authMeBaseCommand) .permissions(OP_ONLY, AdminPermission.LAST_LOGIN) .withArgument("player", "Player name", true) diff --git a/src/test/java/fr/xephi/authme/command/CommandManagerTest.java b/src/test/java/fr/xephi/authme/command/CommandManagerTest.java index cba6ccb9..14b725f1 100644 --- a/src/test/java/fr/xephi/authme/command/CommandManagerTest.java +++ b/src/test/java/fr/xephi/authme/command/CommandManagerTest.java @@ -2,7 +2,6 @@ package fr.xephi.authme.command; import fr.xephi.authme.util.StringUtils; import org.junit.BeforeClass; -import org.junit.Ignore; import org.junit.Test; import java.util.ArrayList; @@ -16,9 +15,10 @@ import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.nullValue; import static org.junit.Assert.assertThat; +import static org.junit.Assert.fail; /** - * Test for {@link CommandManager}. + * Test for {@link CommandManager}, especially to guarantee the integrity of the defined commands. */ public class CommandManagerTest { @@ -77,7 +77,7 @@ public class CommandManagerTest { commandMappings.addAll(newMappings); // Set only contains unique entries, so we just check after adding all new mappings that the size // of the Set corresponds to our expectation - assertThat("All bindings are unique for command with bindings '" + command.getLabels() + "'", + assertThat("All bindings are unique for command with bindings '" + newMappings + "'", commandMappings.size() == initialSize + newMappings.size(), equalTo(true)); } }; @@ -91,18 +91,21 @@ public class CommandManagerTest { * detailed description should be longer and end with a period. */ @Test - public void shouldHaveDescription() { + public void shouldHaveProperDescription() { // given BiConsumer descriptionTester = new BiConsumer() { @Override public void accept(CommandDescription command, int depth) { - assertThat("has description", StringUtils.isEmpty(command.getDescription()), equalTo(false)); - assertThat("short description doesn't end in '.'", command.getDescription().endsWith("."), - equalTo(false)); - assertThat("has detailed description", StringUtils.isEmpty(command.getDetailedDescription()), - equalTo(false)); - assertThat("detailed description ends in '.'", command.getDetailedDescription().endsWith("."), - equalTo(true)); + String forCommandText = " for command with labels '" + command.getLabels() + "'"; + + assertThat("has description" + forCommandText, + StringUtils.isEmpty(command.getDescription()), equalTo(false)); + assertThat("short description doesn't end in '.'" + forCommandText, + command.getDescription().endsWith("."), equalTo(false)); + assertThat("has detailed description" + forCommandText, + StringUtils.isEmpty(command.getDetailedDescription()), equalTo(false)); + assertThat("detailed description ends in '.'" + forCommandText, + command.getDetailedDescription().endsWith("."), equalTo(true)); } }; @@ -142,6 +145,51 @@ public class CommandManagerTest { walkThroughCommands(commands, descriptionTester); } + @Test + public void shouldHaveOptionalArgumentsAfterMandatoryOnes() { + // given + BiConsumer argumentOrderTester = new BiConsumer() { + @Override + public void accept(CommandDescription command, int depth) { + boolean encounteredOptionalArg = false; + for (CommandArgumentDescription argument : command.getArguments()) { + if (argument.isOptional()) { + encounteredOptionalArg = true; + } else if (!argument.isOptional() && encounteredOptionalArg) { + fail("Mandatory arguments should come before optional ones for command with labels '" + + command.getLabels() + "'"); + } + } + } + }; + + // when/then + walkThroughCommands(manager.getCommandDescriptions(), argumentOrderTester); + } + + /** + * Ensure that a command with children (i.e. a base command) doesn't define any arguments. This might otherwise + * clash with the label of the child. + */ + @Test + public void shouldNotHaveArgumentsIfCommandHasChildren() { + // given + BiConsumer noArgumentForParentChecker = new BiConsumer() { + @Override + public void accept(CommandDescription command, int depth) { + // Fail if the command has children and has arguments at the same time + // Exception: If the parent only has one child defining the help label, it is acceptable + if (command.hasChildren() && command.hasArguments() + && (command.getChildren().size() != 1 || !command.getChildren().get(0).hasLabel("help"))) { + fail("Parent command (labels='" + command.getLabels() + "') should not have any arguments"); + } + } + }; + + // when/then + walkThroughCommands(manager.getCommandDescriptions(), noArgumentForParentChecker); + } + // ------------ // Helper methods diff --git a/src/test/java/fr/xephi/authme/command/help/HelpSyntaxHelperTest.java b/src/test/java/fr/xephi/authme/command/help/HelpSyntaxHelperTest.java index ef280033..1c3b4514 100644 --- a/src/test/java/fr/xephi/authme/command/help/HelpSyntaxHelperTest.java +++ b/src/test/java/fr/xephi/authme/command/help/HelpSyntaxHelperTest.java @@ -1,17 +1,12 @@ package fr.xephi.authme.command.help; -import fr.xephi.authme.command.CommandArgumentDescription; import fr.xephi.authme.command.CommandDescription; import fr.xephi.authme.command.CommandParts; -import fr.xephi.authme.command.executable.authme.AuthMeCommand; +import fr.xephi.authme.command.ExecutableCommand; import fr.xephi.authme.command.executable.authme.RegisterCommand; import org.junit.Test; import org.mockito.Mockito; -import java.util.ArrayList; -import java.util.Arrays; - -import static java.util.Collections.singletonList; import static org.bukkit.ChatColor.BOLD; import static org.bukkit.ChatColor.ITALIC; import static org.bukkit.ChatColor.WHITE; @@ -142,6 +137,7 @@ public class HelpSyntaxHelperTest { .description("Base command") .detailedDescription("AuthMe base command") .parent(null) + .executableCommand(Mockito.mock(ExecutableCommand.class)) .build(); return CommandDescription.builder() @@ -149,6 +145,7 @@ public class HelpSyntaxHelperTest { .labels("register", "r") .description("Register a player") .detailedDescription("Register the specified player with the specified password.") - .parent(base); + .parent(base) + .executableCommand(Mockito.mock(ExecutableCommand.class)); } }