You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kirk Lund <kl...@pivotal.io> on 2020/04/27 20:58:31 UTC
Dangers of using sockets in unit tests
This test started failing consistently on Windows (5 builds in a row so
far!):
org.apache.geode.internal.net.SocketCreatorFactoryJUnitTest >
testNewSSLConfigSSLComponentLocator 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.internal.net.SocketCreatorFactoryJUnitTest.testNewSSLConfigSSLComponentLocator(SocketCreatorFactoryJUnitTest.java:106)
The method testNewSSLConfigSSLComponentLocator is the first
in SocketCreatorFactoryJUnitTest to execute. And because a previous unit
test initialized the singleton either directly or indirectly without
cleaning it up in tearDown, it pollutes the JVM for later tests. The only
later unit test that is affected seems to be SocketCreatorFactoryJUnitTest.
Now you could easily fix SocketCreatorFactoryJUnitTest by adding a new
setUp() method:
*@Before*
*public void setUp() throws Exception {*
* SocketCreatorFactory.close();*
*}*
@After
public void tearDown() throws Exception {
SocketCreatorFactory.close();
}
*This change will fix this specific symptom, but not the underlying problem
-- which is "previous unit test initialized SocketCreatorFactory without
cleaning it up". *
*You can see why it's a problem if a unit test fails to cleanup EVERYTHING
that it setup. And you can why singletons (especially internal non-User API
singletons) are so dangerous for maintaining a clean GREEN CI.*
Cheers,
Kirk
Re: Dangers of using sockets in unit tests
Posted by Kirk Lund <kl...@apache.org>.
Yes, that's correct, SocketCreatorFactoryJUnitTest does not actually use
any sockets. I assume some unit test that ran before it used sockets. What
I really mean to say is: (1) singletons are bad, please let's keep working
to get rid of them, (2) please be careful with unit tests -- I continue to
occasionally find a unit test that creates a full cache or touches the file
system. I'm not actually picking on SocketCreatorFactoryJUnitTest --
apologies if that was the interpretation.
On Mon, Apr 27, 2020 at 2:02 PM Bruce Schuchardt <bs...@pivotal.io>
wrote:
> This seems more like it should be another call to remove all singletons.
> Tshe SocketCreatorFactoryJUnitTest that failed doesn't create a socket. It
> just configures a SocketCreator and then asserts that it was correctly
> configured.
>
> On 4/27/20, 1:58 PM, "Kirk Lund" <kl...@pivotal.io> wrote:
>
> This test started failing consistently on Windows (5 builds in a row so
> far!):
>
> org.apache.geode.internal.net.SocketCreatorFactoryJUnitTest >
> testNewSSLConfigSSLComponentLocator 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.internal.net
> .SocketCreatorFactoryJUnitTest.testNewSSLConfigSSLComponentLocator(SocketCreatorFactoryJUnitTest.java:106)
>
> The method testNewSSLConfigSSLComponentLocator is the first
> in SocketCreatorFactoryJUnitTest to execute. And because a previous
> unit
> test initialized the singleton either directly or indirectly without
> cleaning it up in tearDown, it pollutes the JVM for later tests. The
> only
> later unit test that is affected seems to be
> SocketCreatorFactoryJUnitTest.
>
> Now you could easily fix SocketCreatorFactoryJUnitTest by adding a new
> setUp() method:
>
>
> *@Before*
> *public void setUp() throws Exception {*
> * SocketCreatorFactory.close();*
> *}*
>
> @After
> public void tearDown() throws Exception {
> SocketCreatorFactory.close();
> }
>
> *This change will fix this specific symptom, but not the underlying
> problem
> -- which is "previous unit test initialized SocketCreatorFactory
> without
> cleaning it up". *
>
> *You can see why it's a problem if a unit test fails to cleanup
> EVERYTHING
> that it setup. And you can why singletons (especially internal
> non-User API
> singletons) are so dangerous for maintaining a clean GREEN CI.*
>
> Cheers,
> Kirk
>
>
>
>
Re: Dangers of using sockets in unit tests
Posted by Bruce Schuchardt <bs...@pivotal.io>.
This seems more like it should be another call to remove all singletons. Tshe SocketCreatorFactoryJUnitTest that failed doesn't create a socket. It just configures a SocketCreator and then asserts that it was correctly configured.
On 4/27/20, 1:58 PM, "Kirk Lund" <kl...@pivotal.io> wrote:
This test started failing consistently on Windows (5 builds in a row so
far!):
org.apache.geode.internal.net.SocketCreatorFactoryJUnitTest >
testNewSSLConfigSSLComponentLocator 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.internal.net.SocketCreatorFactoryJUnitTest.testNewSSLConfigSSLComponentLocator(SocketCreatorFactoryJUnitTest.java:106)
The method testNewSSLConfigSSLComponentLocator is the first
in SocketCreatorFactoryJUnitTest to execute. And because a previous unit
test initialized the singleton either directly or indirectly without
cleaning it up in tearDown, it pollutes the JVM for later tests. The only
later unit test that is affected seems to be SocketCreatorFactoryJUnitTest.
Now you could easily fix SocketCreatorFactoryJUnitTest by adding a new
setUp() method:
*@Before*
*public void setUp() throws Exception {*
* SocketCreatorFactory.close();*
*}*
@After
public void tearDown() throws Exception {
SocketCreatorFactory.close();
}
*This change will fix this specific symptom, but not the underlying problem
-- which is "previous unit test initialized SocketCreatorFactory without
cleaning it up". *
*You can see why it's a problem if a unit test fails to cleanup EVERYTHING
that it setup. And you can why singletons (especially internal non-User API
singletons) are so dangerous for maintaining a clean GREEN CI.*
Cheers,
Kirk
Re: Dangers of using sockets in unit tests
Posted by Kirk Lund <kl...@apache.org>.
The UpgradeTest job refuses to pass on PR #5011. As far I can tell,
changing a few tests from UnitTest to IntegrationTest should NOT cause
UpgradeTest to fail. It's the Tomcat Session upgrade tests and they just
seem to fail every single time in this PR. Again, dunno what to do with
this. Anyone familiar with the Tomcat Session upgrade tests want to take a
look?
On Mon, Apr 27, 2020 at 4:25 PM Kirk Lund <kl...@pivotal.io> wrote:
> The problems in WindowsUnitTest jobs in CI are caused by various unit
> tests that are either bugged or should be moved to IntegrationTests.
>
> 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 so many 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
>
> I filed a PR to fix all of the above issues:
> https://github.com/apache/geode/pull/5011
>
> Thanks,
> Kirk
>
> On Mon, Apr 27, 2020 at 1:58 PM Kirk Lund <kl...@pivotal.io> wrote:
>
>> This test started failing consistently on Windows (5 builds in a row so
>> far!):
>>
>> org.apache.geode.internal.net.SocketCreatorFactoryJUnitTest >
>> testNewSSLConfigSSLComponentLocator 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.internal.net.SocketCreatorFactoryJUnitTest.testNewSSLConfigSSLComponentLocator(SocketCreatorFactoryJUnitTest.java:106)
>>
>> The method testNewSSLConfigSSLComponentLocator is the first
>> in SocketCreatorFactoryJUnitTest to execute. And because a previous unit
>> test initialized the singleton either directly or indirectly without
>> cleaning it up in tearDown, it pollutes the JVM for later tests. The only
>> later unit test that is affected seems to be SocketCreatorFactoryJUnitTest.
>>
>> Now you could easily fix SocketCreatorFactoryJUnitTest by adding a new
>> setUp() method:
>>
>>
>> *@Before*
>> *public void setUp() throws Exception {*
>> * SocketCreatorFactory.close();*
>> *}*
>>
>> @After
>> public void tearDown() throws Exception {
>> SocketCreatorFactory.close();
>> }
>>
>> *This change will fix this specific symptom, but not the underlying
>> problem -- which is "previous unit test initialized SocketCreatorFactory
>> without cleaning it up". *
>>
>> *You can see why it's a problem if a unit test fails to cleanup
>> EVERYTHING that it setup. And you can why singletons (especially internal
>> non-User API singletons) are so dangerous for maintaining a clean GREEN CI.*
>>
>> Cheers,
>> Kirk
>>
>>
Re: Dangers of using sockets in unit tests
Posted by Kirk Lund <kl...@pivotal.io>.
The problems in WindowsUnitTest jobs in CI are caused by various unit tests
that are either bugged or should be moved to IntegrationTests.
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 so many 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
I filed a PR to fix all of the above issues:
https://github.com/apache/geode/pull/5011
Thanks,
Kirk
On Mon, Apr 27, 2020 at 1:58 PM Kirk Lund <kl...@pivotal.io> wrote:
> This test started failing consistently on Windows (5 builds in a row so
> far!):
>
> org.apache.geode.internal.net.SocketCreatorFactoryJUnitTest >
> testNewSSLConfigSSLComponentLocator 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.internal.net.SocketCreatorFactoryJUnitTest.testNewSSLConfigSSLComponentLocator(SocketCreatorFactoryJUnitTest.java:106)
>
> The method testNewSSLConfigSSLComponentLocator is the first
> in SocketCreatorFactoryJUnitTest to execute. And because a previous unit
> test initialized the singleton either directly or indirectly without
> cleaning it up in tearDown, it pollutes the JVM for later tests. The only
> later unit test that is affected seems to be SocketCreatorFactoryJUnitTest.
>
> Now you could easily fix SocketCreatorFactoryJUnitTest by adding a new
> setUp() method:
>
>
> *@Before*
> *public void setUp() throws Exception {*
> * SocketCreatorFactory.close();*
> *}*
>
> @After
> public void tearDown() throws Exception {
> SocketCreatorFactory.close();
> }
>
> *This change will fix this specific symptom, but not the underlying
> problem -- which is "previous unit test initialized SocketCreatorFactory
> without cleaning it up". *
>
> *You can see why it's a problem if a unit test fails to cleanup EVERYTHING
> that it setup. And you can why singletons (especially internal non-User API
> singletons) are so dangerous for maintaining a clean GREEN CI.*
>
> Cheers,
> Kirk
>
>