qa: fix typehandlerlibrary spotbugs findings #5154

soloturn · 2023-10-28T07:20:04Z

when building with java-11 it ends with:

❯ cd subsystems/TypeHandlerLibrary; gradle clean build
...
> Task :subsystems:TypeHandlerLibrary:spotbugsTest
M B PI: The used identifier ?>?2/2??? as field name in class org.terasology.persistence.typeHandling.InMemorySerializerTest$TypeGetter.STRING in source file ?>?3/2??? shadows the publicly available identifier from the Java Standard Library.  In InMemorySerializerTest.java
M C EC: Using .equals to compare two byte[]'s, (equivalent to ==) in org.terasology.persistence.typeHandling.coreTypes.factories.BytesTypeHandlerTest.byteArraySerializeDeserialize()  At BytesTypeHandlerTest.java:[line 42]
M C EC: Using .equals to compare two byte[]'s, (equivalent to ==) in org.terasology.persistence.typeHandling.coreTypes.factories.BytesTypeHandlerTest.byteArraySerializeDeserialize()  At BytesTypeHandlerTest.java:[line 45]
M C EC: Using .equals to compare two byte[]'s, (equivalent to ==) in org.terasology.persistence.typeHandling.InMemorySerializerTest.serializeBytes()  At InMemorySerializerTest.java:[line 309]
M C EC: Using .equals to compare two byte[]'s, (equivalent to ==) in org.terasology.persistence.typeHandling.InMemorySerializerTest.serializeByteBuffer()  At InMemorySerializerTest.java:[line 338]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.serializeString()  At InMemorySerializerTest.java:[line 82]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.serializeStrings()  At InMemorySerializerTest.java:[line 113]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.template(PersistedData)  At InMemorySerializerTest.java:[line 136]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.serializeBoolean()  At InMemorySerializerTest.java:[line 255]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.serializeBytes()  At InMemorySerializerTest.java:[line 319]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.serializeByteBuffer()  At InMemorySerializerTest.java:[line 348]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.serializeNull()  At InMemorySerializerTest.java:[line 392]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.checkIsArray(PersistedData)  At InMemorySerializerTest.java:[line 434]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.checkIsNumber(PersistedData)  At InMemorySerializerTest.java:[line 458]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.checkValueArray(PersistedData, PersistedData, Set)  At InMemorySerializerTest.java:[line 486]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.lambda$checkValueArray$8(Executable)  At InMemorySerializerTest.java:[line 512]
H C RV: org.junit.jupiter.api.Assertions.assertThrows(Class, Executable) not thrown in org.terasology.persistence.typeHandling.InMemorySerializerTest.lambda$checkValueArray$4(Executable)  At InMemorySerializerTest.java:[line 497]
M P UuF: Unused field: org.terasology.persistence.typeHandling.coreTypes.factories.ObjectFieldMapTypeHandlerFactoryTest$SomeClass.t  In ObjectFieldMapTypeHandlerFactoryTest.java
M P SIC: Should org.terasology.persistence.typeHandling.FutureTypeHandlerTest$ResultCaptor be a _static_ inner class?  At FutureTypeHandlerTest.java:[lines 41-50]
M P UuF: Unused field: org.terasology.persistence.typeHandling.coreTypes.factories.ObjectFieldMapTypeHandlerFactoryTest$SomeClass.list  In ObjectFieldMapTypeHandlerFactoryTest.java
SpotBugs ended with exit code 1

> Task :subsystems:TypeHandlerLibrary:spotbugsMain
SpotBugs ended with exit code 1

see also #3859

...tems/TypeHandlerLibrary/src/main/java/org/terasology/persistence/serializers/Serializer.java

...rg/terasology/persistence/typeHandling/coreTypes/factories/CollectionTypeHandlerFactory.java

...erLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/PersistedBytes.java

...main/java/org/terasology/persistence/typeHandling/inMemory/arrays/PersistedBooleanArray.java

...dlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java

jdrueckert · 2023-10-30T11:33:19Z

...lerLibrary/src/test/java/org/terasology/persistence/typeHandling/InMemorySerializerTest.java

+// spotbugs does thinks Assertions.assertThrows does not throw the exception, even if
+// test is successful. see bug: https://github.com/spotbugs/spotbugs/issues/2667.
+@edu.umd.cs.findbugs.annotations.SuppressFBWarnings


seems to be fixed with spotbugs/spotbugs#2648 but not released yet
we don't fail for spotbugs findings at the moment, so I would not suppress this now but wait for the spotbugs patch

agree and took it out.

new spotbug version now included, warnings are gone.

jdrueckert · 2023-10-30T11:36:57Z

...ology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java

+// both, pmd and spotbugs complain about not used private fields, suppress in
+// the static test class, but fields are checked. suppress.
+@edu.umd.cs.findbugs.annotations.SuppressFBWarnings
+@SuppressWarnings("PMD.UnusedPrivateField")


I'm not entirely sure we want to suppress this as it might be a valid indicator of improvement potential. Either the private fields are indeed unused then this begs the question why they're in here in the first place. And if they are not explicitly used but implicitly required for the tests, then I would argue that this asks for refactoring due to nontransparent side effects.

it is "just" a type handler test and checks the type? to test nothing needs to be implemented using the fields.

to test nothing needs to be implemented using the fields.

Sorry, you'll have to elaborate on this, I don't follow.

moved the suppressions to the class which really causes it, when i tried that initially it failed, but cannot reproduce how. its a minimal test case, no point in using these private fields other than by typehandler.

jdrueckert commented on pr: MovingBlocks/Terasology#5154 that the rule is not really beneficial and better remove the rule than suppress it.

soloturn · 2023-11-05T15:56:42Z

@jdrueckert anything else missing?

