Skip to content
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

Help fix broken / ignored unit tests or other analytics! #3859

Open
Cervator opened this issue Mar 17, 2020 · 8 comments
Open

Help fix broken / ignored unit tests or other analytics! #3859

Cervator opened this issue Mar 17, 2020 · 8 comments
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Good First Issue Good for learners that are new to Terasology

Comments

@Cervator
Copy link
Member

Cervator commented Mar 17, 2020

After merging #3856 one unit test class, TypeRegistryTest, intermittently stalled during local testing. It may be fine on some OSes or in Jenkins, but oddly it stalled even with the @Ignored annotation added to the tests. It would be good if a few different people to re-enable those tests and run them locally several times (one at a time, all three at once, etc) and make sure they don't stall - if not then we can re-enable and I'll just blame my workspace.

Additionally at present we skip 7 tests in EntityAwareWorldProviderTest and another 7 in ParallelTest for reasons of the past I don't remember off the top of my head, but the files and Git / GitHub history may help shed some background info on that topic.

Finally there are two tests in the ModuleTestingEnvironment that are getting stuck in infinite loops for me testing locally in LocalChunkProvider.checkForUnload() - not sure that'd relate to these changes. ClientConnectionTest and ExampleTest

Beyond unit tests there are plenty of other code analytics that result in issues, although the stats are still only published in our new Jenkins which isn't exposed publicly yet, although regular contributors can request access any time for some insight. In the meantime you can run gradlew check locally and find an abundance of warnings :-)

In Jenkins specifically unit tests tend to fail if they involve a game environment, #3415 has the details and it would be a lovely thing to fix as well.

Marking Good First Issue as these topics are all mildly unrelated to anything else and decent detective hunts for curious contributors, even if they aren't necessarily going to be easy!

Edit: there is also a weird issue where SpotBugs works for some modules but not others, resulting in no build/reports/spotbugs directory. I saw an error related to it at one point, but forgot where. It showed both locally and in Jenkins so it shouldn't be hard to replicate if looking across a few modules. Pretty sure Pathfinding had the failure. Could run gradlew spotbugsMain to selectively just run SpotBugs in a workspace then go looking for which modules made that build dir or not.

@Cervator Cervator added Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Good First Issue Good for learners that are new to Terasology labels Mar 17, 2020
@Cervator
Copy link
Member Author

Cervator commented Apr 2, 2020

Wanted to leave this link here as well: https://github.com/jenkinsci/warnings-ng-plugin/blob/master/doc/Documentation.md

It goes a bit beyond the origin of this issue, but beyond hooking analytics back up in the new Jenkins at all there are so many options floating around for how we could do more. I also tried to enable the Sonarsource web scanner but while it worked fine for one project dynamically making it figure out multiple from one set of config / add more as needed without manual action might take a bit more work

@lizzyd710
Copy link

How's the code coverage? I've worked with JUnit and Mockito before and would be happy to work with unit tests.

@Cervator
Copy link
Member Author

Hi @lizzyd710! ... well, we have some! 😁

But 700+ tests barely make a dent in the engine project here when it comes to percentage coverage, then there are oodles of modules and other side projects. We're always low on tests - any assistance writing more would be really appreciated!

@lizzyd710
Copy link

lizzyd710 commented Apr 26, 2020

In TypeRegistryTest I uncommented the @Test annotations (but not the @Ignored) and none of them stalled for me. Here's my results:

  • testModuleTypes(): Passed, 6s 356ms
  • testNonModuleTypes(): Ignored, 4s 141ms
  • testWhitelistedTypes(): Ignored, 2s 795ms

In ParallelTest, all tests except testRunningSuccess pass. For tests that are ignored/disabled but pass when I run them, should I just delete the @Ignored/@Disabled?

@Cervator
Copy link
Member Author

Sure, at this point I'm less worried about the tests stalling in our Jenkins server, since that'll get tested as part of a PR now. Previously when I ignored things and wrote this issue we were still trying to get our build server up and running properly :-)

@pree-T
Copy link

pree-T commented Jan 11, 2022

