You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sqoop.apache.org by Colin Ma <ju...@intel.com> on 2016/01/15 08:32:02 UTC
Review Request 42343: SQOOP-2790: Remove the duplicate test cases in
shell module
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42343/
-----------------------------------------------------------
Review request for Sqoop.
Repository: sqoop-sqoop2
Description
-------
The test cases for shell will be refactored as following:
1. Because the integration tests are created for shell, remove the duplicated cases in shell module.
2. Move the negative test cases from shell module to integration test.
The authorization test cases won't be changed in this ticket, and keep them in shell module for now.
Diffs
-----
shell/src/test/java/org/apache/sqoop/shell/TestCloneCommand.java 4515b1c
shell/src/test/java/org/apache/sqoop/shell/TestCreateCommand.java c1c23db
shell/src/test/java/org/apache/sqoop/shell/TestDeleteCommand.java 47a8f87
shell/src/test/java/org/apache/sqoop/shell/TestDisableCommand.java 9e4e532
shell/src/test/java/org/apache/sqoop/shell/TestEnableCommand.java 4c52448
shell/src/test/java/org/apache/sqoop/shell/TestSetCommand.java 93e1e3e
shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
shell/src/test/java/org/apache/sqoop/shell/TestStartCommand.java 7dc407c
shell/src/test/java/org/apache/sqoop/shell/TestStatusCommand.java ce01842
shell/src/test/java/org/apache/sqoop/shell/TestStopCommand.java ca9d03b
shell/src/test/java/org/apache/sqoop/shell/TestUpdateCommand.java 469ded7
test/src/test/java/org/apache/sqoop/integration/shell/CloneCommandTest.java bcf6334
test/src/test/java/org/apache/sqoop/integration/shell/CreateCommandTest.java 81b7c8a
test/src/test/java/org/apache/sqoop/integration/shell/DeleteCommandTest.java 8ed8570
test/src/test/java/org/apache/sqoop/integration/shell/DisableCommandTest.java e524312
test/src/test/java/org/apache/sqoop/integration/shell/EnableCommandTest.java b104348
test/src/test/java/org/apache/sqoop/integration/shell/SetCommandTest.java f673a58
test/src/test/java/org/apache/sqoop/integration/shell/StartCommandTest.java 5504586
test/src/test/java/org/apache/sqoop/integration/shell/StatusCommandTest.java 7294b05
test/src/test/java/org/apache/sqoop/integration/shell/StopCommandTest.java 69110b2
test/src/test/java/org/apache/sqoop/integration/shell/UpdateCommandTest.java f9a35fe
Diff: https://reviews.apache.org/r/42343/diff/
Testing
-------
Thanks,
Colin Ma
Re: Review Request 42343: SQOOP-2790: Remove the duplicate test cases
in shell module
Posted by Dian Fu <di...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42343/#review116052
-----------------------------------------------------------
Ship it!
Ship It!
- Dian Fu
On Jan. 15, 2016, 7:31 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42343/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 7:31 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> The test cases for shell will be refactored as following:
> 1. Because the integration tests are created for shell, remove the duplicated cases in shell module.
> 2. Move the negative test cases from shell module to integration test.
>
> The authorization test cases won't be changed in this ticket, and keep them in shell module for now.
>
>
> Diffs
> -----
>
> shell/src/test/java/org/apache/sqoop/shell/TestCloneCommand.java 4515b1c
> shell/src/test/java/org/apache/sqoop/shell/TestCreateCommand.java c1c23db
> shell/src/test/java/org/apache/sqoop/shell/TestDeleteCommand.java 47a8f87
> shell/src/test/java/org/apache/sqoop/shell/TestDisableCommand.java 9e4e532
> shell/src/test/java/org/apache/sqoop/shell/TestEnableCommand.java 4c52448
> shell/src/test/java/org/apache/sqoop/shell/TestSetCommand.java 93e1e3e
> shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
> shell/src/test/java/org/apache/sqoop/shell/TestStartCommand.java 7dc407c
> shell/src/test/java/org/apache/sqoop/shell/TestStatusCommand.java ce01842
> shell/src/test/java/org/apache/sqoop/shell/TestStopCommand.java ca9d03b
> shell/src/test/java/org/apache/sqoop/shell/TestUpdateCommand.java 469ded7
> test/src/test/java/org/apache/sqoop/integration/shell/CloneCommandTest.java bcf6334
> test/src/test/java/org/apache/sqoop/integration/shell/CreateCommandTest.java 81b7c8a
> test/src/test/java/org/apache/sqoop/integration/shell/DeleteCommandTest.java 8ed8570
> test/src/test/java/org/apache/sqoop/integration/shell/DisableCommandTest.java e524312
> test/src/test/java/org/apache/sqoop/integration/shell/EnableCommandTest.java b104348
> test/src/test/java/org/apache/sqoop/integration/shell/SetCommandTest.java f673a58
> test/src/test/java/org/apache/sqoop/integration/shell/StartCommandTest.java 5504586
> test/src/test/java/org/apache/sqoop/integration/shell/StatusCommandTest.java 7294b05
> test/src/test/java/org/apache/sqoop/integration/shell/StopCommandTest.java 69110b2
> test/src/test/java/org/apache/sqoop/integration/shell/UpdateCommandTest.java f9a35fe
>
> Diff: https://reviews.apache.org/r/42343/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 42343: SQOOP-2790: Remove the duplicate test cases
in shell module
Posted by Colin Ma <ju...@intel.com>.
> On Jan. 25, 2016, 6:33 p.m., Abraham Fine wrote:
> > why are we doing this? don't the integration tests and unit tests serve two different purposes?
> >
> > the unit tests test that the shell is sending the right message to the server and the integration tests ensure that the server is performing the correct behavior based on the message that the shell sends
Currently, the unit tests don't test sending the right message, actutally, it tests the server's behavior by Mock. The purpose of creating integration tests is to replace these test cases.
This change doesn't reduce the test scope, just delete the meaningless and duplicated test cases.
For the code manitenance, also move some negative test cases to integration test.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42343/#review116135
-----------------------------------------------------------
On Jan. 15, 2016, 7:31 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42343/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 7:31 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> The test cases for shell will be refactored as following:
> 1. Because the integration tests are created for shell, remove the duplicated cases in shell module.
> 2. Move the negative test cases from shell module to integration test.
>
> The authorization test cases won't be changed in this ticket, and keep them in shell module for now.
>
>
> Diffs
> -----
>
> shell/src/test/java/org/apache/sqoop/shell/TestCloneCommand.java 4515b1c
> shell/src/test/java/org/apache/sqoop/shell/TestCreateCommand.java c1c23db
> shell/src/test/java/org/apache/sqoop/shell/TestDeleteCommand.java 47a8f87
> shell/src/test/java/org/apache/sqoop/shell/TestDisableCommand.java 9e4e532
> shell/src/test/java/org/apache/sqoop/shell/TestEnableCommand.java 4c52448
> shell/src/test/java/org/apache/sqoop/shell/TestSetCommand.java 93e1e3e
> shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
> shell/src/test/java/org/apache/sqoop/shell/TestStartCommand.java 7dc407c
> shell/src/test/java/org/apache/sqoop/shell/TestStatusCommand.java ce01842
> shell/src/test/java/org/apache/sqoop/shell/TestStopCommand.java ca9d03b
> shell/src/test/java/org/apache/sqoop/shell/TestUpdateCommand.java 469ded7
> test/src/test/java/org/apache/sqoop/integration/shell/CloneCommandTest.java bcf6334
> test/src/test/java/org/apache/sqoop/integration/shell/CreateCommandTest.java 81b7c8a
> test/src/test/java/org/apache/sqoop/integration/shell/DeleteCommandTest.java 8ed8570
> test/src/test/java/org/apache/sqoop/integration/shell/DisableCommandTest.java e524312
> test/src/test/java/org/apache/sqoop/integration/shell/EnableCommandTest.java b104348
> test/src/test/java/org/apache/sqoop/integration/shell/SetCommandTest.java f673a58
> test/src/test/java/org/apache/sqoop/integration/shell/StartCommandTest.java 5504586
> test/src/test/java/org/apache/sqoop/integration/shell/StatusCommandTest.java 7294b05
> test/src/test/java/org/apache/sqoop/integration/shell/StopCommandTest.java 69110b2
> test/src/test/java/org/apache/sqoop/integration/shell/UpdateCommandTest.java f9a35fe
>
> Diff: https://reviews.apache.org/r/42343/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 42343: SQOOP-2790: Remove the duplicate test cases
in shell module
Posted by Colin Ma <ju...@intel.com>.
> On Jan. 25, 2016, 6:33 p.m., Abraham Fine wrote:
> > why are we doing this? don't the integration tests and unit tests serve two different purposes?
> >
> > the unit tests test that the shell is sending the right message to the server and the integration tests ensure that the server is performing the correct behavior based on the message that the shell sends
>
> Colin Ma wrote:
> Currently, the unit tests don't test sending the right message, actutally, it tests the server's behavior by Mock. The purpose of creating integration tests is to replace these test cases.
> This change doesn't reduce the test scope, just delete the meaningless and duplicated test cases.
> For the code manitenance, also move some negative test cases to integration test.
>
> Abraham Fine wrote:
> is that always the case?
>
> for instance, what about the set command. isn't testing that better suited for unit tests than integration?
>
> in other words, aren't shell tests better suited to be run on the unit test level with a mock "client" rather than a mock server?
Got your points, will keep these unit tests and close the JIRA.
Thanks for review.
- Colin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42343/#review116135
-----------------------------------------------------------
On Jan. 15, 2016, 7:31 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42343/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 7:31 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> The test cases for shell will be refactored as following:
> 1. Because the integration tests are created for shell, remove the duplicated cases in shell module.
> 2. Move the negative test cases from shell module to integration test.
>
> The authorization test cases won't be changed in this ticket, and keep them in shell module for now.
>
>
> Diffs
> -----
>
> shell/src/test/java/org/apache/sqoop/shell/TestCloneCommand.java 4515b1c
> shell/src/test/java/org/apache/sqoop/shell/TestCreateCommand.java c1c23db
> shell/src/test/java/org/apache/sqoop/shell/TestDeleteCommand.java 47a8f87
> shell/src/test/java/org/apache/sqoop/shell/TestDisableCommand.java 9e4e532
> shell/src/test/java/org/apache/sqoop/shell/TestEnableCommand.java 4c52448
> shell/src/test/java/org/apache/sqoop/shell/TestSetCommand.java 93e1e3e
> shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
> shell/src/test/java/org/apache/sqoop/shell/TestStartCommand.java 7dc407c
> shell/src/test/java/org/apache/sqoop/shell/TestStatusCommand.java ce01842
> shell/src/test/java/org/apache/sqoop/shell/TestStopCommand.java ca9d03b
> shell/src/test/java/org/apache/sqoop/shell/TestUpdateCommand.java 469ded7
> test/src/test/java/org/apache/sqoop/integration/shell/CloneCommandTest.java bcf6334
> test/src/test/java/org/apache/sqoop/integration/shell/CreateCommandTest.java 81b7c8a
> test/src/test/java/org/apache/sqoop/integration/shell/DeleteCommandTest.java 8ed8570
> test/src/test/java/org/apache/sqoop/integration/shell/DisableCommandTest.java e524312
> test/src/test/java/org/apache/sqoop/integration/shell/EnableCommandTest.java b104348
> test/src/test/java/org/apache/sqoop/integration/shell/SetCommandTest.java f673a58
> test/src/test/java/org/apache/sqoop/integration/shell/StartCommandTest.java 5504586
> test/src/test/java/org/apache/sqoop/integration/shell/StatusCommandTest.java 7294b05
> test/src/test/java/org/apache/sqoop/integration/shell/StopCommandTest.java 69110b2
> test/src/test/java/org/apache/sqoop/integration/shell/UpdateCommandTest.java f9a35fe
>
> Diff: https://reviews.apache.org/r/42343/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 42343: SQOOP-2790: Remove the duplicate test cases
in shell module
Posted by Abraham Fine <ab...@abrahamfine.com>.
> On Jan. 25, 2016, 6:33 p.m., Abraham Fine wrote:
> > why are we doing this? don't the integration tests and unit tests serve two different purposes?
> >
> > the unit tests test that the shell is sending the right message to the server and the integration tests ensure that the server is performing the correct behavior based on the message that the shell sends
>
> Colin Ma wrote:
> Currently, the unit tests don't test sending the right message, actutally, it tests the server's behavior by Mock. The purpose of creating integration tests is to replace these test cases.
> This change doesn't reduce the test scope, just delete the meaningless and duplicated test cases.
> For the code manitenance, also move some negative test cases to integration test.
is that always the case?
for instance, what about the set command. isn't testing that better suited for unit tests than integration?
in other words, aren't shell tests better suited to be run on the unit test level with a mock "client" rather than a mock server?
- Abraham
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42343/#review116135
-----------------------------------------------------------
On Jan. 15, 2016, 7:31 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42343/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 7:31 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> The test cases for shell will be refactored as following:
> 1. Because the integration tests are created for shell, remove the duplicated cases in shell module.
> 2. Move the negative test cases from shell module to integration test.
>
> The authorization test cases won't be changed in this ticket, and keep them in shell module for now.
>
>
> Diffs
> -----
>
> shell/src/test/java/org/apache/sqoop/shell/TestCloneCommand.java 4515b1c
> shell/src/test/java/org/apache/sqoop/shell/TestCreateCommand.java c1c23db
> shell/src/test/java/org/apache/sqoop/shell/TestDeleteCommand.java 47a8f87
> shell/src/test/java/org/apache/sqoop/shell/TestDisableCommand.java 9e4e532
> shell/src/test/java/org/apache/sqoop/shell/TestEnableCommand.java 4c52448
> shell/src/test/java/org/apache/sqoop/shell/TestSetCommand.java 93e1e3e
> shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
> shell/src/test/java/org/apache/sqoop/shell/TestStartCommand.java 7dc407c
> shell/src/test/java/org/apache/sqoop/shell/TestStatusCommand.java ce01842
> shell/src/test/java/org/apache/sqoop/shell/TestStopCommand.java ca9d03b
> shell/src/test/java/org/apache/sqoop/shell/TestUpdateCommand.java 469ded7
> test/src/test/java/org/apache/sqoop/integration/shell/CloneCommandTest.java bcf6334
> test/src/test/java/org/apache/sqoop/integration/shell/CreateCommandTest.java 81b7c8a
> test/src/test/java/org/apache/sqoop/integration/shell/DeleteCommandTest.java 8ed8570
> test/src/test/java/org/apache/sqoop/integration/shell/DisableCommandTest.java e524312
> test/src/test/java/org/apache/sqoop/integration/shell/EnableCommandTest.java b104348
> test/src/test/java/org/apache/sqoop/integration/shell/SetCommandTest.java f673a58
> test/src/test/java/org/apache/sqoop/integration/shell/StartCommandTest.java 5504586
> test/src/test/java/org/apache/sqoop/integration/shell/StatusCommandTest.java 7294b05
> test/src/test/java/org/apache/sqoop/integration/shell/StopCommandTest.java 69110b2
> test/src/test/java/org/apache/sqoop/integration/shell/UpdateCommandTest.java f9a35fe
>
> Diff: https://reviews.apache.org/r/42343/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>
Re: Review Request 42343: SQOOP-2790: Remove the duplicate test cases
in shell module
Posted by Abraham Fine <ab...@abrahamfine.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/42343/#review116135
-----------------------------------------------------------
why are we doing this? don't the integration tests and unit tests serve two different purposes?
the unit tests test that the shell is sending the right message to the server and the integration tests ensure that the server is performing the correct behavior based on the message that the shell sends
- Abraham Fine
On Jan. 15, 2016, 7:31 a.m., Colin Ma wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42343/
> -----------------------------------------------------------
>
> (Updated Jan. 15, 2016, 7:31 a.m.)
>
>
> Review request for Sqoop.
>
>
> Repository: sqoop-sqoop2
>
>
> Description
> -------
>
> The test cases for shell will be refactored as following:
> 1. Because the integration tests are created for shell, remove the duplicated cases in shell module.
> 2. Move the negative test cases from shell module to integration test.
>
> The authorization test cases won't be changed in this ticket, and keep them in shell module for now.
>
>
> Diffs
> -----
>
> shell/src/test/java/org/apache/sqoop/shell/TestCloneCommand.java 4515b1c
> shell/src/test/java/org/apache/sqoop/shell/TestCreateCommand.java c1c23db
> shell/src/test/java/org/apache/sqoop/shell/TestDeleteCommand.java 47a8f87
> shell/src/test/java/org/apache/sqoop/shell/TestDisableCommand.java 9e4e532
> shell/src/test/java/org/apache/sqoop/shell/TestEnableCommand.java 4c52448
> shell/src/test/java/org/apache/sqoop/shell/TestSetCommand.java 93e1e3e
> shell/src/test/java/org/apache/sqoop/shell/TestShowCommand.java 49affa3
> shell/src/test/java/org/apache/sqoop/shell/TestStartCommand.java 7dc407c
> shell/src/test/java/org/apache/sqoop/shell/TestStatusCommand.java ce01842
> shell/src/test/java/org/apache/sqoop/shell/TestStopCommand.java ca9d03b
> shell/src/test/java/org/apache/sqoop/shell/TestUpdateCommand.java 469ded7
> test/src/test/java/org/apache/sqoop/integration/shell/CloneCommandTest.java bcf6334
> test/src/test/java/org/apache/sqoop/integration/shell/CreateCommandTest.java 81b7c8a
> test/src/test/java/org/apache/sqoop/integration/shell/DeleteCommandTest.java 8ed8570
> test/src/test/java/org/apache/sqoop/integration/shell/DisableCommandTest.java e524312
> test/src/test/java/org/apache/sqoop/integration/shell/EnableCommandTest.java b104348
> test/src/test/java/org/apache/sqoop/integration/shell/SetCommandTest.java f673a58
> test/src/test/java/org/apache/sqoop/integration/shell/StartCommandTest.java 5504586
> test/src/test/java/org/apache/sqoop/integration/shell/StatusCommandTest.java 7294b05
> test/src/test/java/org/apache/sqoop/integration/shell/StopCommandTest.java 69110b2
> test/src/test/java/org/apache/sqoop/integration/shell/UpdateCommandTest.java f9a35fe
>
> Diff: https://reviews.apache.org/r/42343/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Colin Ma
>
>