Fix failing command tests

- Detailed description missing period
- Fix twice defined /authme delete
- Add executable command to HelpSyntaxHelperTest initializations
- Remove unneeded constructors in CommandDescription

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<CommandArgumentDescription> arguments) {
-        setExecutableCommand(executableCommand);
-        this.labels = Collections.singletonList(label);
-        this.description = description;
-        this.detailedDescription = detailedDescription;
-        setParent(parent);
-        setArguments(arguments);
-    }
-
     /**
      * Constructor.
      *

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)

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));
+                String forCommandText = " for command with labels '" + command.getLabels() + "'";
-                assertThat("short description doesn't end in '.'", command.getDescription().endsWith("."),
+
-                    equalTo(false));
+                assertThat("has description" + forCommandText,
-                assertThat("has detailed description", StringUtils.isEmpty(command.getDetailedDescription()),
+                    StringUtils.isEmpty(command.getDescription()), equalTo(false));
-                    equalTo(false));
+                assertThat("short description doesn't end in '.'" + forCommandText,
-                assertThat("detailed description ends in '.'", command.getDetailedDescription().endsWith("."),
+                    command.getDescription().endsWith("."), equalTo(false));
-                    equalTo(true));
+                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

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));
     }
 }