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 <ki...@gmail.com> on 2015/12/07 18:25:32 UTC
Review Request 41044: Update process stream handling in tests that
fork JVMs
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41044/
-----------------------------------------------------------
Review request for geode, Darrel Schneider and Jens Deppe.
Bugs: GEODE-291
https://issues.apache.org/jira/browse/GEODE-291
Repository: geode
Description
-------
Add build dependencies for testing frameworks Awaitility and Catch-Exception. Upgrade AssertJ build dependency.
Refactor DistributedSystem PROPERTY_FILE constants, system property and APIs to facilitate automated testing. Create UnitTest and IntegrationTest for DistributedSystem.
Tighten up ProcessWrapper, ProcessStreamReader and ProcessOutputReader to eliminate intermittent failures and unwanted stderr output in Jenkins.
Extract integration tests from launcher UnitTests to new IntegrationTests.
Convert tests in LocatorLauncherIntegrationJUnitTest and ServerLauncherIntegrationJUnitTest to follow BDD style.
I will delete Geode291TestSuite before merging to develop.
Diffs
-----
build.gradle b5465b8
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/AbstractLauncher.java bfd3de7
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java e2fccd9
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherIntegrationJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherJUnitTest.java 399c78f
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/CommonLauncherTestSuite.java 94ba320
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemIntegrationJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherIntegrationJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherJUnitTest.java 003a098
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherIntegrationJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherJUnitTest.java 6884e58
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/Geode291TestSuite.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessOutputReader.java e99dceb
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessStreamReader.java f99f5f9
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java 2847b34
gradle/dependency-versions.properties 3e6b6a5
Diff: https://reviews.apache.org/r/41044/diff/
Testing
-------
new tests and precheckin
Thanks,
Kirk Lund
Re: Review Request 41044: Update process stream handling in tests
that fork JVMs
Posted by Kirk Lund <ki...@gmail.com>.
> On Dec. 10, 2015, 11:16 p.m., Jens Deppe wrote:
> > gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java, line 53
> > <https://reviews.apache.org/r/41044/diff/1/?file=1155207#file1155207line53>
> >
> > Can the timeouts be consolidated at all?
> >
> > Can the defaults be shortened? If something is actually wrong, a timeout of 10 minutes is a long time to wait to find out.
>
> Kirk Lund wrote:
> The current default is 5 minutes. This has been a gradually growing timeout because slow, overloaded machines timeout. This unfortunately is the problem with IntegrationTests and forking of JVMs on unknown machines.
The reason there were two constants is the pattern of havine a PROPERTY (the key for the System.getProperty) and a DEFAULT value (the default if the property is null). It was converted from System property to a field on the new Builder. I'm deleting the PROPERTY and will update the diff.
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41044/#review109886
-----------------------------------------------------------
On Dec. 7, 2015, 5:25 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41044/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2015, 5:25 p.m.)
>
>
> Review request for geode, Darrel Schneider and Jens Deppe.
>
>
> Bugs: GEODE-291
> https://issues.apache.org/jira/browse/GEODE-291
>
>
> Repository: geode
>
>
> Description
> -------
>
> Add build dependencies for testing frameworks Awaitility and Catch-Exception. Upgrade AssertJ build dependency.
>
> Refactor DistributedSystem PROPERTY_FILE constants, system property and APIs to facilitate automated testing. Create UnitTest and IntegrationTest for DistributedSystem.
>
> Tighten up ProcessWrapper, ProcessStreamReader and ProcessOutputReader to eliminate intermittent failures and unwanted stderr output in Jenkins.
>
> Extract integration tests from launcher UnitTests to new IntegrationTests.
>
> Convert tests in LocatorLauncherIntegrationJUnitTest and ServerLauncherIntegrationJUnitTest to follow BDD style.
>
> I will delete Geode291TestSuite before merging to develop.
>
>
> Diffs
> -----
>
> build.gradle b5465b8
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/AbstractLauncher.java bfd3de7
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java e2fccd9
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherJUnitTest.java 399c78f
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/CommonLauncherTestSuite.java 94ba320
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherJUnitTest.java 003a098
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherJUnitTest.java 6884e58
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/Geode291TestSuite.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessOutputReader.java e99dceb
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessStreamReader.java f99f5f9
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java 2847b34
> gradle/dependency-versions.properties 3e6b6a5
>
> Diff: https://reviews.apache.org/r/41044/diff/
>
>
> Testing
> -------
>
> new tests and precheckin
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 41044: Update process stream handling in tests
that fork JVMs
Posted by Kirk Lund <ki...@gmail.com>.
> On Dec. 10, 2015, 11:16 p.m., Jens Deppe wrote:
> > gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java, line 53
> > <https://reviews.apache.org/r/41044/diff/1/?file=1155207#file1155207line53>
> >
> > Can the timeouts be consolidated at all?
> >
> > Can the defaults be shortened? If something is actually wrong, a timeout of 10 minutes is a long time to wait to find out.
The current default is 5 minutes. This has been a gradually growing timeout because slow, overloaded machines timeout. This unfortunately is the problem with IntegrationTests and forking of JVMs on unknown machines.
> On Dec. 10, 2015, 11:16 p.m., Jens Deppe wrote:
> > gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java, line 52
> > <https://reviews.apache.org/r/41044/diff/1/?file=1155207#file1155207line52>
> >
> > Is this used?
This one can actually be deleted now. It's no longer used and I apparently missed that.
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41044/#review109886
-----------------------------------------------------------
On Dec. 7, 2015, 5:25 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41044/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2015, 5:25 p.m.)
>
>
> Review request for geode, Darrel Schneider and Jens Deppe.
>
>
> Bugs: GEODE-291
> https://issues.apache.org/jira/browse/GEODE-291
>
>
> Repository: geode
>
>
> Description
> -------
>
> Add build dependencies for testing frameworks Awaitility and Catch-Exception. Upgrade AssertJ build dependency.
>
> Refactor DistributedSystem PROPERTY_FILE constants, system property and APIs to facilitate automated testing. Create UnitTest and IntegrationTest for DistributedSystem.
>
> Tighten up ProcessWrapper, ProcessStreamReader and ProcessOutputReader to eliminate intermittent failures and unwanted stderr output in Jenkins.
>
> Extract integration tests from launcher UnitTests to new IntegrationTests.
>
> Convert tests in LocatorLauncherIntegrationJUnitTest and ServerLauncherIntegrationJUnitTest to follow BDD style.
>
> I will delete Geode291TestSuite before merging to develop.
>
>
> Diffs
> -----
>
> build.gradle b5465b8
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/AbstractLauncher.java bfd3de7
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java e2fccd9
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherJUnitTest.java 399c78f
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/CommonLauncherTestSuite.java 94ba320
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherJUnitTest.java 003a098
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherJUnitTest.java 6884e58
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/Geode291TestSuite.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessOutputReader.java e99dceb
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessStreamReader.java f99f5f9
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java 2847b34
> gradle/dependency-versions.properties 3e6b6a5
>
> Diff: https://reviews.apache.org/r/41044/diff/
>
>
> Testing
> -------
>
> new tests and precheckin
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 41044: Update process stream handling in tests
that fork JVMs
Posted by Jens Deppe <jd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41044/#review109886
-----------------------------------------------------------
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java (line 52)
<https://reviews.apache.org/r/41044/#comment169555>
Is this used?
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java (line 53)
<https://reviews.apache.org/r/41044/#comment169556>
Can the timeouts be consolidated at all?
Can the defaults be shortened? If something is actually wrong, a timeout of 10 minutes is a long time to wait to find out.
- Jens Deppe
On Dec. 7, 2015, 5:25 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41044/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2015, 5:25 p.m.)
>
>
> Review request for geode, Darrel Schneider and Jens Deppe.
>
>
> Bugs: GEODE-291
> https://issues.apache.org/jira/browse/GEODE-291
>
>
> Repository: geode
>
>
> Description
> -------
>
> Add build dependencies for testing frameworks Awaitility and Catch-Exception. Upgrade AssertJ build dependency.
>
> Refactor DistributedSystem PROPERTY_FILE constants, system property and APIs to facilitate automated testing. Create UnitTest and IntegrationTest for DistributedSystem.
>
> Tighten up ProcessWrapper, ProcessStreamReader and ProcessOutputReader to eliminate intermittent failures and unwanted stderr output in Jenkins.
>
> Extract integration tests from launcher UnitTests to new IntegrationTests.
>
> Convert tests in LocatorLauncherIntegrationJUnitTest and ServerLauncherIntegrationJUnitTest to follow BDD style.
>
> I will delete Geode291TestSuite before merging to develop.
>
>
> Diffs
> -----
>
> build.gradle b5465b8
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/AbstractLauncher.java bfd3de7
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java e2fccd9
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherJUnitTest.java 399c78f
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/CommonLauncherTestSuite.java 94ba320
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherJUnitTest.java 003a098
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherJUnitTest.java 6884e58
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/Geode291TestSuite.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessOutputReader.java e99dceb
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessStreamReader.java f99f5f9
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java 2847b34
> gradle/dependency-versions.properties 3e6b6a5
>
> Diff: https://reviews.apache.org/r/41044/diff/
>
>
> Testing
> -------
>
> new tests and precheckin
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 41044: Update process stream handling in tests
that fork JVMs
Posted by Jens Deppe <jd...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41044/#review110046
-----------------------------------------------------------
Ship it!
Ship It!
- Jens Deppe
On Dec. 10, 2015, 11:54 p.m., Kirk Lund wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41044/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2015, 11:54 p.m.)
>
>
> Review request for geode, Darrel Schneider and Jens Deppe.
>
>
> Bugs: GEODE-291
> https://issues.apache.org/jira/browse/GEODE-291
>
>
> Repository: geode
>
>
> Description
> -------
>
> Add build dependencies for testing frameworks Awaitility and Catch-Exception. Upgrade AssertJ build dependency.
>
> Refactor DistributedSystem PROPERTY_FILE constants, system property and APIs to facilitate automated testing. Create UnitTest and IntegrationTest for DistributedSystem.
>
> Tighten up ProcessWrapper, ProcessStreamReader and ProcessOutputReader to eliminate intermittent failures and unwanted stderr output in Jenkins.
>
> Extract integration tests from launcher UnitTests to new IntegrationTests.
>
> Convert tests in LocatorLauncherIntegrationJUnitTest and ServerLauncherIntegrationJUnitTest to follow BDD style.
>
> I will delete Geode291TestSuite before merging to develop.
>
>
> Diffs
> -----
>
> build.gradle b5465b8
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/AbstractLauncher.java bfd3de7
> gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java e2fccd9
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherJUnitTest.java 399c78f
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/CommonLauncherTestSuite.java 94ba320
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherJUnitTest.java 003a098
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherIntegrationJUnitTest.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherJUnitTest.java 6884e58
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/Geode291TestSuite.java PRE-CREATION
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessOutputReader.java e99dceb
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessStreamReader.java f99f5f9
> gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java 2847b34
> gradle/dependency-versions.properties 3e6b6a5
>
> Diff: https://reviews.apache.org/r/41044/diff/
>
>
> Testing
> -------
>
> new tests and precheckin
>
>
> Thanks,
>
> Kirk Lund
>
>
Re: Review Request 41044: Update process stream handling in tests
that fork JVMs
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/41044/
-----------------------------------------------------------
(Updated Dec. 10, 2015, 11:54 p.m.)
Review request for geode, Darrel Schneider and Jens Deppe.
Changes
-------
I deleted both constants in the review so that this is the only one remaining:
private static final long PROCESS_TIMEOUT_MILLIS = 10 * 60 * 1000L; // 10 minutes
Yes, I know 10 minutes is a long time but I haven't changed that value. If you want to try and shorten that AFTER I commit my changes then let's go for it but this started at 2 minutes and kept timing out forcing me to increase it to the current 10 minutes.
Bugs: GEODE-291
https://issues.apache.org/jira/browse/GEODE-291
Repository: geode
Description
-------
Add build dependencies for testing frameworks Awaitility and Catch-Exception. Upgrade AssertJ build dependency.
Refactor DistributedSystem PROPERTY_FILE constants, system property and APIs to facilitate automated testing. Create UnitTest and IntegrationTest for DistributedSystem.
Tighten up ProcessWrapper, ProcessStreamReader and ProcessOutputReader to eliminate intermittent failures and unwanted stderr output in Jenkins.
Extract integration tests from launcher UnitTests to new IntegrationTests.
Convert tests in LocatorLauncherIntegrationJUnitTest and ServerLauncherIntegrationJUnitTest to follow BDD style.
I will delete Geode291TestSuite before merging to develop.
Diffs (updated)
-----
build.gradle b5465b8
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/AbstractLauncher.java bfd3de7
gemfire-core/src/main/java/com/gemstone/gemfire/distributed/DistributedSystem.java e2fccd9
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherIntegrationJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/AbstractLauncherJUnitTest.java 399c78f
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/CommonLauncherTestSuite.java 94ba320
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemIntegrationJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/DistributedSystemJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherIntegrationJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/LocatorLauncherJUnitTest.java 003a098
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherIntegrationJUnitTest.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/distributed/ServerLauncherJUnitTest.java 6884e58
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/Geode291TestSuite.java PRE-CREATION
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessOutputReader.java e99dceb
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessStreamReader.java f99f5f9
gemfire-core/src/test/java/com/gemstone/gemfire/test/process/ProcessWrapper.java 2847b34
gradle/dependency-versions.properties 3e6b6a5
Diff: https://reviews.apache.org/r/41044/diff/
Testing
-------
new tests and precheckin
Thanks,
Kirk Lund