Re: Review Request 59326: GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses

2017-05-17 Thread Kirk Lund 


> On May 17, 2017, 5:20 p.m., Patrick Rhomberg wrote:
> > geode-core/src/test/java/org/apache/geode/internal/lang/SystemUtilsJUnitTest.java
> > Lines 17-18 (original), 17-18 (patched)
> > 
> >
> > Are star-imports as frowned upon in Java as they are in Python?  
> > IntelliJ suggests optimal imports as...
> > 
> > ```
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.APPLE_JVM_VENDOR_NAME;
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.IBM_J9_JVM_NAME;
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.JAVA_HOTSPOT_JVM_NAME;
> > import static org.apache.geode.internal.lang.SystemUtils.LINUX_OS_NAME;
> > import static org.apache.geode.internal.lang.SystemUtils.MAC_OSX_NAME;
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.ORACLE_JROCKIT_JVM_NAME;
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.ORACLE_JVM_VENDOR_NAME;
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.WINDOWS_OS_NAME;
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.getBootClassPath;
> > import static org.apache.geode.internal.lang.SystemUtils.getClassPath;
> > import static org.apache.geode.internal.lang.SystemUtils.getJavaVersion;
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.getOsArchitecture;
> > import static org.apache.geode.internal.lang.SystemUtils.getOsName;
> > import static org.apache.geode.internal.lang.SystemUtils.getOsVersion;
> > import static org.apache.geode.internal.lang.SystemUtils.isAppleJVM;
> > import static org.apache.geode.internal.lang.SystemUtils.isHotSpotVM;
> > import static org.apache.geode.internal.lang.SystemUtils.isJ9VM;
> > import static org.apache.geode.internal.lang.SystemUtils.isJRockitVM;
> > import static 
> > org.apache.geode.internal.lang.SystemUtils.isJavaVersionAtLeast;
> > import static org.apache.geode.internal.lang.SystemUtils.isLinux;
> > import static org.apache.geode.internal.lang.SystemUtils.isMacOSX;
> > import static org.apache.geode.internal.lang.SystemUtils.isOracleJVM;
> > import static org.apache.geode.internal.lang.SystemUtils.isWindows;
> > import static org.assertj.core.api.Assertions.assertThat;
> > import static org.junit.Assert.assertEquals;
> > import static org.junit.Assert.assertFalse;
> > import static org.junit.Assert.assertTrue;
> > import static org.junit.Assume.assumeNotNull;
> > 
> > import org.apache.geode.test.junit.categories.UnitTest;
> > import org.junit.Test;
> > import org.junit.experimental.categories.Category;
> > 
> > import java.lang.management.ManagementFactory;
> > ```

"import package.*" is frowned on, but "import static package.Class.*" generally 
is not because it's the "import static" equivalent of "import package.Class" -- 
spotless doesn't seem to care either way


- Kirk


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59326/#review175277
---


