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

GEODE-8032: Fix unit test and reclassify some tests as integration tests #5011

Merged

Conversation

kirklund
Copy link
Contributor

@kirklund kirklund commented Apr 27, 2020

I found 3 tests named *IntegrationTests in src/test that need to move:
* geode-core/src/test/java/org/apache/geode/distributed/internal/InternalDistributedSystemIntegrationTest.java
* geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/operations/OqlQueryRequestOperationHandlerIntegrationTest.java
* geode-wan/src/test/java/org/apache/geode/internal/cache/wan/GatewaySenderEventRemoteDispatcherIntegrationTest.java

These are unit tests that are better off renamed and moved to src/integrationTest because they touch several Geode classes including singletons:
* geode-core/src/test/java/org/apache/geode/distributed/internal/InternalLocatorTest.java
* geode-core/src/test/java/org/apache/geode/distributed/internal/locks/DLockServiceJUnitTest.java

This one creates a full DistributedSystem stack so it belongs in src/integrationTest:
* geode-core/src/test/java/org/apache/geode/distributed/internal/InternalDistributedSystemTest.java

And these unit tests create a full Cache stack so they belong in src/integrationTest:
* geode-lucene/src/test/java/org/apache/geode/cache/lucene/FlatFormatPdxSerializerJunitTest.java
* geode-protobuf/src/test/java/org/apache/geode/internal/protocol/protobuf/v1/serialization/codec/JsonPdxConverterJUnitTest.java

And one last unit test that I was able to fix -- this test creates a spy to test as a partial mock with a common goof that results in a full Cache being created and then not used by the test:
* extensions/geode-modules/src/test/java/org/apache/geode/modules/util/BootstrappingFunctionTest.java

@jdeppe-pivotal please review BootstrappingFunctionTest.java
@dschneider-pivotal please review InternalLocatorClusterManagementServiceIntegrationTest.java
@kirklund please review InternalDistributedSystemIntegrationTest.java
@pivotal-eshu please review InternalDistributedSystemLockMemoryIntegrationTest.java
@bschuchardt please review DLockServiceDisconnectIntegrationTest.java
@gesterzhou please review FlatFormatPdxSerializerIntegrationTest.java
@upthewaterspout please review OqlQueryRequestOperationHandlerIntegrationTest.java
@kohlmu-pivotal please review JsonPdxConverterIntegrationTest.java
@Bill please review GatewaySenderEventRemoteDispatcherIntegrationTest.java

Copy link
Contributor

@DonalEvans DonalEvans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely help avoid test ordering issues causing spurious failures.

Copy link
Contributor

@bschuchardt bschuchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

someone more familiar with Functions should review the refactoring of BootstrappingFunction. The rest of the changes look good to me. Thanks for doing this work Kirk.

@kirklund
Copy link
Contributor Author

Tomcat session tests failed in UpgradeTestOpen. Unrelated to this PR.

@kirklund
Copy link
Contributor Author

Tomcat session tests are still failing in UpgradeTestOpen.

These are tests that either create a full Cache or DistributedSystem
or touch so many classes that they're better off as integration
tests.
@kirklund kirklund force-pushed the GEODE-8032-unit-or-integration-tests-01 branch from 8963b1d to 5238d68 Compare April 29, 2020 16:07
@kirklund
Copy link
Contributor Author

Backing out the changes to BootstrappingFunction restores UpgradeTest to GREEN. I have filed https://issues.apache.org/jira/browse/GEODE-8040 and renamed/moved BootstrappingFunctionTest to src/integrationTest so that it won't break any other unit tests.

@kirklund
Copy link
Contributor Author

kirklund commented Apr 29, 2020

DistributedTest has an unrelated failure:

org.apache.geode.distributed.internal.deadlock.GemFireDeadlockDetectorDUnitTest > testDistributedDeadlockWithDLock FAILED
    java.lang.AssertionError
        at org.junit.Assert.fail(Assert.java:86)
        at org.junit.Assert.assertTrue(Assert.java:41)
        at org.junit.Assert.assertTrue(Assert.java:52)
        at org.apache.geode.distributed.internal.deadlock.GemFireDeadlockDetectorDUnitTest.testDistributedDeadlockWithDLock(GemFireDeadlockDetectorDUnitTest.java:201)

@kirklund kirklund merged commit dd695c4 into apache:develop Apr 29, 2020
@kirklund kirklund deleted the GEODE-8032-unit-or-integration-tests-01 branch April 29, 2020 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants