Test CommandManager

proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java

@@ -46,6 +46,7 @@ import net.kyori.adventure.identity.Identity;
 import net.kyori.adventure.text.Component;
 import net.kyori.adventure.text.format.NamedTextColor;
 import org.checkerframework.checker.lock.qual.GuardedBy;
+import org.jetbrains.annotations.VisibleForTesting;
 
 public class VelocityCommandManager implements CommandManager {
 
@@ -238,9 +239,9 @@ public class VelocityCommandManager implements CommandManager {
     return dispatcher.getRoot().getChild(alias.toLowerCase(Locale.ENGLISH)) != null;
   }
 
-  public CommandDispatcher<CommandSource> getDispatcher() {
+  @VisibleForTesting // this constitutes unsafe publication
-    // TODO Can we remove this? This is only used by tests, and constitutes unsafe publication.
+  RootCommandNode<CommandSource> getRoot() {
-    return dispatcher;
+    return dispatcher.getRoot();
   }
 
   public CommandGraphInjector<CommandSource> getInjector() {

proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java

@@ -45,7 +45,7 @@ public class CommandGraphInjectorTests {
 
   @BeforeEach
   void setUp() {
-    this.manager = new VelocityCommandManager(OldCommandManagerTests.EVENT_MANAGER);
+    this.manager = CommandManagerTests.newManager();
     this.dest = new RootCommandNode<>();
   }
 
@@ -56,7 +56,7 @@ public class CommandGraphInjectorTests {
     manager.getInjector().inject(dest, SOURCE);
 
     // Preserves alias and arguments node
-    final var expected = manager.getDispatcher().getRoot();
+    final var expected = manager.getRoot();
     assertEquals(expected, dest);
   }
 

proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java

@@ -0,0 +1,179 @@
+package com.velocitypowered.proxy.command;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.api.Assertions.fail;
+
+import com.mojang.brigadier.builder.LiteralArgumentBuilder;
+import com.velocitypowered.api.command.BrigadierCommand;
+import com.velocitypowered.api.command.CommandSource;
+import com.velocitypowered.api.command.RawCommand;
+import com.velocitypowered.api.command.SimpleCommand;
+import com.velocitypowered.proxy.event.MockEventManager;
+import com.velocitypowered.proxy.event.VelocityEventManager;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+public class CommandManagerTests {
+
+  static final VelocityEventManager EVENT_MANAGER = new MockEventManager();
+
+  static {
+    Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+      try {
+        EVENT_MANAGER.shutdown();
+      } catch (InterruptedException e) {
+        Thread.currentThread().interrupt();
+      }
+    }));
+  }
+
+  static VelocityCommandManager newManager() {
+    return new VelocityCommandManager(EVENT_MANAGER);
+  }
+
+  private VelocityCommandManager manager;
+
+  @BeforeEach
+  void setUp() {
+    this.manager = newManager();
+  }
+
+  // Registration
+
+  @Test
+  void testRegisterWithMeta() {
+    final var meta = manager.metaBuilder("hello").build();
+    manager.register(meta, DummyCommand.INSTANCE);
+
+    assertTrue(manager.hasCommand("hello"));
+  }
+
+  @Test
+  void testRegisterWithMetaContainingMultipleAliases() {
+    final var meta = manager.metaBuilder("foo")
+            .aliases("bar")
+            .aliases("baz", "qux")
+            .build();
+    manager.register(meta, DummyCommand.INSTANCE);
+
+    assertTrue(manager.hasCommand("foo"));
+    assertTrue(manager.hasCommand("bar"));
+    assertTrue(manager.hasCommand("baz"));
+    assertTrue(manager.hasCommand("qux"));
+  }
+
+  @Test
+  void testRegisterAliasesAreCaseInsensitive() {
+    final var meta = manager.metaBuilder("Foo")
+            .aliases("Bar")
+            .build();
+    manager.register(meta, DummyCommand.INSTANCE);
+
+    assertTrue(manager.hasCommand("foo"));
+    assertTrue(manager.hasCommand("bar"));
+  }
+
+  @Test
+  void testRegisterBrigadierCommand() {
+    final var node = LiteralArgumentBuilder
+            .<CommandSource>literal("hello")
+            .build();
+    manager.register(new BrigadierCommand(node));
+
+    assertTrue(manager.hasCommand("hello"));
+  }
+
+  @Test
+  void testRegisterOverridesPreviousCommand() {
+    final var called = new AtomicBoolean();
+
+    final var oldMeta = manager.metaBuilder("foo").build();
+    manager.register(oldMeta, DummyCommand.INSTANCE); // fails on execution
+    final var newMeta = manager.metaBuilder("foo").build();
+    manager.register("foo", (RawCommand) invocation -> called.set(true));
+    manager.executeAsync(MockCommandSource.INSTANCE, "foo").join();
+
+    assertTrue(called.get());
+  }
+
+  @Test
+  void testAddingExecutableHintToMetaThrows() {
+    final var hintNode = LiteralArgumentBuilder
+            .<CommandSource>literal("hint")
+            .executes(context -> fail())
+            .build();
+
+    assertThrows(IllegalArgumentException.class, () -> {
+      manager.metaBuilder("hello").hint(hintNode);
+    });
+  }
+
+  @Test
+  void testAddingHintWithRedirectToMetaThrows() {
+    final var targetNode = LiteralArgumentBuilder
+            .<CommandSource>literal("target")
+            .build();
+    final var hintNode = LiteralArgumentBuilder
+            .<CommandSource>literal("origin")
+            .redirect(targetNode)
+            .build();
+
+    assertThrows(IllegalArgumentException.class, () -> {
+      manager.metaBuilder("hello").hint(hintNode);
+    });
+  }
+
+  // Un-registration
+
+
+  @Test
+  void testUnregisterUnregisteredAliasIsIgnored() {
+    manager.unregister("hello");
+
+    assertFalse(manager.hasCommand("hello"));
+  }
+
+  @Test
+  void testUnregisterRegisteredAlias() {
+    manager.register("hello", DummyCommand.INSTANCE);
+    manager.unregister("hello");
+
+    assertFalse(manager.hasCommand("hello"));
+  }
+
+  @Test
+  void testUnregisterSecondaryAlias() {
+    final var meta = manager.metaBuilder("foo")
+            .aliases("bar")
+            .build();
+    manager.register(meta, DummyCommand.INSTANCE);
+    manager.unregister("bar");
+
+    assertFalse(manager.hasCommand("bar"));
+    assertTrue(manager.hasCommand("foo"));
+  }
+
+  static class DummyCommand implements SimpleCommand {
+
+    static final DummyCommand INSTANCE = new DummyCommand();
+
+    @Override
+    public void execute(final Invocation invocation) {
+      fail();
+    }
+
+    @Override
+    public List<String> suggest(final Invocation invocation) {
+      return fail();
+    }
+
+    @Override
+    public boolean hasPermission(final Invocation invocation) {
+      return fail();
+    }
+  }
+}

proxy/src/test/java/com/velocitypowered/proxy/command/CommandTestSuite.java

@@ -15,7 +15,7 @@ abstract class CommandTestSuite {
 
   @BeforeEach
   void setUp() {
-    this.manager = new VelocityCommandManager(OldCommandManagerTests.EVENT_MANAGER);
+    this.manager = CommandManagerTests.newManager();
   }
 
   final void assertHandled(final String input) {

proxy/src/test/java/com/velocitypowered/proxy/command/OldCommandManagerTests.java

@@ -49,27 +49,15 @@ import org.junit.jupiter.api.Test;
 @Disabled
 public class OldCommandManagerTests {
 
-  public static final VelocityEventManager EVENT_MANAGER = new MockEventManager();
-
-  static {
-    Runtime.getRuntime().addShutdownHook(new Thread(() -> {
-      try {
-        EVENT_MANAGER.shutdown();
-      } catch (InterruptedException e) {
-        Thread.currentThread().interrupt();
-      }
-    }));
-  }
-
   static VelocityCommandManager createManager() {
-    return new VelocityCommandManager(EVENT_MANAGER);
+    return new VelocityCommandManager(CommandManagerTests.EVENT_MANAGER);
   }
 
   @Test
   void testConstruction() {
     VelocityCommandManager manager = createManager();
     assertFalse(manager.hasCommand("foo"));
-    assertTrue(manager.getDispatcher().getRoot().getChildren().isEmpty());
+    assertTrue(manager.getRoot().getChildren().isEmpty());
     assertFalse(manager.executeAsync(MockCommandSource.INSTANCE, "foo").join());
     assertFalse(manager.executeImmediatelyAsync(MockCommandSource.INSTANCE, "bar").join());
     assertTrue(manager.offerSuggestions(MockCommandSource.INSTANCE, "").join().isEmpty());