NMS independent test toolkit to allow developers to test their commands with MockBukkit

[willkroboth]

Aims to resolve #356.

Currently, when developers try to load the CommandAPI in a MockBukkit environment, it is very hard to get the NMS code accessed by the CommandAPI to cooperate with MockBukkit. There is work on the dev/public-test-suite branch that is trying to make it easier to resolve these issues. As an alternative solution, this PR aims to create a framework for mocking an NMS-independent version of the CommandAPI. Various utilities are also included to make testing commands easier.

These features are accessible using the commandapi-bukkit-test-toolkit module. Developers should declare a test scope dependency on this module before their dependency on the plugin or shaded CommandAPI. This overrides CommandAPIVersionHandler when running tests to load MockCommandAPIBukkit instead, which doesn't rely on any NMS classes.

The class CommandAPITestUtilities provides many static methods for asserting particular behavior when running commands. The class CommandAPIHandlerSpy intercepts command registration in the background to enable verifying the arguments input when running a command without having to make any changes to the plugin code that registers commands.

In order to mock Arguments that usually rely upon NMS ArgumentTypes, we would have to implement the parsing logic ourselves. I've included IntegerRangeArgumentType as a proof of concept. Currently, any unimplemented NMS methods throw an UnimplementedMethodException. I don't think we need to worry about specific implementations for all these methods right now. As developers try to use this framework, the methods they actually use can be implemented first.

I've implemented two example projects that showcase how to set up and use the test toolkit, one that shades the CommandAPI and one that depends on the CommandAPI plugin. This also helps verify that each setup works. To more rigorously test the toolkit's features, additional tests have been added inside the src/test directory of the commandapi-bukkit-test-toolkit module. The result of these tests will appear in the Jacoco coverage report.

I still need to add javadocs and documentation, but I think this is a good start feature-wise. There are obviously many methods that throw an UnimplementedMethodException, but again I think developers can try using this first and show what methods are actually used. Feel free to suggest anything that should be included in an initial snapshot release of the framework though. Also, of course, code review is greatly appreciated!

[willkroboth]

Actually, I thought of one important system that should probably be implemented: default ArgumentType suggestions. The IntegerRangeArgument doesn't suggest anything by default, so I didn't think about it at first. There is a parser system in place to make creating new ArgumentType parsers easier, and I can probably add suggestions to that as well. I think I'll add support for the PlayerArgument, since that provides suggestions by default and is commonly used any way.

[willkroboth]

Alright, ArgumentType suggestions implemented and docs written. After code review, I'm happy to merge this so developers can try it and figure out which important methods I haven't implemented :P.

[DerEchtePilz]

I have absolutely no idea how the Parser class works so I just left it alone for now.

Noticed a lot of Spigot API - is there a reason we can't use Paper?
Also when there were selectors, @n was missing almost every time. Maybe I missed something while reviewing, but for now I marked those places.

[DerEchtePilz]

Is there a reason to use Spigot and not Paper here when Spigot's repository is not listed?
Also, shouldn't we go with the latest version instead of 1.20.6?

[DerEchtePilz]

Hmm, yeah, not sure if this makes too much sense. If anyone uses the test toolkit and comes across this exception, the error message tells you that the method hasn't been implemented.
In addition, the warnings might be a little unnecessary too given that all the unimplemented methods are located under a big comment saying the methods are unimplemented.
I mean, we can also keep this deprecation-as-a-reminder thingy, just wanted to express my opinion about this.

[DerEchtePilz]

I think it might be nice to also list the dependencies for Gradle builds.

[willkroboth]

Also when there were selectors, @n was missing almost every time.

Ah, yeah good catch. I wrote most of this for 1.20.6, and @n was added in 1.21 so I didn't include it. I'll definitely update all the dependencies to match 1.21 and add @n.
Although, I wonder if this framework should provide some way to account for version-specific behavior? Maybe default to latest, but add something similar to CommandAPIVersionHandler#usePlatformImplementation to set a version. Shouldn't be too hard to add that.