Is this issue still open?

@jdrueckert
Copy link
Member

jdrueckert commented Jan 11, 2022

@pree-T Generally yes. It does need a decent bit of investigation first, though. But sure, go ahead, give it a try and ideally first drop your findings about broken or ignored tests in here before starting to try to fix them 😉

@soloturn
Copy link
Contributor

when building with java-11 it ends with:

❯ 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

i'll give to fix these ones a try.

soloturn added a commit to soloturn/terasology that referenced this issue Oct 28, 2023
soloturn added a commit to soloturn/terasology that referenced this issue Oct 28, 2023
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 MovingBlocks#3859
soloturn added a commit to soloturn/terasology that referenced this issue Oct 28, 2023
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

pmd warnings not yet addressed, there are some log guard related, which
are false positive.

see MovingBlocks#3859

typehandlerlibrary qa, checkstyle

squash to qa
soloturn added a commit to soloturn/terasology that referenced this issue Oct 28, 2023
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

pmd warnings not yet addressed, there are some log guard related, which
are false positive.

see MovingBlocks#3859

typehandlerlibrary qa, checkstyle

squash to qa
soloturn added a commit to soloturn/terasology that referenced this issue Oct 28, 2023
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

pmd warnings not yet addressed, there are some log guard related, which
are false positive.

see MovingBlocks#3859

typehandlerlibrary qa, checkstyle

squash to qa
soloturn added a commit to soloturn/terasology that referenced this issue Oct 29, 2023
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

pmd warnings not yet addressed, there are some log guard related, which
are false positive.

see MovingBlocks#3859

typehandlerlibrary qa, checkstyle

squash to qa
soloturn added a commit to soloturn/terasology that referenced this issue Oct 29, 2023
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

pmd warnings not yet addressed, there are some log guard related, which
are false positive.

see MovingBlocks#3859

typehandlerlibrary qa, checkstyle

squash to qa
soloturn added a commit to soloturn/terasology that referenced this issue Oct 29, 2023
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

pmd warnings not yet addressed, there are some log guard related, which
are false positive.

see MovingBlocks#3859

typehandlerlibrary qa, checkstyle

squash to qa
soloturn added a commit to soloturn/terasology that referenced this issue Oct 29, 2023
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
soloturn added a commit to soloturn/terasology that referenced this issue Oct 29, 2023
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
soloturn added a commit to soloturn/terasology that referenced this issue Oct 29, 2023
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
soloturn added a commit to soloturn/terasology that referenced this issue Nov 8, 2023
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
soloturn added a commit to soloturn/terasology that referenced this issue Nov 9, 2023
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
jdrueckert added a commit that referenced this issue Nov 11, 2023
* make final fields static
* remove unnecessary local before return
* remove instanceof checks in catch clause
* avoid throwing java.lang.Exception
* remove null checks covered by instanceof check
* remove toString on String objects
* remove overrides that only call their super
* remove unused private methods
* remove unnecessary if statement nesting
* remove unused local variables
* use try-with-resources to close resources after use
* use equals() to compare object references
* fix broken javadoc references
* fix disallowed self-closing and empty javadoc elements
* fix bad use of symbols in javadoc
* fix disallowed list item tag in javadoc without surrounding list
* fix erroneous javadoc tags
* fix emphasize tags in javadoc
* remove illegal throws
* remove unused imports

Related:
Terasology/CoreRendering#75
Terasology/Furnishings#17
Terasology/Health#105
Terasology/Inventory#51
Terasology/Behaviors#114
Terasology/CoreWorlds#45
Terasology/FlexiblePathfinding#32

Adds to #3859
soloturn added a commit to soloturn/terasology that referenced this issue Nov 11, 2023
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
soloturn added a commit to soloturn/terasology that referenced this issue Nov 17, 2023
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
soloturn added a commit to soloturn/terasology that referenced this issue Nov 18, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Build/CI Requests, Issues and Changes targeting gradle, groovy, Jenkins, etc. Good First Issue Good for learners that are new to Terasology
Projects
None yet
Development

No branches or pull requests

5 participants