From 1f3e359d0bfd5f16c14c9b52f081019eae84d017 Mon Sep 17 00:00:00 2001 From: soloturn Date: Sun, 3 Dec 2023 15:55:30 +0100 Subject: [PATCH] qa: improve code quality in engine tests (#5178) * qa engine-tests, checkstyle. * parameterized log statement. * remove unused * upgrade sonarqube gradle plugin. * AssertThrows instead of cacthing exception * pmd warnings in CountCallsTest * final fields in TypeSerializertest * remove unused method in InputsystemTests * qa engine-tests, feedback skaldarnar --- build-logic/build.gradle.kts | 2 +- .../apiScraper/CompleteApiScraper.java | 4 +- ...lasspathCompromisingModuleFactoryTest.java | 6 +- .../engine/entitySystem/PrefabTest.java | 5 -- .../internal/MetadataBuilderTest.java | 4 +- .../ClientConnectionTest.java | 3 +- .../engine/logic/behavior/CountCallsTest.java | 21 ++++--- .../serializers/TypeSerializerTest.java | 4 +- .../serializers/VectorTypeSerializerTest.java | 24 ++++---- ...er.java => VectorEventSerializerTest.java} | 22 +++---- .../mathTypes/AABBTypeHandlerTest.java | 28 +++++---- .../mathTypes/BlockAreaTypeHandlerTest.java | 11 ++-- .../mathTypes/BlockRegionTypeHandlerTest.java | 10 ++-- .../engine/world/Zones/ZoneTest.java | 9 +-- .../terasology/input/InputSystemTests.java | 10 ---- .../utilities/tree/SpaceTreeTest.java | 57 +++---------------- 16 files changed, 77 insertions(+), 143 deletions(-) rename engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/event/{VectorEventSerializer.java => VectorEventSerializerTest.java} (99%) diff --git a/build-logic/build.gradle.kts b/build-logic/build.gradle.kts index 3e9f710485e..79cf5169b18 100644 --- a/build-logic/build.gradle.kts +++ b/build-logic/build.gradle.kts @@ -48,7 +48,7 @@ dependencies { // plugins we configure implementation("com.github.spotbugs.snom:spotbugs-gradle-plugin:5.2.3") - implementation("org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:3.3") + implementation("org.sonarsource.scanner.gradle:sonarqube-gradle-plugin:4.4.1.3373") api(kotlin("test")) } diff --git a/engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java b/engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java index 3d90d48cd04..c2aea8f6267 100644 --- a/engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java +++ b/engine-tests/src/test/java/org/terasology/documentation/apiScraper/CompleteApiScraper.java @@ -63,7 +63,7 @@ static StringBuffer getApi() throws Exception { } if (location == null) { - logger.error("Failed to get a class/package location, skipping " + apiClass); + logger.error("Failed to get a class/package location, skipping {}", apiClass); continue; } @@ -97,7 +97,7 @@ static StringBuffer getApi() throws Exception { break; default : - logger.error("Unknown protocol for: " + apiClass + ", came from " + location); + logger.error("Unknown protocol for: {} , came from {}", apiClass, location); } } api.putAll(EXTERNAL, ExternalApiWhitelist.CLASSES.stream() diff --git a/engine-tests/src/test/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactoryTest.java b/engine-tests/src/test/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactoryTest.java index b9ee465aa1e..5e9bfaf5303 100644 --- a/engine-tests/src/test/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactoryTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/core/module/ClasspathCompromisingModuleFactoryTest.java @@ -25,7 +25,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class ClasspathCompromisingModuleFactoryTest { - static final Class someClassOutsideTheModule = ClasspathCompromisingModuleFactory.class; + static final Class SOME_CLASS_OUTSIDE_THE_MODULE = ClasspathCompromisingModuleFactory.class; static final String METADATA_NAME = "module.json"; ClasspathCompromisingModuleFactory factory; @@ -46,7 +46,7 @@ public void directoryModuleContainsClass() { // and that ExampleClass is inside that directory assertTrue(module.getClassPredicate().test(ExampleClass.class)); // and that this other class (in engine, not engine-test) is outside that directory. - assertFalse(module.getClassPredicate().test(someClassOutsideTheModule)); + assertFalse(module.getClassPredicate().test(SOME_CLASS_OUTSIDE_THE_MODULE)); // These assumptions could break if things get moved around enough. } @@ -59,7 +59,7 @@ public void archiveModuleContainsClass() throws IOException { Class someClassInTheModule = module.getModuleManifest().getTypesAnnotatedWith(API.class).iterator().next(); assertTrue(module.getClassPredicate().test(someClassInTheModule)); - assertFalse(module.getClassPredicate().test(someClassOutsideTheModule)); + assertFalse(module.getClassPredicate().test(SOME_CLASS_OUTSIDE_THE_MODULE)); } @Test diff --git a/engine-tests/src/test/java/org/terasology/engine/entitySystem/PrefabTest.java b/engine-tests/src/test/java/org/terasology/engine/entitySystem/PrefabTest.java index 8796182566d..813d84b2351 100644 --- a/engine-tests/src/test/java/org/terasology/engine/entitySystem/PrefabTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/entitySystem/PrefabTest.java @@ -4,8 +4,6 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; import org.terasology.engine.core.module.ModuleManager; import org.terasology.gestalt.assets.AssetType; import org.terasology.gestalt.assets.management.AssetManager; @@ -45,9 +43,6 @@ import static org.mockito.Mockito.when; public class PrefabTest { - - private static final Logger logger = LoggerFactory.getLogger(PrefabTest.class); - private PrefabManager prefabManager; @BeforeEach diff --git a/engine-tests/src/test/java/org/terasology/engine/entitySystem/metadata/internal/MetadataBuilderTest.java b/engine-tests/src/test/java/org/terasology/engine/entitySystem/metadata/internal/MetadataBuilderTest.java index 6c2943e0349..278c5cec06b 100644 --- a/engine-tests/src/test/java/org/terasology/engine/entitySystem/metadata/internal/MetadataBuilderTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/entitySystem/metadata/internal/MetadataBuilderTest.java @@ -17,8 +17,8 @@ public class MetadataBuilderTest { - private ReflectFactory factory = new ReflectionReflectFactory(); - private CopyStrategyLibrary copyStrategyLibrary = new CopyStrategyLibrary(factory); + private final ReflectFactory factory = new ReflectionReflectFactory(); + private final CopyStrategyLibrary copyStrategyLibrary = new CopyStrategyLibrary(factory); @BeforeEach public void setup() { diff --git a/engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientConnectionTest.java b/engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientConnectionTest.java index ae3d17d74d1..36e4956a17b 100644 --- a/engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientConnectionTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/integrationenvironment/ClientConnectionTest.java @@ -6,7 +6,6 @@ import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import org.terasology.engine.context.Context; import org.terasology.engine.core.TerasologyEngine; import org.terasology.engine.core.modes.StateIngame; import org.terasology.engine.integrationenvironment.jupiter.IntegrationEnvironment; @@ -21,7 +20,7 @@ public class ClientConnectionTest { @Test public void testClientConnection(ModuleTestingHelper helper) throws IOException { - Context clientContext = helper.createClient(); + helper.createClient(); List engines = helper.getEngines(); Assertions.assertEquals(2, engines.size()); logger.info("Engine 0 is {}", engines.get(0)); diff --git a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/CountCallsTest.java b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/CountCallsTest.java index 33168edd9ae..c54cd8ff8ff 100644 --- a/engine-tests/src/test/java/org/terasology/engine/logic/behavior/CountCallsTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/logic/behavior/CountCallsTest.java @@ -12,14 +12,16 @@ import org.terasology.engine.logic.behavior.core.DelegateNode; import java.util.List; +import java.util.stream.Collectors; +import java.util.stream.IntStream; import static org.junit.jupiter.api.Assertions.assertEquals; public class CountCallsTest { public int nextId2; - private List constructCalled = Lists.newArrayList(); - private List destructCalled = Lists.newArrayList(); - private List executeCalled = Lists.newArrayList(); + private final List constructCalled = Lists.newArrayList(); + private final List destructCalled = Lists.newArrayList(); + private final List executeCalled = Lists.newArrayList(); private GsonBuilder gsonBuilder; @BeforeEach @@ -49,16 +51,13 @@ public void assertBT(String tree, List result, List exec assertBT(tree, result, executed, true); } - public void assertBT(String tree, List result, List executed, boolean step) { + // TODO check why ignoredStep is there and tested. + public void assertBT(String tree, List result, List executed, boolean ignoredStep) { BehaviorNode node = fromJson(tree); node.construct(null); - List actualStates = Lists.newArrayList(); - for (int i = 0; i < result.size(); i++) { - BehaviorState state = node.execute(null); - actualStates.add(state); - - } + List actualStates = + IntStream.range(0, result.size()).mapToObj(i -> node.execute(null)).collect(Collectors.toList()); node.destruct(null); assertEquals(result, actualStates); @@ -70,7 +69,7 @@ public BehaviorNode fromJson(String json) { } private class CountDelegate extends DelegateNode { - private int id; + private final int id; CountDelegate(BehaviorNode delegate) { super(delegate); diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java b/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java index 4adecab3a09..dcdfa70494a 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/TypeSerializerTest.java @@ -113,8 +113,8 @@ void testJsonSerializeDeserialize() throws IOException { private static final class SomeClass { @SerializedName("generic-t") private T data; - private List list = Lists.newArrayList(); - private Set> animals = Sets.newHashSet(); + private final List list = Lists.newArrayList(); + private final Set> animals = Sets.newHashSet(); private Animal singleAnimal; private SomeClass(T data) { diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java b/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java index 8d454d629bb..0ad30391fb7 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/serializers/VectorTypeSerializerTest.java @@ -25,18 +25,6 @@ @SuppressWarnings("FieldCanBeLocal") class VectorTypeSerializerTest extends ModuleEnvironmentTest { - static class TestObject { - public Vector3f v1; - public Vector2f v2; - public Vector4f v3; - } - - static class TestObject2 { - public Vector3fc v1; - public Vector4fc v2; - public Vector2fc v3; - } - private TypeHandlerLibrary typeHandlerLibrary; private Serializer gsonSerializer; private Gson gson = new Gson(); @@ -70,4 +58,16 @@ void testSerializationConstant() { assertEquals(new Vector4f(1.0f, 2.0f, 3.0f, 5.0f), o.v2, .00001f); assertEquals(new Vector2f(1.0f, 2.0f), o.v3, .00001f); } + + static class TestObject { + public Vector3f v1; + public Vector2f v2; + public Vector4f v3; + } + + static class TestObject2 { + public Vector3fc v1; + public Vector4fc v2; + public Vector2fc v3; + } } diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/event/VectorEventSerializer.java b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/event/VectorEventSerializerTest.java similarity index 99% rename from engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/event/VectorEventSerializer.java rename to engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/event/VectorEventSerializerTest.java index 0225cf41c3f..a67c30c29fa 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/event/VectorEventSerializer.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/event/VectorEventSerializerTest.java @@ -36,7 +36,7 @@ import static org.junit.jupiter.api.Assumptions.assumeTrue; import static org.terasology.joml.test.VectorAssert.assertEquals; -public class VectorEventSerializer { +public class VectorEventSerializerTest { Map, Integer> eventMap = new HashMap<>(); int indexCount = 0; @@ -46,16 +46,6 @@ public class VectorEventSerializer { private ReflectFactory reflectFactory = new ReflectionReflectFactory(); private CopyStrategyLibrary copyStrategies = new CopyStrategyLibrary(reflectFactory); - public static class Vector3fTestEvent implements Event { - public Vector3f v1; - public Vector4f v2; - public Vector2f v3; - - public Vector3fc v1c; - public Vector4fc v2c; - public Vector2fc v3c; - } - @BeforeEach public void setup() throws Exception { ContextImpl context = new ContextImpl(); @@ -114,4 +104,14 @@ public void testEventSerializationConstant() throws IOException { assertEquals(((Vector3fTestEvent) dev).v2c, new Vector4f(1.0f, 1.0f, 2.0f, 2.0f), .00001f); assertEquals(((Vector3fTestEvent) dev).v3c, new Vector2f(1.0f, 1.0f), .00001f); } + + public static class Vector3fTestEvent implements Event { + public Vector3f v1; + public Vector4f v2; + public Vector2f v3; + + public Vector3fc v1c; + public Vector4fc v2c; + public Vector2fc v3c; + } } diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/AABBTypeHandlerTest.java b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/AABBTypeHandlerTest.java index b032f178904..700183735f6 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/AABBTypeHandlerTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/AABBTypeHandlerTest.java @@ -18,23 +18,8 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class AABBTypeHandlerTest extends MathTypeAssert { - - public static class AABB1Test { - public AABBi a1; - } - - public static class AABB2Test { - public AABBf a1; - } - - public static class AABB3Test { - public AABBf a1; - public AABBi a2; - } - private final Reflections reflections = new Reflections(getClass().getClassLoader()); private final TypeHandlerLibrary typeHandlerLibrary = TypeHandlerLibraryImpl.withReflections(reflections); - private AABBiTypeHandler handler = new AABBiTypeHandler(); private final Gson gson = GsonBuilderFactory.createGsonBuilderWithTypeSerializationLibrary(typeHandlerLibrary) @@ -93,4 +78,17 @@ public void testSerializeAABBf() { assertTrue(obj.has("a1")); assertAABBf(obj.get("a1"), 0, 2.0f, 1.5f, 10.0f, 5.0f, 10); } + + public static class AABB1Test { + public AABBi a1; + } + + public static class AABB2Test { + public AABBf a1; + } + + public static class AABB3Test { + public AABBf a1; + public AABBi a2; + } } diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java index a70db1ae67d..15f88346aa5 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockAreaTypeHandlerTest.java @@ -23,12 +23,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; public class BlockAreaTypeHandlerTest extends ModuleEnvironmentTest { - - static class TestObject { - public BlockArea b1; - public BlockAreac b2; - } - private TypeHandlerLibrary typeHandlerLibrary; private Serializer gsonSerializer; private Gson gson = new Gson(); @@ -60,4 +54,9 @@ public void testGsonSerialization() throws IOException { assertEquals(new BlockArea(-1, -1, 0, 0), o.b1); assertEquals(new BlockArea(0, 0, 1, 1), o.b2); } + + static class TestObject { + public BlockArea b1; + public BlockAreac b2; + } } diff --git a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockRegionTypeHandlerTest.java b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockRegionTypeHandlerTest.java index bfe36b916c4..83cf760cc54 100644 --- a/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockRegionTypeHandlerTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/mathTypes/BlockRegionTypeHandlerTest.java @@ -17,11 +17,6 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class BlockRegionTypeHandlerTest extends MathTypeAssert { - public static class AABBBlockRegion1Test { - public BlockRegion a1; - public AABBi a2; - } - private final Reflections reflections = new Reflections(getClass().getClassLoader()); private final TypeHandlerLibrary typeHandlerLibrary = TypeHandlerLibraryImpl.withReflections(reflections); @@ -42,4 +37,9 @@ public void testSerializeBlockRegion() { assertTrue(obj.has("a2")); assertAABBi(obj.get("a2"), 3, 5, 5, 22, 12, 14); } + + public static class AABBBlockRegion1Test { + public BlockRegion a1; + public AABBi a2; + } } diff --git a/engine-tests/src/test/java/org/terasology/engine/world/Zones/ZoneTest.java b/engine-tests/src/test/java/org/terasology/engine/world/Zones/ZoneTest.java index 7c19baf789d..dff9e4e2a71 100644 --- a/engine-tests/src/test/java/org/terasology/engine/world/Zones/ZoneTest.java +++ b/engine-tests/src/test/java/org/terasology/engine/world/Zones/ZoneTest.java @@ -8,8 +8,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; 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; public class ZoneTest { @@ -27,12 +27,7 @@ public void testGetChildZones() { zone.addZone(child); assertFalse(zone.getChildZones().isEmpty()); assertTrue(zone.getChildZones().contains(child)); - - try { - zone.getChildZone("Invalid name"); - fail(); - } catch (Exception e) { } - + assertThrows(Exception.class, () -> zone.getChildZone("Invalid name")); assertEquals(child, zone.getChildZone("Child")); } diff --git a/engine-tests/src/test/java/org/terasology/input/InputSystemTests.java b/engine-tests/src/test/java/org/terasology/input/InputSystemTests.java index 89a8a870631..2cc7d01fc13 100644 --- a/engine-tests/src/test/java/org/terasology/input/InputSystemTests.java +++ b/engine-tests/src/test/java/org/terasology/input/InputSystemTests.java @@ -238,16 +238,6 @@ private void releaseKey(Key key) { testKeyboard.add(rawKeyboardAction); } - private static char characterFor(Key key) { - //internal key chars depend on the lwjgl keyboard, this works for all letters - String displayName = key.getDisplayName(); - if (displayName.length() == 1) { - return displayName.charAt(0); - } else { - return ' '; - } - } - private static class TestKeyboard implements KeyboardDevice { private Queue queue = new LinkedBlockingQueue<>(); diff --git a/engine-tests/src/test/java/org/terasology/utilities/tree/SpaceTreeTest.java b/engine-tests/src/test/java/org/terasology/utilities/tree/SpaceTreeTest.java index 17cbb5afb5c..0b096a5fa73 100644 --- a/engine-tests/src/test/java/org/terasology/utilities/tree/SpaceTreeTest.java +++ b/engine-tests/src/test/java/org/terasology/utilities/tree/SpaceTreeTest.java @@ -13,62 +13,21 @@ import static org.junit.jupiter.api.Assertions.assertNotSame; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; public class SpaceTreeTest { @Test public void test2DTreeErrors() { SpaceTree tree = new SpaceTree<>(2); - try { - tree.add(null, new Object()); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException exp) { - // Expected - } - - try { - tree.add(new float[1], new Object()); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException exp) { - // Expected - } - - try { - tree.add(new float[3], new Object()); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException exp) { - // Expected - } - - try { - tree.add(new float[2], null); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException exp) { - // Expected - } - - try { - tree.remove(null); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException exp) { - // Expected - } - - try { - tree.remove(new float[1]); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException exp) { - // Expected - } - - try { - tree.remove(new float[3]); - fail("Expected IllegalArgumentException"); - } catch (IllegalArgumentException exp) { - // Expected - } + assertThrows(IllegalArgumentException.class, () -> tree.add(null, new Object())); + assertThrows(IllegalArgumentException.class, () -> tree.add(new float[1], new Object())); + assertThrows(IllegalArgumentException.class, () -> tree.add(new float[3], new Object())); + assertThrows(IllegalArgumentException.class, () -> tree.add(new float[2], null)); + assertThrows(IllegalArgumentException.class, () -> tree.remove(null)); + assertThrows(IllegalArgumentException.class, () -> tree.remove(new float[1])); + assertThrows(IllegalArgumentException.class, () -> tree.remove(new float[3])); } @Test