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

@@ -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"))
 }

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()

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

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

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() {

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<TerasologyEngine> engines = helper.getEngines();
         Assertions.assertEquals(2, engines.size());
         logger.info("Engine 0 is {}", engines.get(0));

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<Integer> constructCalled = Lists.newArrayList();
-    private List<Integer> destructCalled = Lists.newArrayList();
-    private List<Integer> executeCalled = Lists.newArrayList();
+    private final List<Integer> constructCalled = Lists.newArrayList();
+    private final List<Integer> destructCalled = Lists.newArrayList();
+    private final List<Integer> executeCalled = Lists.newArrayList();
     private GsonBuilder gsonBuilder;
 
     @BeforeEach
@@ -49,16 +51,13 @@ public void assertBT(String tree, List<BehaviorState> result, List<Integer> exec
         assertBT(tree, result, executed, true);
     }
 
-    public void assertBT(String tree, List<BehaviorState> result, List<Integer> executed, boolean step) {
+    // TODO check why ignoredStep is there and tested.
+    public void assertBT(String tree, List<BehaviorState> result, List<Integer> executed, boolean ignoredStep) {
         BehaviorNode node = fromJson(tree);
 
         node.construct(null);
-        List<BehaviorState> actualStates = Lists.newArrayList();
-        for (int i = 0; i < result.size(); i++) {
-            BehaviorState state = node.execute(null);
-            actualStates.add(state);
-
-        }
+        List<BehaviorState> 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);

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<T> {
         @SerializedName("generic-t")
         private T data;
-        private List<T> list = Lists.newArrayList();
-        private Set<Animal<?>> animals = Sets.newHashSet();
+        private final List<T> list = Lists.newArrayList();
+        private final Set<Animal<?>> animals = Sets.newHashSet();
         private Animal<T> singleAnimal;
 
         private SomeClass(T data) {

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;
+    }
 }

engine-tests/src/test/java/org/terasology/engine/persistence/typeHandling/event/VectorEventSerializer.java → 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<Class<? extends Event>, 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;
+    }
 }

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;
+    }
 }

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;
+    }
 }

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;
+    }
 }

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"));
     }
 

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<RawKeyboardAction> queue = new LinkedBlockingQueue<>();