...ology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java

...erLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/PersistedBytes.java

jdrueckert · 2023-11-08T14:48:43Z

...dlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java

+        }
+    }
+
+    // the test verfies that bot, static class (above), and class work. with this typehandler.


Spotbugs complains about the inner class ResultCaptor not being static. I'm not convinced, that the statement "the test verfies that bot, static class (above), and class work. with this typehandler." is correct. The test verifies that for recursive types, the expected typehandler (FutureTypeHandler) is fetched. For this, it uses ResultCaptor as a mock to first capture the result and later assert on it.

The reason why it doesn't work making the inner ResultCaptor class static is that this mock is not a static mock - it doesn't return a pre-defined value but executes the "real" method and captures its result.

Furthermore, the reason why Spotbugs complains about this is that the inner class not being static may result in performance issues due to increased instance sizes and object lifetimes. However, as this is a test, we don't care as much about performance.

...dlerLibrary/src/test/java/org/terasology/persistence/typeHandling/FutureTypeHandlerTest.java

...ology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java

...erLibrary/src/main/java/org/terasology/persistence/typeHandling/inMemory/PersistedBytes.java

...main/java/org/terasology/persistence/typeHandling/inMemory/arrays/PersistedBooleanArray.java

...rg/terasology/persistence/typeHandling/coreTypes/factories/CollectionTypeHandlerFactory.java

as a workaround, add dependency for spotbugs annotations, and create a [ticket there so the spotbugs gradle plugin would include the dependency] (spotbugs/spotbugs-gradle-plugin#1018). see here a motivation for inner type last rule. if we leave the rule we should respect it, otherwise disable the rule. https://stackoverflow.com/questions/25158939/what-motivation-is-behind-checkstyle-inner-type-last-rule especially in tests just quite the warnings, in most cases the code is deliberate as it is. see MovingBlocks#3859 and see bugs: * pmd/pmd#4731 * spotbugs/spotbugs-gradle-plugin#1018 * spotbugs/spotbugs#2667

now AssertThrows error is fixed: spotbugs/spotbugs#2667

especially take out pmd guardlog suppresses. fix part of them in a catch even if not necessary from a performance point of view. and create a pull request towards teraconfig to not use the guardlog rule any more: MovingBlocks/TeraConfig#23 Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>

soloturn force-pushed the typehandlerlibrary-qa branch 10 times, most recently from 1de72e9 to 655a6f4 Compare October 29, 2023 19:00

jdrueckert reviewed Oct 30, 2023

View reviewed changes

jdrueckert added this to the 2023 Revive - Milestone 2 milestone Oct 30, 2023

soloturn added a commit to soloturn/teraconfig that referenced this pull request Nov 1, 2023

no benefit in using guardlog.

5c1d774

jdrueckert commented on pr: MovingBlocks/Terasology#5154 that the rule is not really beneficial and better remove the rule than suppress it.

soloturn mentioned this pull request Nov 1, 2023

no benefit in using guardlog. MovingBlocks/TeraConfig#23

Closed

soloturn force-pushed the typehandlerlibrary-qa branch 3 times, most recently from af320a2 to a49261d Compare November 8, 2023 07:18

soloturn commented Nov 8, 2023

View reviewed changes

...ology/persistence/typeHandling/coreTypes/factories/ObjectFieldMapTypeHandlerFactoryTest.java Show resolved Hide resolved

jdrueckert reviewed Nov 8, 2023

View reviewed changes

jdrueckert mentioned this pull request Nov 8, 2023

test: split and improve ObjectFieldMapTypeHandlerFactoryTests #5168

Merged

soloturn force-pushed the typehandlerlibrary-qa branch 6 times, most recently from 511dbe2 to 0263c33 Compare November 11, 2023 18:51

soloturn force-pushed the typehandlerlibrary-qa branch 3 times, most recently from 842db88 to 4ab80e5 Compare November 17, 2023 08:08

jdrueckert requested changes Nov 17, 2023

View reviewed changes

soloturn force-pushed the typehandlerlibrary-qa branch from 4ab80e5 to 885c9eb Compare November 17, 2023 15:49

soloturn and others added 4 commits November 17, 2023 21:42

chore: kotlin jps updated itself

d6469ab

update spotbugs version.

60b1984

now AssertThrows error is fixed: spotbugs/spotbugs#2667

soloturn force-pushed the typehandlerlibrary-qa branch from 885c9eb to 2de615c Compare November 18, 2023 03:15

Merge branch 'develop' into typehandlerlibrary-qa

6e26882

jdrueckert approved these changes Nov 18, 2023

View reviewed changes

jdrueckert changed the title ~~typehandlerlibrary qa, spotbugs~~ qa: fix typehandlerlibrary spotbugs findings Nov 18, 2023

jdrueckert merged commit 8b4b6b9 into MovingBlocks:develop Nov 18, 2023
9 checks passed

soloturn deleted the typehandlerlibrary-qa branch December 3, 2023 20:48

jdrueckert assigned soloturn Jan 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qa: fix typehandlerlibrary spotbugs findings #5154

qa: fix typehandlerlibrary spotbugs findings #5154

soloturn commented Oct 28, 2023

jdrueckert Oct 30, 2023

soloturn Nov 2, 2023

soloturn Nov 10, 2023

jdrueckert Oct 30, 2023

soloturn Nov 1, 2023

jdrueckert Nov 7, 2023

soloturn Nov 8, 2023

soloturn commented Nov 5, 2023

jdrueckert Nov 8, 2023

qa: fix typehandlerlibrary spotbugs findings #5154

qa: fix typehandlerlibrary spotbugs findings #5154

Conversation

soloturn commented Oct 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soloturn commented Nov 5, 2023

Choose a reason for hiding this comment