----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58393/#review171910 -----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java Lines 55 (patched) <https://reviews.apache.org/r/58393/#comment244899> We should fix these on Windows or use org.junit.Assume to prevent running these tests on that OS: ```java @Test public void myTest() throws Exception { assumeFalse(SystemUtils.isWindows()); ... } ``` geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java Lines 65 (patched) <https://reviews.apache.org/r/58393/#comment244901> In general, I would recommend moving the setting of vars like this into setup() so you can see all test state centralized in setup(). geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java Lines 104 (patched) <https://reviews.apache.org/r/58393/#comment244902> Delete all try-catch blocks from these tests. Let JUnit deal with them. geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java Lines 122 (patched) <https://reviews.apache.org/r/58393/#comment244903> Replace all try-fail-catch patterns with AssertJ's assertThatThrownBy or Catch-Exception (I prefer AssertJ). geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java Lines 367 (patched) <https://reviews.apache.org/r/58393/#comment244904> Change all test method signatures to throws Exception and delete all try-catch structures for unexpected Exceptions. geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java Lines 500 (patched) <https://reviews.apache.org/r/58393/#comment244905> I think I reviewed this code previously with some additional tips on using Executor instead of Thread and getting rid of all try-catch blocks. geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java Lines 97 (patched) <https://reviews.apache.org/r/58393/#comment244909> One more variable for the next two changes: ```java volatile isJarDeployed = false; ``` geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java Line 130 (original), 112 (patched) <https://reviews.apache.org/r/58393/#comment244908> Insert another line here to set a boolean true which await() will test for a few lines later: ```java isJarDeployed = true; ``` geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java Line 144 (original), 116 (patched) <https://reviews.apache.org/r/58393/#comment244906> Thread.sleep should be replaced with Awaitility: ```java await().atMost(2, MINUTES).until(() -> assertThat(isJarDeployed).isTrue()); ``` geode-core/src/test/java/org/apache/geode/internal/cache/IncrementalBackupDUnitTest.java Lines 1106 (patched) <https://reviews.apache.org/r/58393/#comment244911> Don't use Java assertions in tests. Change this to assertTrue or assertThat().isTrue. geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandRedeployDUnitTest.java Lines 47 (patched) <https://reviews.apache.org/r/58393/#comment244912> Recommend renaming these constants in UPPERCASE. geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandRedeployDUnitTest.java Lines 115 (patched) <https://reviews.apache.org/r/58393/#comment244913> Recommend doing this in a test/resources file next time instead of concatenating Strings. I would still be good to consider moving this to a resource. geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigBaseTest.java Line 33 (original), 34 (patched) <https://reviews.apache.org/r/58393/#comment244916> I would change this class to abstract. Also rename to ClusterConfigTestBase or checkMissedTests gradle task is going to fail (or it should if it's implemented correctly). geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigBaseTest.java Lines 62 (patched) <https://reviews.apache.org/r/58393/#comment244915> You might want to consider changing this to a loop from zero to getHost(0).getVMCount(). It might be unlikely right now, but it's possible someone might write a DUnit test that executes before this test which expands the VMCount beyond the default 4 -- unless extra VMs won't impact these tests that extend this class? geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java Line 38 (original), 40 (patched) <https://reviews.apache.org/r/58393/#comment244917> Having seen super.setUp() abused in horrible ways, I recommend avoiding this at all costs. You already have a hint of this abuse in this example -- this before bounces 3 VMs and then invokes super.before which bounches those 3 AGAIN and bounces the 4th one. Change the super class to have: ```java public void beforeClusterConfigTestBase() throws Exception { ... } ``` Now subclass can create either before() or beforeClusterConfigDeployJarDUnitTest(). The superclass @Before method(s) are always executed before the subclass @Before method(s). geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java Lines 199 (patched) <https://reviews.apache.org/r/58393/#comment244922> Is this a Geode bug? If so then file a Jira ticket and add that GEODE-nn here. - Kirk Lund On April 12, 2017, 6:06 p.m., Jared Stewart wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58393/ > ----------------------------------------------------------- > > (Updated April 12, 2017, 6:06 p.m.) > > > Review request for geode, Jinmei Liao, Ken Howe, Kirk Lund, and Patrick > Rhomberg. > > > Repository: geode > > > Description > ------- > > GEODE-2686: Remove JarClassLoader > - Remove JarClassLoader > - Replace ClassPathLoader's collection of JarClassLoaders with a single > URLClassLoader > - Change naming scheme for deployed jars from 'vf.gf#myJar.jar#1' to > 'myJar.v1.jar' > > GEODE-2705: Jars undeployed from cluster configuration will not be loaded > from disk on member restart > > GEODE-2686: Add more logging to JarDeployer > > GEODE-2290: Limit scanning of deployed jars > - Uses fast-classpath-scanner to scan jars for classes containing Functions > without eagerly loading all classes in the jar. > > > Diffs > ----- > > geode-assembly/build.gradle 304a1c4d7ede5b82299c8a0f05038f3d11b2ac82 > geode-assembly/src/test/resources/expected_jars.txt > b7d1dc22fd7995e6f7786984994ce62e1064bed3 > geode-core/build.gradle 757599a9d76844dec17e545d1b4d19b32c22afef > geode-core/src/main/java/org/apache/geode/internal/ClassPathLoader.java > 9ab7c308579c9ea5c8fb300ee7550b3153507b1c > geode-core/src/main/java/org/apache/geode/internal/DeployedJar.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java > f4f40692d1574bb9eca9bf8f7d42ce64fab18f9b > geode-core/src/main/java/org/apache/geode/internal/JarClassLoader.java > 9cd05899898fc82eec87a3f0ab5ace31b67ef308 > geode-core/src/main/java/org/apache/geode/internal/JarDeployer.java > 18d4b42213f29761792d35f12eddfb1cbfbba081 > > geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java > f904af188eb295f1cb84e41023de168c160ed07f > > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java > 08f916b117a07ed303638366c64a2c9d5b807b91 > > geode-core/src/main/java/org/apache/geode/internal/cache/persistence/BackupManager.java > d052551d85e5b01b16255160f76795e829a14429 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DeployFunction.java > 5f1f16167e08dd79b2995555c42541fb01ee2546 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ListDeployedFunction.java > 8df24dbe35a08173932776226c9f20665db5e9a3 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/UndeployFunction.java > 3f0508270b51b4930c2eafe15547b9b258b96bd9 > > geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderIntegrationTest.java > c52d5754792cfeaed1bb87bdb9380a360308395a > geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderTest.java > 6baddaf4b9d3b58fce10fc224440111c2e88b849 > > geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/JarClassLoaderJUnitTest.java > adc1d2eb2b633d4c81076db2b4685fedb6315913 > > geode-core/src/test/java/org/apache/geode/internal/JarDeployerDUnitTest.java > a365899c46bb7f28ce75118e45137a243493cbd0 > > geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java > 71daeccc42e7944f01ce50d94fc3c4afc5c9b121 > > geode-core/src/test/java/org/apache/geode/internal/cache/IncrementalBackupDUnitTest.java > 0cc003ebd15628ea62a67a36c1ac595fe0a02693 > > geode-core/src/test/java/org/apache/geode/management/DeployJarTestSuite.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandRedeployDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/DeployCommandsDUnitTest.java > 7b0823bea38043967502e0accee90963d5d7b650 > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/UserCommandsDUnitTest.java > 24f5cdb549fbc172e12f965044681e83f11b5829 > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfig.java > c3d7f5ec2483b4ae28f73e3b3477180d20b1a53e > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigBaseTest.java > cecc8cfd683a2fb24d621c9010d5650ef2996966 > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java > 7cc84d6c4417872daf9a13ea5f237e458cafe154 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > 02bad30e97f0a3550eac0bf00ce0c6833d761dae > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpDUnitTest.java > 5f4cd74b8af81ae3bbeff04d6ce44a5c4ac9ec59 > > > Diff: https://reviews.apache.org/r/58393/diff/1/ > > > Testing > ------- > > - Precheckin passed > - Manually deployed jars and executed functions via gfsh > > > Thanks, > > Jared Stewart > >
