> On April 13, 2017, 7:13 p.m., Kirk Lund wrote: > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigBaseTest.java > > Lines 62 (patched) > > <https://reviews.apache.org/r/58393/diff/1/?file=1691037#file1691037line62> > > > > 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?
I made this more robust and added it to `LocatorServerStarterRule` as optional functionality. > On April 13, 2017, 7:13 p.m., Kirk Lund wrote: > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java > > Line 38 (original), 40 (patched) > > <https://reviews.apache.org/r/58393/diff/1/?file=1691038#file1691038line40> > > > > 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). Thanks for pointing this out. - Jared ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/58393/#review171910 ----------------------------------------------------------- On April 13, 2017, 11:06 p.m., Jared Stewart wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/58393/ > ----------------------------------------------------------- > > (Updated April 13, 2017, 11: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 304a1c4 > geode-assembly/src/test/resources/expected_jars.txt b7d1dc2 > geode-core/build.gradle 757599a > geode-core/src/main/java/org/apache/geode/internal/ClassPathLoader.java > 9ab7c30 > geode-core/src/main/java/org/apache/geode/internal/DeployedJar.java > PRE-CREATION > > geode-core/src/main/java/org/apache/geode/internal/InternalDataSerializer.java > f4f4069 > geode-core/src/main/java/org/apache/geode/internal/JarClassLoader.java > 9cd0589 > geode-core/src/main/java/org/apache/geode/internal/JarDeployer.java 18d4b42 > > geode-core/src/main/java/org/apache/geode/internal/cache/ClusterConfigurationLoader.java > f904af1 > > geode-core/src/main/java/org/apache/geode/internal/cache/GemFireCacheImpl.java > 08f916b > > geode-core/src/main/java/org/apache/geode/internal/cache/persistence/BackupManager.java > d052551 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/DeployFunction.java > 5f1f161 > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/ListDeployedFunction.java > 8df24db > > geode-core/src/main/java/org/apache/geode/management/internal/cli/functions/UndeployFunction.java > 3f05082 > > geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderIntegrationTest.java > c52d575 > geode-core/src/test/java/org/apache/geode/internal/ClassPathLoaderTest.java > 6baddaf > > geode-core/src/test/java/org/apache/geode/internal/DeployedJarJUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/JarClassLoaderJUnitTest.java > adc1d2e > > geode-core/src/test/java/org/apache/geode/internal/JarDeployerDUnitTest.java > a365899 > > geode-core/src/test/java/org/apache/geode/internal/JarDeployerIntegrationTest.java > 71daecc > > geode-core/src/test/java/org/apache/geode/internal/cache/IncrementalBackupDUnitTest.java > 0cc003e > > 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 > 7b0823b > > geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/UserCommandsDUnitTest.java > 24f5cdb > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfig.java > c3d7f5e > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigBaseTest.java > cecc8cf > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigDeployJarDUnitTest.java > 7cc84d6 > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigImportDUnitTest.java > 696d22c > > geode-core/src/test/java/org/apache/geode/management/internal/configuration/ClusterConfigStartMemberDUnitTest.java > 652ec60 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java > 02bad30 > > geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorServerStartupRule.java > 7350241 > > geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/CommandOverHttpDUnitTest.java > 5f4cd74 > gradle/dependency-versions.properties da8bdf2 > > > Diff: https://reviews.apache.org/r/58393/diff/3/ > > > Testing > ------- > > - Precheckin passed > - Manually deployed jars and executed functions via gfsh > > > Thanks, > > Jared Stewart > >
