From 905d4cb091e28644c2ae29906ba7aab81fcae6a4 Mon Sep 17 00:00:00 2001 From: Jonathan Coates Date: Sun, 8 Oct 2023 15:21:33 +0100 Subject: [PATCH] Fix crash when joining a dedicated server We can't use FriendlyByte.readCollection to read to a pre-allocated/array-backed NonNullList, as that doesn't implement List.add. Instead, we just need to do a normal loop. We add a couple of tests to round-trip our recipe specs. Unfortunately we can't test the recipes themselves as our own registries aren't set up, so this'll have to do for now. --- projects/common/build.gradle.kts | 2 + .../shared/recipe/RecipeUtil.java | 5 +- .../shared/recipe/RecipeArbitraries.java | 50 +++++++++++++++++ .../shared/recipe/RecipeEqualities.java | 34 +++++++++++ .../shared/recipe/ShapedRecipeSpecTest.java | 32 +++++++++++ .../recipe/ShapelessRecipeSpecTest.java | 32 +++++++++++ .../upgrades/TurtleToolSerialiserTest.java | 31 +++------- .../test/shared/MinecraftArbitraries.java | 5 ++ .../test/shared/MinecraftEqualities.java | 39 +++++++++++++ .../test/shared/NetworkSupport.java | 56 +++++++++++++++++++ .../test/core/StructuralEqualities.java | 23 ++++++++ .../test/core/StructuralEquality.java | 9 +++ projects/fabric/build.gradle.kts | 2 + projects/forge/build.gradle.kts | 2 + 14 files changed, 297 insertions(+), 25 deletions(-) create mode 100644 projects/common/src/test/java/dan200/computercraft/shared/recipe/RecipeArbitraries.java create mode 100644 projects/common/src/test/java/dan200/computercraft/shared/recipe/RecipeEqualities.java create mode 100644 projects/common/src/test/java/dan200/computercraft/shared/recipe/ShapedRecipeSpecTest.java create mode 100644 projects/common/src/test/java/dan200/computercraft/shared/recipe/ShapelessRecipeSpecTest.java create mode 100644 projects/common/src/testFixtures/java/dan200/computercraft/test/shared/MinecraftEqualities.java create mode 100644 projects/common/src/testFixtures/java/dan200/computercraft/test/shared/NetworkSupport.java diff --git a/projects/common/build.gradle.kts b/projects/common/build.gradle.kts index b9d8fa399..69be0f26c 100644 --- a/projects/common/build.gradle.kts +++ b/projects/common/build.gradle.kts @@ -39,4 +39,6 @@ dependencies { testModImplementation(testFixtures(project(":core"))) testModImplementation(testFixtures(project(":common"))) testModImplementation(libs.bundles.kotlin) + + testFixturesImplementation(testFixtures(project(":core"))) } diff --git a/projects/common/src/main/java/dan200/computercraft/shared/recipe/RecipeUtil.java b/projects/common/src/main/java/dan200/computercraft/shared/recipe/RecipeUtil.java index 482d1ea16..83ad4c44b 100644 --- a/projects/common/src/main/java/dan200/computercraft/shared/recipe/RecipeUtil.java +++ b/projects/common/src/main/java/dan200/computercraft/shared/recipe/RecipeUtil.java @@ -21,7 +21,10 @@ public final class RecipeUtil { } public static NonNullList readIngredients(FriendlyByteBuf buffer) { - return buffer.readCollection(x -> NonNullList.withSize(x, Ingredient.EMPTY), Ingredient::fromNetwork); + var count = buffer.readVarInt(); + var ingredients = NonNullList.withSize(count, Ingredient.EMPTY); + for (var i = 0; i < ingredients.size(); i++) ingredients.set(i, Ingredient.fromNetwork(buffer)); + return ingredients; } public static void writeIngredients(FriendlyByteBuf buffer, NonNullList ingredients) { diff --git a/projects/common/src/test/java/dan200/computercraft/shared/recipe/RecipeArbitraries.java b/projects/common/src/test/java/dan200/computercraft/shared/recipe/RecipeArbitraries.java new file mode 100644 index 000000000..36235fb0c --- /dev/null +++ b/projects/common/src/test/java/dan200/computercraft/shared/recipe/RecipeArbitraries.java @@ -0,0 +1,50 @@ +// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.shared.recipe; + +import dan200.computercraft.test.shared.MinecraftArbitraries; +import it.unimi.dsi.fastutil.ints.IntIntImmutablePair; +import net.jqwik.api.Arbitraries; +import net.jqwik.api.Arbitrary; +import net.jqwik.api.Combinators; +import net.minecraft.core.NonNullList; +import net.minecraft.world.item.crafting.CraftingBookCategory; +import net.minecraft.world.item.crafting.Ingredient; + +/** + * {@link Arbitrary} implementations for recipes. + */ +public final class RecipeArbitraries { + public static Arbitrary recipeProperties() { + return Combinators.combine( + Arbitraries.strings().ofMinLength(1).withChars("abcdefghijklmnopqrstuvwxyz_"), + Arbitraries.of(CraftingBookCategory.values()) + ).as(RecipeProperties::new); + } + + public static Arbitrary shapelessRecipeSpec() { + return Combinators.combine( + recipeProperties(), + MinecraftArbitraries.ingredient().array(Ingredient[].class).ofMinSize(1).map(x -> NonNullList.of(Ingredient.EMPTY, x)), + MinecraftArbitraries.nonEmptyItemStack() + ).as(ShapelessRecipeSpec::new); + } + + public static Arbitrary shapedTemplate() { + return Combinators.combine(Arbitraries.integers().between(1, 3), Arbitraries.integers().between(1, 3)) + .as(IntIntImmutablePair::new) + .flatMap(x -> MinecraftArbitraries.ingredient().array(Ingredient[].class).ofSize(x.leftInt() * x.rightInt()) + .map(i -> new ShapedTemplate(x.leftInt(), x.rightInt(), NonNullList.of(Ingredient.EMPTY, i))) + ); + } + + public static Arbitrary shapedRecipeSpec() { + return Combinators.combine( + recipeProperties(), + shapedTemplate(), + MinecraftArbitraries.nonEmptyItemStack() + ).as(ShapedRecipeSpec::new); + } +} diff --git a/projects/common/src/test/java/dan200/computercraft/shared/recipe/RecipeEqualities.java b/projects/common/src/test/java/dan200/computercraft/shared/recipe/RecipeEqualities.java new file mode 100644 index 000000000..c237c8686 --- /dev/null +++ b/projects/common/src/test/java/dan200/computercraft/shared/recipe/RecipeEqualities.java @@ -0,0 +1,34 @@ +// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.shared.recipe; + +import dan200.computercraft.test.core.StructuralEquality; +import dan200.computercraft.test.shared.MinecraftEqualities; + +/** + * {@link StructuralEquality} implementations for recipes. + */ +public final class RecipeEqualities { + private RecipeEqualities() { + } + + public static final StructuralEquality shapelessRecipeSpec = StructuralEquality.all( + StructuralEquality.at("properties", ShapelessRecipeSpec::properties), + StructuralEquality.at("ingredients", ShapelessRecipeSpec::ingredients, MinecraftEqualities.ingredient.list()), + StructuralEquality.at("result", ShapelessRecipeSpec::result, MinecraftEqualities.itemStack) + ); + + public static final StructuralEquality shapedTemplate = StructuralEquality.all( + StructuralEquality.at("width", ShapedTemplate::width), + StructuralEquality.at("height", ShapedTemplate::height), + StructuralEquality.at("ingredients", ShapedTemplate::ingredients, MinecraftEqualities.ingredient.list()) + ); + + public static final StructuralEquality shapedRecipeSpec = StructuralEquality.all( + StructuralEquality.at("properties", ShapedRecipeSpec::properties), + StructuralEquality.at("ingredients", ShapedRecipeSpec::template, shapedTemplate), + StructuralEquality.at("result", ShapedRecipeSpec::result, MinecraftEqualities.itemStack) + ); +} diff --git a/projects/common/src/test/java/dan200/computercraft/shared/recipe/ShapedRecipeSpecTest.java b/projects/common/src/test/java/dan200/computercraft/shared/recipe/ShapedRecipeSpecTest.java new file mode 100644 index 000000000..eaae17a04 --- /dev/null +++ b/projects/common/src/test/java/dan200/computercraft/shared/recipe/ShapedRecipeSpecTest.java @@ -0,0 +1,32 @@ +// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.shared.recipe; + +import dan200.computercraft.test.shared.NetworkSupport; +import dan200.computercraft.test.shared.WithMinecraft; +import net.jqwik.api.Arbitrary; +import net.jqwik.api.ForAll; +import net.jqwik.api.Property; +import net.jqwik.api.Provide; + +import static org.hamcrest.MatcherAssert.assertThat; + +@WithMinecraft +public class ShapedRecipeSpecTest { + static { + WithMinecraft.Setup.bootstrap(); // @Property doesn't run test lifecycle methods. + } + + @Property + public void testRoundTrip(@ForAll("recipe") ShapedRecipeSpec spec) { + var converted = NetworkSupport.roundTrip(spec, ShapedRecipeSpec::toNetwork, ShapedRecipeSpec::fromNetwork); + assertThat("Recipes are equal", converted, RecipeEqualities.shapedRecipeSpec.asMatcher(ShapedRecipeSpec.class, spec)); + } + + @Provide + Arbitrary recipe() { + return RecipeArbitraries.shapedRecipeSpec(); + } +} diff --git a/projects/common/src/test/java/dan200/computercraft/shared/recipe/ShapelessRecipeSpecTest.java b/projects/common/src/test/java/dan200/computercraft/shared/recipe/ShapelessRecipeSpecTest.java new file mode 100644 index 000000000..3e145a269 --- /dev/null +++ b/projects/common/src/test/java/dan200/computercraft/shared/recipe/ShapelessRecipeSpecTest.java @@ -0,0 +1,32 @@ +// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.shared.recipe; + +import dan200.computercraft.test.shared.NetworkSupport; +import dan200.computercraft.test.shared.WithMinecraft; +import net.jqwik.api.Arbitrary; +import net.jqwik.api.ForAll; +import net.jqwik.api.Property; +import net.jqwik.api.Provide; + +import static org.hamcrest.MatcherAssert.assertThat; + +@WithMinecraft +public class ShapelessRecipeSpecTest { + static { + WithMinecraft.Setup.bootstrap(); // @Property doesn't run test lifecycle methods. + } + + @Property + public void testRoundTrip(@ForAll("recipe") ShapelessRecipeSpec spec) { + var converted = NetworkSupport.roundTrip(spec, ShapelessRecipeSpec::toNetwork, ShapelessRecipeSpec::fromNetwork); + assertThat("Recipes are equal", converted, RecipeEqualities.shapelessRecipeSpec.asMatcher(ShapelessRecipeSpec.class, spec)); + } + + @Provide + Arbitrary recipe() { + return RecipeArbitraries.shapelessRecipeSpec(); + } +} diff --git a/projects/common/src/test/java/dan200/computercraft/shared/turtle/upgrades/TurtleToolSerialiserTest.java b/projects/common/src/test/java/dan200/computercraft/shared/turtle/upgrades/TurtleToolSerialiserTest.java index fe520a271..338acdc1c 100644 --- a/projects/common/src/test/java/dan200/computercraft/shared/turtle/upgrades/TurtleToolSerialiserTest.java +++ b/projects/common/src/test/java/dan200/computercraft/shared/turtle/upgrades/TurtleToolSerialiserTest.java @@ -8,16 +8,13 @@ import dan200.computercraft.api.turtle.ITurtleUpgrade; import dan200.computercraft.api.turtle.TurtleToolDurability; import dan200.computercraft.test.core.StructuralEquality; import dan200.computercraft.test.shared.MinecraftArbitraries; +import dan200.computercraft.test.shared.MinecraftEqualities; +import dan200.computercraft.test.shared.NetworkSupport; import dan200.computercraft.test.shared.WithMinecraft; -import io.netty.buffer.Unpooled; import net.jqwik.api.*; import net.minecraft.core.registries.Registries; -import net.minecraft.network.FriendlyByteBuf; -import net.minecraft.world.item.ItemStack; -import org.hamcrest.Description; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.assertEquals; @WithMinecraft class TurtleToolSerialiserTest { @@ -32,11 +29,9 @@ class TurtleToolSerialiserTest { */ @Property public void testRoundTrip(@ForAll("tool") TurtleTool tool) { - var buffer = new FriendlyByteBuf(Unpooled.directBuffer()); - TurtleToolSerialiser.INSTANCE.toNetwork(buffer, tool); - - var converted = TurtleToolSerialiser.INSTANCE.fromNetwork(tool.getUpgradeID(), buffer); - assertEquals(buffer.readableBytes(), 0, "Whole packet was read"); + var converted = NetworkSupport.roundTripSerialiser( + tool.getUpgradeID(), tool, TurtleToolSerialiser.INSTANCE::toNetwork, TurtleToolSerialiser.INSTANCE::fromNetwork + ); if (!equality.equals(tool, converted)) { System.out.println("Break"); @@ -58,22 +53,10 @@ class TurtleToolSerialiserTest { ).as(TurtleTool::new); } - private static final StructuralEquality stackEquality = new StructuralEquality<>() { - @Override - public boolean equals(ItemStack left, ItemStack right) { - return ItemStack.isSameItemSameTags(left, right) && left.getCount() == right.getCount(); - } - - @Override - public void describe(Description description, ItemStack object) { - description.appendValue(object).appendValue(object.getTag()); - } - }; - private static final StructuralEquality equality = StructuralEquality.all( StructuralEquality.at("id", ITurtleUpgrade::getUpgradeID), - StructuralEquality.at("craftingItem", ITurtleUpgrade::getCraftingItem, stackEquality), - StructuralEquality.at("tool", x -> x.item, stackEquality), + StructuralEquality.at("craftingItem", ITurtleUpgrade::getCraftingItem, MinecraftEqualities.itemStack), + StructuralEquality.at("tool", x -> x.item, MinecraftEqualities.itemStack), StructuralEquality.at("damageMulitiplier", x -> x.damageMulitiplier), StructuralEquality.at("allowEnchantments", x -> x.allowEnchantments), StructuralEquality.at("consumeDurability", x -> x.consumeDurability), diff --git a/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/MinecraftArbitraries.java b/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/MinecraftArbitraries.java index 05926d752..176dfe04d 100644 --- a/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/MinecraftArbitraries.java +++ b/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/MinecraftArbitraries.java @@ -17,6 +17,7 @@ import net.minecraft.tags.TagKey; import net.minecraft.world.item.Item; import net.minecraft.world.item.ItemStack; import net.minecraft.world.item.Items; +import net.minecraft.world.item.crafting.Ingredient; import java.util.List; @@ -44,6 +45,10 @@ public final class MinecraftArbitraries { return Arbitraries.oneOf(List.of(Arbitraries.just(ItemStack.EMPTY), nonEmptyItemStack())); } + public static Arbitrary ingredient() { + return nonEmptyItemStack().list().ofMinSize(1).map(x -> Ingredient.of(x.stream())); + } + public static Arbitrary blockPos() { // BlockPos has a maximum range that can be sent over the network - use those. var xz = Arbitraries.integers().between(-3_000_000, -3_000_000); diff --git a/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/MinecraftEqualities.java b/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/MinecraftEqualities.java new file mode 100644 index 000000000..8686bb250 --- /dev/null +++ b/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/MinecraftEqualities.java @@ -0,0 +1,39 @@ +// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.test.shared; + +import dan200.computercraft.test.core.StructuralEquality; +import net.minecraft.world.item.ItemStack; +import net.minecraft.world.item.crafting.Ingredient; +import org.hamcrest.Description; + +/** + * {@link StructuralEquality} implementations for Minecraft types. + */ +public class MinecraftEqualities { + public static final StructuralEquality itemStack = new StructuralEquality<>() { + @Override + public boolean equals(ItemStack left, ItemStack right) { + return ItemStack.isSameItemSameTags(left, right) && left.getCount() == right.getCount(); + } + + @Override + public void describe(Description description, ItemStack object) { + description.appendValue(object).appendValue(object.getTag()); + } + }; + + public static final StructuralEquality ingredient = new StructuralEquality<>() { + @Override + public boolean equals(Ingredient left, Ingredient right) { + return left.toJson().equals(right.toJson()); + } + + @Override + public void describe(Description description, Ingredient object) { + description.appendValue(object.toJson()); + } + }; +} diff --git a/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/NetworkSupport.java b/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/NetworkSupport.java new file mode 100644 index 000000000..9e723d5ff --- /dev/null +++ b/projects/common/src/testFixtures/java/dan200/computercraft/test/shared/NetworkSupport.java @@ -0,0 +1,56 @@ +// SPDX-FileCopyrightText: 2023 The CC: Tweaked Developers +// +// SPDX-License-Identifier: MPL-2.0 + +package dan200.computercraft.test.shared; + +import io.netty.buffer.Unpooled; +import net.minecraft.network.FriendlyByteBuf; +import net.minecraft.resources.ResourceLocation; +import net.minecraft.world.item.crafting.RecipeSerializer; + +import java.util.function.BiConsumer; +import java.util.function.BiFunction; +import java.util.function.Function; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Support methods for working with Minecraft's networking code. + */ +public final class NetworkSupport { + private NetworkSupport() { + } + + /** + * Attempt to serialise and then deserialise a value. + * + * @param value The value to serialise. + * @param write Serialise this value to a buffer. + * @param read Deserialise this value from a buffer. + * @param The type of the value to round trip. + * @return The converted value, for checking equivalency. + */ + public static T roundTrip(T value, BiConsumer write, Function read) { + var buffer = new FriendlyByteBuf(Unpooled.directBuffer()); + write.accept(value, buffer); + + var converted = read.apply(buffer); + assertEquals(buffer.readableBytes(), 0, "Whole packet was read"); + return converted; + } + + /** + * Attempt to serialise and then deserialise a value from a {@link RecipeSerializer}-like interface. + * + * @param id The id of this value. + * @param value The value to serialise. + * @param write Serialise this value to a buffer. + * @param read Deserialise this value from a buffer. + * @param The type of the value to round trip. + * @return The converted value, for checking equivalency. + */ + public static T roundTripSerialiser(ResourceLocation id, T value, BiConsumer write, BiFunction read) { + return roundTrip(value, (x, b) -> write.accept(b, x), b -> read.apply(id, b)); + } +} diff --git a/projects/core/src/testFixtures/java/dan200/computercraft/test/core/StructuralEqualities.java b/projects/core/src/testFixtures/java/dan200/computercraft/test/core/StructuralEqualities.java index a35a4a04c..9517cc526 100644 --- a/projects/core/src/testFixtures/java/dan200/computercraft/test/core/StructuralEqualities.java +++ b/projects/core/src/testFixtures/java/dan200/computercraft/test/core/StructuralEqualities.java @@ -100,6 +100,29 @@ final class StructuralEqualities { } } + record ListEquality(StructuralEquality equality) implements StructuralEquality> { + @Override + public boolean equals(List left, List right) { + if (left.size() != right.size()) return false; + for (var i = 0; i < left.size(); i++) { + if (!equality.equals(left.get(i), right.get(i))) return false; + } + return true; + } + + @Override + public void describe(Description description, List object) { + description.appendText("["); + var separator = false; + for (var value : object) { + if (separator) description.appendText(", "); + separator = true; + equality.describe(description, value); + } + description.appendText("]"); + } + } + static final class EqualityMatcher extends TypeSafeMatcher { private final StructuralEquality equality; private final T equalTo; diff --git a/projects/core/src/testFixtures/java/dan200/computercraft/test/core/StructuralEquality.java b/projects/core/src/testFixtures/java/dan200/computercraft/test/core/StructuralEquality.java index ac0d26de8..5045ebf43 100644 --- a/projects/core/src/testFixtures/java/dan200/computercraft/test/core/StructuralEquality.java +++ b/projects/core/src/testFixtures/java/dan200/computercraft/test/core/StructuralEquality.java @@ -36,6 +36,15 @@ public interface StructuralEquality { */ void describe(Description description, T object); + /** + * Lift this equality to a list of values. + * + * @return A equality for a list of values. + */ + default StructuralEquality> list() { + return new StructuralEqualities.ListEquality<>(this); + } + /** * Convert this equality instance to a {@link Matcher}. * diff --git a/projects/fabric/build.gradle.kts b/projects/fabric/build.gradle.kts index 9b4912996..3a737ca38 100644 --- a/projects/fabric/build.gradle.kts +++ b/projects/fabric/build.gradle.kts @@ -91,6 +91,8 @@ dependencies { testImplementation(libs.byteBuddyAgent) testImplementation(libs.bundles.test) testRuntimeOnly(libs.bundles.testRuntime) + + testFixturesImplementation(testFixtures(project(":core"))) } sourceSets.main { resources.srcDir("src/generated/resources") } diff --git a/projects/forge/build.gradle.kts b/projects/forge/build.gradle.kts index 4dcb48ecb..bb80450e8 100644 --- a/projects/forge/build.gradle.kts +++ b/projects/forge/build.gradle.kts @@ -165,6 +165,8 @@ dependencies { testModImplementation(testFixtures(project(":core"))) testModImplementation(testFixtures(project(":forge"))) + testFixturesImplementation(testFixtures(project(":core"))) + "cctJavadoc"(libs.cctJavadoc) }