Noticed a lot of Spigot API - is there a reason we can't use Paper?

I was originally thinking that since the CommandAPI is supposed to work on Spigot, the testing framework should work on Spigot, so the Spigot repo makes sure the testing framework doesn't accidentally use Paper-only API. However, I now know that MockBukkit is based on Paper API, so we actually end up having access to Paper API transitively. Technically, we could completely not list a dependency for Spigot/Paper and just transitively inherit it from MockBukkit, which might actually be good because MockBukkit depends on the correct version of the classes being loaded.

So yeah, since MockBukkit uses Paper API this testing framework probably should too?

[DerEchtePilz]

As written on Discord yesterday, I have now written up a complete example without using the Parser system as I don't think that system is necessary.

The parsing logic has been adapted from Vanilla code and should basically be a 1:1 copy. This could also be implemented more generally to account for the FloatRangeArgument if/when implementing it.
The changes in the test class were results from ingame tests where the position is before the range input.

I think it would be good to do something similar with the other argument types.

[DerEchtePilz]

Suggested change

	private static final Parser.Argument<Integer> READ_INT_BEFORE_RANGE = reader -> {
	// Custom parser avoids reading `..` indicator for range as part of a number
	int start = reader.getCursor();
	while (reader.canRead()) {
	char c = reader.peek();
	if (!(c == '-' // Negative sign
	|| (c >= '0' && c <= '9') // Digit
	|| c == '.' && !(reader.canRead(2) && reader.peek(1) == '.') // Decimal point, but not `..`
	)) break;
	reader.skip();
	}
	String number = reader.getString().substring(start, reader.getCursor());
	if (number.isEmpty()) {
	reader.setCursor(start);
	throw CommandSyntaxException.BUILT_IN_EXCEPTIONS.readerExpectedInt().createWithContext(reader);
	}
	try {
	return Integer.parseInt(number);
	} catch (NumberFormatException ignored) {
	reader.setCursor(start);
	throw CommandSyntaxException.BUILT_IN_EXCEPTIONS.readerInvalidInt().createWithContext(reader, number);
	}
	};
	private static final Predicate<CommandSyntaxException> THROW_INVALID_INT_EXCEPTIONS = exception ->
	exception.getType().equals(CommandSyntaxException.BUILT_IN_EXCEPTIONS.readerInvalidInt());
	private static final Parser<IntegerRange> PARSER = ArgumentUtilities
	.assertCanRead(EMPTY_INPUT::createWithContext)
	.alwaysThrowException()
	.continueWith(
	Parser.tryParse(ArgumentUtilities.literal("..")
	.neverThrowException()
	// Input ..
	.continueWith(
	Parser.tryParse(Parser.parse(StringReader::readInt)
	// It looks like they tried to enter ..high, but high was not a valid int
	.throwExceptionIfTrue(THROW_INVALID_INT_EXCEPTIONS)
	// Input ..high
	.continueWith(high -> Parser.parse(
	reader -> IntegerRange.integerRangeLessThanOrEq(high.get())
	))
	).then(Parser.parse(
	// Input just ..
	reader -> {
	// Move cursor to start of ..
	reader.setCursor(reader.getCursor() - 2);
	throw EMPTY_INPUT.createWithContext(reader);
	}
	))
	)
	).then(Parser.parse(StringReader::getCursor)
	.alwaysThrowException()
	.continueWith(start ->
	Parser.parse(READ_INT_BEFORE_RANGE)
	.alwaysMapException(exception -> {
	if (exception.getType().equals(CommandSyntaxException.BUILT_IN_EXCEPTIONS.readerExpectedInt())) {
	// If we didn't find any int to input, this range is empty
	StringReader context = new StringReader(exception.getInput());
	context.setCursor(exception.getCursor());
	return EMPTY_INPUT.createWithContext(context);
	}
	// Otherwise throw original exception (invalid integer)
	return exception;
	})
	// Input low
	.continueWith(getLow ->
	Parser.tryParse(ArgumentUtilities.literal("..")
	.neverThrowException()
	// Input low..
	.continueWith(
	Parser.tryParse(Parser.parse(StringReader::readInt)
	.throwExceptionIfTrue(THROW_INVALID_INT_EXCEPTIONS)
	.continueWith(getHigh -> Parser.parse(
	reader -> {
	int low = getLow.get();
	int high = getHigh.get();
	if (low > high) {
	// Reset to start of input
	reader.setCursor(start.get());
	throw RANGE_SWAPPED.createWithContext(reader);
	}
	return new IntegerRange(low, high);
	}
	))
	).then(Parser.parse(
	// Input low..
	reader -> IntegerRange.integerRangeGreaterThanOrEq(getLow.get())
	))
	)
	).then(Parser.parse(
	// Input exact
	reader -> {
	int exact = getLow.get();
	return new IntegerRange(exact, exact);
	}
	))
	)
	)
	)
	);

