diff --git a/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java b/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java index 5d4665a45..6332c51f1 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/command/VelocityCommandManager.java +++ b/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 getDispatcher() { - // TODO Can we remove this? This is only used by tests, and constitutes unsafe publication. - return dispatcher; + @VisibleForTesting // this constitutes unsafe publication + RootCommandNode getRoot() { + return dispatcher.getRoot(); } public CommandGraphInjector getInjector() { diff --git a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java index b1471009e..d0c3a9414 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandGraphInjectorTests.java +++ b/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); } diff --git a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandManagerTests.java new file mode 100644 index 000000000..0d6570de4 --- /dev/null +++ b/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 + .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 + .literal("hint") + .executes(context -> fail()) + .build(); + + assertThrows(IllegalArgumentException.class, () -> { + manager.metaBuilder("hello").hint(hintNode); + }); + } + + @Test + void testAddingHintWithRedirectToMetaThrows() { + final var targetNode = LiteralArgumentBuilder + .literal("target") + .build(); + final var hintNode = LiteralArgumentBuilder + .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 suggest(final Invocation invocation) { + return fail(); + } + + @Override + public boolean hasPermission(final Invocation invocation) { + return fail(); + } + } +} diff --git a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandTestSuite.java b/proxy/src/test/java/com/velocitypowered/proxy/command/CommandTestSuite.java index ccbdb798b..ff7748905 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/command/CommandTestSuite.java +++ b/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) { diff --git a/proxy/src/test/java/com/velocitypowered/proxy/command/OldCommandManagerTests.java b/proxy/src/test/java/com/velocitypowered/proxy/command/OldCommandManagerTests.java index 2835c6f7f..76202c833 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/command/OldCommandManagerTests.java +++ b/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());