On May 17, 2017, 1:10 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59326/
> ---
> 
> (Updated May 17, 2017, 1:10 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick 
> Rhomberg.
> 
> 
> Bugs: GEODE-2934
> https://issues.apache.org/jira/browse/GEODE-2934
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses
> 
> Subclasses now only need to override the method "String getModuleName()" and 
> return "geode-xxx" where "xxx" is the name of the geode module.
> 
> Eliminate lots of duplicated code, use SystemUtils for Java version check. 
> Convert statics to member variables. Move setup code from tests to the 
> @Before method. Use static imports to reduce line lengths. Generally make it 
> easier to read and debug (this test gave me some issues during recent 
> refactoring).
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 
> 3485ede59 
>   
> geode-core/src/test/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTest.java
>  5b78f065f 
>   
> geode-core/src/test/java/org/apache/geode/internal/lang/SystemUtilsJUnitTest.java
>  8a83dc082 
>   
> geode-cq/src/test/java/org/apache/geode/codeAnalysis/AnalyzeCQSerializablesJUnitTest.java
>  747dd8b4e 
>   
> geode-wan/src/test/java/org/apache/geode/codeAnalysis/AnalyzeWANSerializablesJUnitTest.java
>  77c541cbd 
> 
> 
> Diff: https://reviews.apache.org/r/59326/diff/1/
> 
> 
> Testing
> ---
>

Re: Review Request 59326: GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses

2017-05-17 Thread Patrick Rhomberg 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59326/#review175277
---




geode-core/src/test/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTest.java
Lines 117 (patched)


discrepencies -> discrepancies



geode-core/src/test/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTest.java
Lines 137 (patched)


discrepencies -> discrepancies



geode-core/src/test/java/org/apache/geode/internal/lang/SystemUtilsJUnitTest.java
Lines 17-18 (original), 17-18 (patched)


Are star-imports as frowned upon in Java as they are in Python?  IntelliJ 
suggests optimal imports as...

```
import static 
org.apache.geode.internal.lang.SystemUtils.APPLE_JVM_VENDOR_NAME;
import static org.apache.geode.internal.lang.SystemUtils.IBM_J9_JVM_NAME;
import static 
org.apache.geode.internal.lang.SystemUtils.JAVA_HOTSPOT_JVM_NAME;
import static org.apache.geode.internal.lang.SystemUtils.LINUX_OS_NAME;
import static org.apache.geode.internal.lang.SystemUtils.MAC_OSX_NAME;
import static 
org.apache.geode.internal.lang.SystemUtils.ORACLE_JROCKIT_JVM_NAME;
import static 
org.apache.geode.internal.lang.SystemUtils.ORACLE_JVM_VENDOR_NAME;
import static org.apache.geode.internal.lang.SystemUtils.WINDOWS_OS_NAME;
import static org.apache.geode.internal.lang.SystemUtils.getBootClassPath;
import static org.apache.geode.internal.lang.SystemUtils.getClassPath;
import static org.apache.geode.internal.lang.SystemUtils.getJavaVersion;
import static org.apache.geode.internal.lang.SystemUtils.getOsArchitecture;
import static org.apache.geode.internal.lang.SystemUtils.getOsName;
import static org.apache.geode.internal.lang.SystemUtils.getOsVersion;
import static org.apache.geode.internal.lang.SystemUtils.isAppleJVM;
import static org.apache.geode.internal.lang.SystemUtils.isHotSpotVM;
import static org.apache.geode.internal.lang.SystemUtils.isJ9VM;
import static org.apache.geode.internal.lang.SystemUtils.isJRockitVM;
import static 
org.apache.geode.internal.lang.SystemUtils.isJavaVersionAtLeast;
import static org.apache.geode.internal.lang.SystemUtils.isLinux;
import static org.apache.geode.internal.lang.SystemUtils.isMacOSX;
import static org.apache.geode.internal.lang.SystemUtils.isOracleJVM;
import static org.apache.geode.internal.lang.SystemUtils.isWindows;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assume.assumeNotNull;

import org.apache.geode.test.junit.categories.UnitTest;
import org.junit.Test;
import org.junit.experimental.categories.Category;

import java.lang.management.ManagementFactory;
```


- Patrick Rhomberg


On May 17, 2017, 1:10 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59326/
> ---
> 
> (Updated May 17, 2017, 1:10 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick 
> Rhomberg.
> 
> 
> Bugs: GEODE-2934
> https://issues.apache.org/jira/browse/GEODE-2934
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses
> 
> Subclasses now only need to override the method "String getModuleName()" and 
> return "geode-xxx" where "xxx" is the name of the geode module.
> 
> Eliminate lots of duplicated code, use SystemUtils for Java version check. 
> Convert statics to member variables. Move setup code from tests to the 
> @Before method. Use static imports to reduce line lengths. Generally make it 
> easier to read and debug (this test gave me some issues during recent 
> refactoring).
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 
> 3485ede59 
>   
> geode-core/src/test/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTest.java
>  5b78f065f 
>   
> geode-core/src/test/java/org/apache/geode/internal/lang/SystemUtilsJUnitTest.java
>  8a83dc082 
>   
> geode-cq/src/test/java/org/apache/geode/codeAnalysis/AnalyzeCQSerializablesJUnitTest.java
>  747dd8b4e 
>   
> geode-wan/src/test/java/org/apache/geode/codeAnalysis/AnalyzeWANSerializablesJUnitTest.java
>  77c541cbd 
> 
> 
> Diff: https://reviews.apache.org/r/59326/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin in progress
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 59326: GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses

2017-05-17 Thread Jinmei Liao 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59326/#review175276
---


Ship it!




Ship It!

- Jinmei Liao


On May 17, 2017, 1:10 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59326/
> ---
> 
> (Updated May 17, 2017, 1:10 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick 
> Rhomberg.
> 
> 
> Bugs: GEODE-2934
> https://issues.apache.org/jira/browse/GEODE-2934
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses
> 
> Subclasses now only need to override the method "String getModuleName()" and 
> return "geode-xxx" where "xxx" is the name of the geode module.
> 
> Eliminate lots of duplicated code, use SystemUtils for Java version check. 
> Convert statics to member variables. Move setup code from tests to the 
> @Before method. Use static imports to reduce line lengths. Generally make it 
> easier to read and debug (this test gave me some issues during recent 
> refactoring).
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 
> 3485ede59 
>   
> geode-core/src/test/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTest.java
>  5b78f065f 
>   
> geode-core/src/test/java/org/apache/geode/internal/lang/SystemUtilsJUnitTest.java
>  8a83dc082 
>   
> geode-cq/src/test/java/org/apache/geode/codeAnalysis/AnalyzeCQSerializablesJUnitTest.java
>  747dd8b4e 
>   
> geode-wan/src/test/java/org/apache/geode/codeAnalysis/AnalyzeWANSerializablesJUnitTest.java
>  77c541cbd 
> 
> 
> Diff: https://reviews.apache.org/r/59326/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin in progress
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 59326: GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses

2017-05-17 Thread Jared Stewart 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59326/#review175274
---




geode-core/src/test/java/org/apache/geode/internal/lang/SystemUtilsJUnitTest.java
Lines 19 (patched)


You can remove this import


- Jared Stewart


On May 17, 2017, 1:10 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59326/
> ---
> 
> (Updated May 17, 2017, 1:10 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick 
> Rhomberg.
> 
> 
> Bugs: GEODE-2934
> https://issues.apache.org/jira/browse/GEODE-2934
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses
> 
> Subclasses now only need to override the method "String getModuleName()" and 
> return "geode-xxx" where "xxx" is the name of the geode module.
> 
> Eliminate lots of duplicated code, use SystemUtils for Java version check. 
> Convert statics to member variables. Move setup code from tests to the 
> @Before method. Use static imports to reduce line lengths. Generally make it 
> easier to read and debug (this test gave me some issues during recent 
> refactoring).
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 
> 3485ede59 
>   
> geode-core/src/test/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTest.java
>  5b78f065f 
>   
> geode-core/src/test/java/org/apache/geode/internal/lang/SystemUtilsJUnitTest.java
>  8a83dc082 
>   
> geode-cq/src/test/java/org/apache/geode/codeAnalysis/AnalyzeCQSerializablesJUnitTest.java
>  747dd8b4e 
>   
> geode-wan/src/test/java/org/apache/geode/codeAnalysis/AnalyzeWANSerializablesJUnitTest.java
>  77c541cbd 
> 
> 
> Diff: https://reviews.apache.org/r/59326/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin in progress
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 59326: GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses

2017-05-17 Thread Jared Stewart 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59326/#review175275
---


Ship it!




Ship It!

- Jared Stewart


On May 17, 2017, 1:10 a.m., Kirk Lund wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59326/
> ---
> 
> (Updated May 17, 2017, 1:10 a.m.)
> 
> 
> Review request for geode, Jinmei Liao, Jared Stewart, Ken Howe, and Patrick 
> Rhomberg.
> 
> 
> Bugs: GEODE-2934
> https://issues.apache.org/jira/browse/GEODE-2934
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> GEODE-2934: refactor AnalyzeSerializablesJUnitTest and subclasses
> 
> Subclasses now only need to override the method "String getModuleName()" and 
> return "geode-xxx" where "xxx" is the name of the geode module.
> 
> Eliminate lots of duplicated code, use SystemUtils for Java version check. 
> Convert statics to member variables. Move setup code from tests to the 
> @Before method. Use static imports to reduce line lengths. Generally make it 
> easier to read and debug (this test gave me some issues during recent 
> refactoring).
> 
> 
> Diffs
> -
> 
>   geode-core/src/main/java/org/apache/geode/internal/lang/SystemUtils.java 
> 3485ede59 
>   
> geode-core/src/test/java/org/apache/geode/codeAnalysis/AnalyzeSerializablesJUnitTest.java
>  5b78f065f 
>   
> geode-core/src/test/java/org/apache/geode/internal/lang/SystemUtilsJUnitTest.java
>  8a83dc082 
>   
> geode-cq/src/test/java/org/apache/geode/codeAnalysis/AnalyzeCQSerializablesJUnitTest.java
>  747dd8b4e 
>   
> geode-wan/src/test/java/org/apache/geode/codeAnalysis/AnalyzeWANSerializablesJUnitTest.java
>  77c541cbd 
> 
> 
> Diff: https://reviews.apache.org/r/59326/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin in progress
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>