[DerEchtePilz]

Suggested change

	return PARSER.parse(reader);
	if (!reader.canRead()) {
	throw EMPTY_INPUT.createWithContext(reader);
	}
	int start = reader.getCursor();
	try {
	Result<Integer> maybeLow = Result.withValue(readNumber(reader));
	Result<Integer> maybeHigh;
	if (reader.canRead(2) && (reader.peek() == '.' && reader.peek(1) == '.')) {
	reader.skip();
	reader.skip();
	maybeHigh = Result.withValue(readNumber(reader));
	if (maybeLow.getValue() == null && maybeHigh.getValue() == null) {
	throw EMPTY_INPUT.createWithContext(reader);
	}
	} else {
	maybeHigh = maybeLow;
	}
	if (maybeLow.getValue() == null && maybeHigh.getValue() == null) {
	throw EMPTY_INPUT.createWithContext(reader);
	} else {
	int low = maybeLow.getValue() == null ? Integer.MIN_VALUE : maybeLow.getValue();
	int high = maybeHigh.getValue() == null ? Integer.MAX_VALUE : maybeHigh.getValue();
	if (low > high) {
	throw RANGE_SWAPPED.createWithContext(reader);
	}
	return new IntegerRange(low, high);
	}
	} catch (CommandSyntaxException e) {
	reader.setCursor(start);
	throw new CommandSyntaxException(e.getType(), e.getRawMessage(), e.getInput(), start);
	}

[DerEchtePilz]

Suggested change

	}
	}
	private Integer readNumber(StringReader reader) throws CommandSyntaxException {
	int cursor = reader.getCursor();
	while (reader.canRead() && isAllowedNumber(reader)) {
	reader.skip();
	}
	String number = reader.getString().substring(cursor, reader.getCursor());
	if (number.isEmpty()) {
	return null;
	} else {
	try {
	return Integer.parseInt(number);
	} catch (NumberFormatException e) {
	throw CommandSyntaxException.BUILT_IN_EXCEPTIONS.readerInvalidInt().createWithContext(reader, number);
	}
	}
	}
	private boolean isAllowedNumber(StringReader reader) {
	char c = reader.peek();
	return (c >= '0' && c <= '9')
	|| (c == '-')
	|| (c == '.')
	&& (!reader.canRead(2) || reader.peek(1) != '.');
	}

[DerEchtePilz]

Suggested change

	}
	}
	public T getValue() {
	return value;
	}
	public CommandSyntaxException getException() {
	return exception;
	}

[DerEchtePilz]

Suggested change

	"Invalid integer '1.0' at position 8: test 0..<--[HERE]"
	);
	assertCommandFails(
	player, "test ..1.0",
	"Invalid integer '1.0' at position 7: test ..<--[HERE]"
	"Invalid integer '1.0' at position 5: test <--[HERE]"
	);
	assertCommandFails(
	player, "test ..1.0",
	"Invalid integer '1.0' at position 5: test <--[HERE]"