-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fabric Game Test API #1622
Fabric Game Test API #1622
Conversation
Pushed what ive done for now, it should build and run. I think im going to move over to using custom entrypoints to register the test suites, thats for another day though 👍 |
@Inject(method = "main", cancellable = true, locals = LocalCapture.CAPTURE_FAILHARD, at = @At(value = "INVOKE", target = "Lnet/minecraft/world/level/storage/LevelStorage$Session;readLevelProperties(Lcom/mojang/serialization/DynamicOps;Lnet/minecraft/resource/DataPackSettings;)Lnet/minecraft/world/SaveProperties;")) | ||
private static void main(String[] args, CallbackInfo info, OptionParser optionParser, OptionSpec optionSpec, OptionSpec optionSpec2, OptionSpec optionSpec3, OptionSpec optionSpec4, OptionSpec optionSpec5, OptionSpec optionSpec6, OptionSpec optionSpec7, OptionSpec optionSpec8, OptionSpec optionSpec9, OptionSpec optionSpec10, OptionSpec optionSpec11, OptionSpec optionSpec12, OptionSpec optionSpec13, OptionSpec optionSpec14, OptionSet optionSet, DynamicRegistryManager.Impl impl, Path path, ServerPropertiesLoader serverPropertiesLoader, Path path2, EulaReader eulaReader, File file, YggdrasilAuthenticationService yggdrasilAuthenticationService, MinecraftSessionService minecraftSessionService, GameProfileRepository gameProfileRepository, UserCache userCache, String string, LevelStorage levelStorage, LevelStorage.Session session, LevelSummary levelSummary, DataPackSettings dataPackSettings, boolean bl, ResourcePackManager resourcePackManager, DataPackSettings dataPackSettings2, CompletableFuture completableFuture, ServerResourceManager serverResourceManager, RegistryOps registryOps) { | ||
if (FabricGameTestHelperImpl.ENABLED) { | ||
FabricGameTestHelperImpl.runHeadlessServer(session, resourcePackManager, serverResourceManager, impl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a side note, we should explore using custom game providers in the loader in the future for such purposes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving to another game provider opens a huge rabbit hole that I dont want to go into, this is a much neater solution imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it, it might not be as much as I initally thought but loader needs quite a bit of work to allow extending the minecraft game provider from a mod and with a stable api that we could trust to depend on in fab api.
The current way is basically the same for modders but doesnt require massive amounts of work to create stable apis in loader. It will also be easier to run in a prod server. Doing it that was has little to nothing to gain, but a huge amount of added complexitly.
Marking as low priority as this doesn't affect most game users or modders and there are other companion tasks as well, namely moving the test invocation from mixin on server main to a custom loader game provider. |
...ic-gametest-api-v1/src/main/java/net/fabricmc/fabric/impl/gametest/FabricGameTestHelper.java
Outdated
Show resolved
Hide resolved
...est-api-v1/src/main/java/net/fabricmc/fabric/impl/gametest/FabricGameTestModInitializer.java
Show resolved
Hide resolved
...c-gametest-api-v1/src/main/java/net/fabricmc/fabric/mixin/gametest/MinecraftServerMixin.java
Outdated
Show resolved
Hide resolved
public abstract class TestServerMixin { | ||
@Inject(method = "isDedicated", at = @At("HEAD"), cancellable = true) | ||
public void isDedicated(CallbackInfoReturnable<Boolean> cir) { | ||
cir.setReturnValue(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget as I wrote this months ago :D I will check and get back to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is used to allow dedicated server commands to be registered. I think when I first wrote this I was running with all the fabric test mods and it would cause the server test to fail. Seems reasonable to leave enabled, I will add a comment.
LOGGER.info("Starting test server"); | ||
MinecraftServer server = TestServer.startServer(thread -> { | ||
TestServer testServer = new TestServer(thread, session, resourcePackManager, serverResourceManager, getBatches(), BlockPos.ORIGIN, registryManager); | ||
FabricLoader.INSTANCE.setGameInstance(testServer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Humm, just noticed this is using the internal loader api to do this, might need checking with 0.12 first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to break, going to back this out of last call for now untill a solution is found.
Out of curiosity, does this support the vanilla |
Yes, those work just fine. |
fabric-gametest-api-v1/src/main/java/net/fabricmc/fabric/mixin/gametest/TestFunctionsMixin.java
Outdated
Show resolved
Hide resolved
public abstract class StructureTestUtilMixin { | ||
private static final String GAMETEST_STRUCTURE_PATH = "gametest/structures/"; | ||
|
||
// Replace the default test structure loading with something that works a bit better for mods. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the normal structure path here would allow modders to use normal structures (eg. from their mods) in game tests. I wonder if that's something we want?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test structures are all loaded from snbt files, and not the normal structure files. Personally I dont see the need to support that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GameTest automatically attempts to load data structures from data packs when it can't find a snbt structure in the gametest structures folder. This folder is just to make the custom test structures more easily version-controllable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, realized I overlooked something here, ignore my comment. I thought this was changing the default gameteststructures
folder but apparently not.
@@ -29,6 +30,9 @@ public void diamond(TestContext context) { | |||
context.addInstantFinalTask(() -> | |||
context.checkBlock(new BlockPos(0, 2, 0), (block) -> block == Blocks.DIAMOND_BLOCK, "Expect block to be diamond") | |||
); | |||
|
|||
// This should be null for the game tests | |||
assert FabricLoader.getInstance().getGameInstance() == null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't assert
useless by default? It should probably be if (FabricLoader.getInstance().getGameInstance() != null) throw new AssertionError();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might just remove this, was just a quick test tbh;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just add -ea
for vm args for test mod runs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's enabled for the regular test
task too.
...entity-events-v1/src/main/java/net/fabricmc/fabric/mixin/entity/event/LivingEntityMixin.java
Outdated
Show resolved
Hide resolved
Add mixin to exit with a non-zero exit code in case the test server fails to start.
Enable JUnit XML report generation
|
||
if (reportPath != null) { | ||
try { | ||
TestFailureLogger.setCompletionListener(new XmlReportingTestCompletionListener(new File(reportPath))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This XmlReportingTestCompletionListener
generates a JUnit-like XML file, but it's seen as invalid by the EnricoMi/publish-unit-test-result-action@v1
GitHub Action (the easiest action to upload test results). Noticed this when testing with the GameTest framework in a custom mod (that adds a similar API), and wrote this workaround (in Mojang mappings):
https://github.com/FoxShadew/JustEnoughDebugTools/blob/master/src/main/java/net/shadew/debug/test/ProperJUnitLikeTestReporter.java
context.addInstantFinalTask(() -> | ||
context.checkBlock(new BlockPos(0, 2, 0), (block) -> block == Blocks.DIAMOND_BLOCK, "Expect block to be diamond") | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As basic test structure I used a sand block that fell down, and asserted it being on the ground. Might be an inspiration for some more tests, but I see that for the sake of testing the testing api the tests themself don't really matter anyway. Just visually, the sand block test shows a visual feedback as soon as the block has landed.
This API is more or less the same as what I implemented as part of a debugging mod/API I work on: Just Enough Debug Tools (JEDT), see most certainly this package. I wanna propose a couple of features:
Not intending to advertise anything, but I recommend looking at the test part of JEDT for some comparison, which implements a fairly similar API for using the GameTest system. See these packages:
And some usages:
I'll likely keep my testing API in JEDT for a while even after this PR gets merged, just to evaluate the differences. |
Please feel free to ask questions, ill be happy to try and explain my approach, might even find a better way of doing something 👍
Yes, I can see that beign useful, I think its out of scope for this first version but something we can look at doing later. Expanding the intergration with existing test tools such gradle, junit or IDE's is definally something that I want to look into more, again this seems like something after this first version. I have had a quick look at your JEDT project, it looks good. I think it will still be useful as a few parts seem beyond fabric's scope. |
# Conflicts: # build.gradle # fabric-entity-events-v1/build.gradle
Sorry, but I don't understand what this PR is for. Could someone explain it? |
@ExoPlant this PR exposes the new* game testing framework to modders, this will allow modders to write automated tests for their mods. https://www.youtube.com/watch?v=vXaWOJTCYNg is a good primer on what this is, and why and how mojang use it. |
* First proof of concept pass * Cleanup and fixes. * Checkstyle * Fix running. * Updated * Fix build * Cleanup + fixes. * Fix package * and test package * game-test -> gametest * Fix exclusion * Review feedback and fixes. * Remove comment * Review feedback. * Don't set the game instance * Fix * Work around shadowed fields from super classes not getting remapped... dejavu anyone? See: FabricMC@c55516d#diff-0956caa3cd38a54f5910979f0cfd98198a93e4d585e111300f2f7ab7301ad122 * Add mixin to exit with a non-zero exit code in case the test server fails to start. * Enable JUnit XML report generation. Co-authored-by: Sebastian Hartte <[email protected]>
This example has 2 tests, the first one uses a structure that can be manually built and exported using the test command. The 2nd test uses an empty structure provided via the api, this allows you to write code only tests.
invokeTestMethod
can be used to customise and extend the test method invocation, useful for before/after methods or having a custom context.These tests can easily be ran headless via gradle:
Registered via entrypoints like so: