Re: Review Request 57514: GEODE-2267: enhance error output for gfsh.

2017-03-14 Thread Jared Stewart 

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


Ship it!




Ship It!

- Jared Stewart


On March 13, 2017, 5:32 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57514/
> ---
> 
> (Updated March 13, 2017, 5:32 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * correctly output error message if gfsh execution has an error
> * export logs should output correct log message over http connection as well
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
>  36d071c153ed8c2559cc8c29e248058b42cbf7ff 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
>  0351573f6eda80e1751bd6b95efdbe6ce36a620d 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportStatsDUnitTest.java
>  b7f8c2a080614ab59dc9da29c31f606749b58e4a 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/web/controllers/ExportLogControllerTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  f3674581d6e27bba8932294152cc2db8727a4024 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpDUnitTest.java
>  cc4ae28ff97fc50c3a9298ea3355847008776ec3 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpIntegrationTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57514/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin running ...
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 57514: GEODE-2267: enhance error output for gfsh.

2017-03-13 Thread Kirk Lund 

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


Fix it, then Ship it!




I think we renamed the tests but missed ExportLogsCommand.java.


geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
Line 53 (original), 53 (patched)


I thought we renamed this to ExportLogsCommand.java?


- Kirk Lund


On March 13, 2017, 5:32 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57514/
> ---
> 
> (Updated March 13, 2017, 5:32 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * correctly output error message if gfsh execution has an error
> * export logs should output correct log message over http connection as well
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
>  36d071c153ed8c2559cc8c29e248058b42cbf7ff 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
>  0351573f6eda80e1751bd6b95efdbe6ce36a620d 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportStatsDUnitTest.java
>  b7f8c2a080614ab59dc9da29c31f606749b58e4a 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/web/controllers/ExportLogControllerTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  f3674581d6e27bba8932294152cc2db8727a4024 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpDUnitTest.java
>  cc4ae28ff97fc50c3a9298ea3355847008776ec3 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpIntegrationTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57514/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin running ...
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 57514: GEODE-2267: enhance error output for gfsh.

2017-03-13 Thread Kirk Lund 


> On March 13, 2017, 5:47 p.m., Kirk Lund wrote:
> > I think we renamed the tests but missed ExportLogsCommand.java.

My review includes a Ship It! But the GUI is showing it under "Fix It!"


- Kirk


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


On March 13, 2017, 5:32 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57514/
> ---
> 
> (Updated March 13, 2017, 5:32 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * correctly output error message if gfsh execution has an error
> * export logs should output correct log message over http connection as well
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
>  36d071c153ed8c2559cc8c29e248058b42cbf7ff 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
>  0351573f6eda80e1751bd6b95efdbe6ce36a620d 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportStatsDUnitTest.java
>  b7f8c2a080614ab59dc9da29c31f606749b58e4a 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/web/controllers/ExportLogControllerTest.java
>  PRE-CREATION 
>   
> geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
>  f3674581d6e27bba8932294152cc2db8727a4024 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpDUnitTest.java
>  cc4ae28ff97fc50c3a9298ea3355847008776ec3 
>   
> geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpIntegrationTest.java
>  PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/57514/diff/2/
> 
> 
> Testing
> ---
> 
> precheckin running ...
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 57514: GEODE-2267: enhance error output for gfsh.

2017-03-13 Thread Jinmei Liao 

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

(Updated March 13, 2017, 5:32 p.m.)


Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

* correctly output error message if gfsh execution has an error
* export logs should output correct log message over http connection as well


Diffs (updated)
-

  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
 36d071c153ed8c2559cc8c29e248058b42cbf7ff 
  
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
 0351573f6eda80e1751bd6b95efdbe6ce36a620d 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsIntegrationTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportStatsDUnitTest.java
 b7f8c2a080614ab59dc9da29c31f606749b58e4a 
  
geode-core/src/test/java/org/apache/geode/management/internal/web/controllers/ExportLogControllerTest.java
 PRE-CREATION 
  
geode-core/src/test/java/org/apache/geode/test/dunit/rules/GfshShellConnectionRule.java
 f3674581d6e27bba8932294152cc2db8727a4024 
  
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpDUnitTest.java
 cc4ae28ff97fc50c3a9298ea3355847008776ec3 
  
geode-web/src/test/java/org/apache/geode/management/internal/cli/commands/ExportLogsOverHttpIntegrationTest.java
 PRE-CREATION 


Diff: https://reviews.apache.org/r/57514/diff/2/

Changes: https://reviews.apache.org/r/57514/diff/1-2/


Testing
---

precheckin running ...


Thanks,

Jinmei Liao

Re: Review Request 57514: GEODE-2267: enhance error output for gfsh.

2017-03-10 Thread Kirk Lund 

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



I really need to see UnitTests or IntegrationTests being written. In 
particular, we need to be focused on using Mockito to create new UnitTests. I'm 
available to pair on this if there are any questions about how to do this.

I really do not want to see any of these classes being changed further without 
new tests being written.


geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
Line 53 (original), 53 (patched)


Since the command is "export logs" can we please rename this class to 
ExportLogsCommand?

Then we really need to create ExportLogsCommandTest (UnitTest) and/or 
ExportLogsCommandIntegrationTest (IntegrationTest). I don't think we should be 
altering any code that we don't create a new UnitTest and/or IntegrationTest 
for.

To make this class testable with Mocks, you would for example, not invoke 
GemFireCacheImpl.getInstance() inside exportLogs.

Instead create:

private GemFireCache gemfireCache;

Set this in a new constructor:

public ExportLogsCommand() {
  this.gemfireCache = GemFireCacheImpl.getInstance();
}

Now you can actually test this class in a UnitTest with Mockito by simply 
using @Mock annotation on a private GemFireCache variable in the test and 
Mockito will inject that mock into this class!



geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
Line 37 (original), 40 (patched)


This is another class without UnitTest or IntegrationTest. Expecting the 
existing high-level DistributedTest (dunit) to cover these changes is not what 
we want. We want to pay down technical debt as we go and that means taking the 
time to create better unit tests for any new classes (this should be done 100% 
without question) and also for old classes that we need to modify.

So I'm looking for either ExportLogControllerTest or 
ExportLogControllerIntegrationTest or both. If the class needs to touch file 
system or sockets for basic testing then it's ok for that to be an 
ExportLogControllerIntegrationTest and you can even use Mockito in that type of 
testing.



geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
Line 85 (original), 85 (patched)


Another class that we really need to create a UnitTest or IntegrationTest 
for.


- Kirk Lund


On March 10, 2017, 5:17 p.m., Jinmei Liao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57514/
> ---
> 
> (Updated March 10, 2017, 5:17 p.m.)
> 
> 
> Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, 
> and Patrick Rhomberg.
> 
> 
> Repository: geode
> 
> 
> Description
> ---
> 
> * correctly output error message if gfsh execution has an error
> * export logs should output correct log message over http connection as well
> 
> 
> Diffs
> -
> 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
>  36d071c153ed8c2559cc8c29e248058b42cbf7ff 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java
>  ad78efd34fc4c9b1598751f71ba58ff5d29476a4 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
>  0351573f6eda80e1751bd6b95efdbe6ce36a620d 
>   
> geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
>  f60aabceeae5d973caa900e5443b7cec0c0a75ca 
>   
> geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportStatsDUnitTest.java
>  b7f8c2a080614ab59dc9da29c31f606749b58e4a 
> 
> 
> Diff: https://reviews.apache.org/r/57514/diff/1/
> 
> 
> Testing
> ---
> 
> precheckin running ...
> 
> 
> Thanks,
> 
> Jinmei Liao
> 
>

Re: Review Request 57514: GEODE-2267: enhance error output for gfsh.

2017-03-10 Thread Jinmei Liao 

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

(Updated March 10, 2017, 5:17 p.m.)


Review request for geode, Jared Stewart, Kevin Duling, Ken Howe, Kirk Lund, and 
Patrick Rhomberg.


Repository: geode


Description
---

* correctly output error message if gfsh execution has an error
* export logs should output correct log message over http connection as well


Diffs
-

  
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ExportLogCommand.java
 36d071c153ed8c2559cc8c29e248058b42cbf7ff 
  
geode-core/src/main/java/org/apache/geode/management/internal/cli/shell/GfshExecutionStrategy.java
 ad78efd34fc4c9b1598751f71ba58ff5d29476a4 
  
geode-core/src/main/java/org/apache/geode/management/internal/web/controllers/ExportLogController.java
 0351573f6eda80e1751bd6b95efdbe6ce36a620d 
  
geode-core/src/main/java/org/apache/geode/management/internal/web/shell/AbstractHttpOperationInvoker.java
 f60aabceeae5d973caa900e5443b7cec0c0a75ca 
  
geode-core/src/test/java/org/apache/geode/management/internal/cli/commands/ExportStatsDUnitTest.java
 b7f8c2a080614ab59dc9da29c31f606749b58e4a 


Diff: https://reviews.apache.org/r/57514/diff/1/


Testing (updated)
---

precheckin running ...


Thanks,

Jinmei Liao