Agree that the normal use if properties is null would be to call one of the other startServer methods. My concern was that there’s nothing that enforces that.
Adding a null check is good. Ken > On Feb 14, 2017, at 10:48 AM, Jinmei Liao <jil...@pivotal.io> wrote: > > > >> On Feb. 14, 2017, 6:37 p.m., Ken Howe wrote: >>> geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java, >>> lines 94-99 >>> <https://reviews.apache.org/r/56637/diff/1/?file=1632825#file1632825line94> >>> >>> Looks like a null properties argument will cause an NPE > > Usually when user call this method, he should have a non-null property > object, otherwise, he would call the other vairiation of the startServer > without the property argument. But I can put a null check here. > > > - Jinmei > > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56637/#review165541 > ----------------------------------------------------------- > > > On Feb. 14, 2017, 4:19 p.m., Jinmei Liao wrote: >> >> ----------------------------------------------------------- >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/56637/ >> ----------------------------------------------------------- >> >> (Updated Feb. 14, 2017, 4:19 p.m.) >> >> >> Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, and Kirk >> Lund. >> >> >> Repository: geode >> >> >> Description >> ------- >> >> refactor ServerStarterRule and LocatorStarterRule so that they can be >> created without a Properties first. >> >> * this would ease the usage of thsee rules, so that it can always be used as >> a rule and users don't have to worry about stopping the locator/server >> manually. >> >> >> Diffs >> ----- >> >> >> geode-assembly/src/test/java/org/apache/geode/rest/internal/web/RestSecurityWithSSLTest.java >> 2fd6771e24a0a1c2b0450348845dbd9ee36e4d38 >> >> geode-core/src/test/java/org/apache/geode/management/internal/security/CacheServerStartupRule.java >> 07e82c319dac25a3d0997d5ea9419127db171487 >> >> geode-core/src/test/java/org/apache/geode/management/internal/security/MemberMBeanSecurityJUnitTest.java >> 33b32e9f720319b9ddbf70d5ab21c688897e1210 >> >> geode-core/src/test/java/org/apache/geode/security/AbstractSecureServerDUnitTest.java >> d202b768f14d6ada6c94700df417b733a49c8d8b >> >> geode-core/src/test/java/org/apache/geode/security/ClusterConfigWithoutSecurityDUnitTest.java >> 23ffa9a9e5eb403742d31db042b3d0b59f095400 >> >> geode-core/src/test/java/org/apache/geode/security/SecurityClusterConfigDUnitTest.java >> fcda2a322e31f5bcd00f922adb9fe7332e2073ef >> >> geode-core/src/test/java/org/apache/geode/security/SecurityWithoutClusterConfigDUnitTest.java >> 3add1abe0c14b7948716bce44c0ad7b4dbebd2ea >> >> geode-core/src/test/java/org/apache/geode/security/StartServerAuthorizationTest.java >> 8a797022b05c5e3d24599639b5a44830cf669f25 >> >> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java >> 0ac459c96cb6e588e016de67fa5b827e62fdcfa0 >> >> geode-core/src/test/java/org/apache/geode/test/dunit/rules/LocatorStarterRule.java >> 00a0f1e580d17417004e4462ff42c3364522550c >> >> geode-core/src/test/java/org/apache/geode/test/dunit/rules/ServerStarterRule.java >> f93498fcbcea0ec8d8f0b91a0367e9b6fb7d4ae1 >> geode-cq/src/test/java/org/apache/geode/security/CQClientAuthDunitTest.java >> 1aa275d0cda7b7dcf9de9fe2837d48491bebfcfa >> >> Diff: https://reviews.apache.org/r/56637/diff/ >> >> >> Testing >> ------- >> >> precheckin successful >> >> This code change will also fix the currently failing integration tests. See >> the change in MemberMBeanSecurityJUnitTest.java >> >> >> Thanks, >> >> Jinmei Liao >> >> >
