----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16867/#review32353 -----------------------------------------------------------
test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61106> Move services class to config.cfg under config directory. Then use testclient getconfig object to get the configuration parsed as dictionary. test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61112> Please use try except block for entire setUpClass and do clean up in case of any exception here? Otherwise we may miss to clean up few things in case of exception. Exit in case of exception and don't proceed further test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61107> Append to list like we did for appending an account test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61111> VirtualMachine class under base.py has services as second argument but we are passing services["virtual_machine"], why so? Also, inside the create method, we are using account details as well passed from services class. Can we check this? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61109> Use common way of using services class. Here we are referring using class name, below we referred using services class. Use one way. test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61110> Why assign port and protocol values here? We could see its already available under services class? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61108> Remove prints and use log methods provided test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61113> Please add some comments, otherwise code looks little cluttered and not clear to understand. Organize them as functional units if possible. test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61114> Are we creating same account with similar parameters twice, once firstaccount and then secondaccount here? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61146> add try except block and exit the test run in case of exception test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61145> Do we need this rpm? Can we have allthese packages available as part of test client already? Dont we want to exit in case these steps fail? If we need this rpm can we have these packages available already rather than downloading every time? TC may take more time right? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61147> move these calls in to a function call with proper arguments, rather the code looks repetitive? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61144> We can move this required files to a marvin lib/config directory with your module name and use the scripts\conf from there? This will make code look neater. As well, we are not sure open with w+ may fail in case the file already exists? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61143> is %d right here? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61148> why some static methods and few class methods? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61150> move these scripts as ready and hands on scripts rather than writing inside of test code and make sure that all paths are used in common rather open(file,"mode") use open( relative_path + file_name,"mode") test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61149> what is this l? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61130> Please move this to a new file and separate it from this test module? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61128> Can we move this script outside of this test module and checkin as a separate script, it will then be available under marvin path for use? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61129> how does first argument to shell script is interpreted as start function? or stop function? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61115> Please move this to library utils\common test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61116> Use runCommand instead of execute from SshClient class. Here, we will come to know the status of the command and then we can take a call. Otherwise execute will not let you know the status of the command whether it failed\succeeded? Please check test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61122> What is the expected behavior? after deleting a user ssh should not work or work? Its not clear with steps? Add comments for expected behavior for each test case test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61117> There is no assert in this test case. How are we determining pass\fail criteria here? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61119> Cant we use the port mentioned in services class instead of hardcoding again 22? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61120> change execute to runCommand as mentioned above and verify the response and output properly? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61118> what is the use of this if else, it just dumps the debug message? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61121> Are we executing this command in vpnclient shell or vpN vm1 nic0? if ssh failed at line 874, still we execute this command? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61123> i believe we wanted to fail here rather than throwing exception? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61126> Use test case writing as flow of steps, step1, step2, step3 marking them as comments, make sure to match these flow with comments mentioned, its easy to understand the flow test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61125> where is the assert condition for this test case? Also, can we comment on what is expected behavior? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61124> this message does not give any useful information. Please use the proper message? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61131> what is the behavior of stop\start commands? Where are we verifying the command succeeded or failed? So, stop and start should allow a ssh access from second vpn user? Please add some comments? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61137> if else is just doing a debug message dump. It wont do any action and ssh should depend upon if else case? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61127> Do we want to execute this command even if ssh at 918 fails? Then this should be part of else statement? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61132> which removed user? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61133> So, test case dependencies are there? Here, we are assuming that this test case will run post test case2? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61135> This seems to be repetitive code? Can we move this to a common function with some comments? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61134> We want to remove them only for success or assert passed? not when assert failed? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61136> Assertion is missing? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61139> what if delete failed? Can we add try\except block for total test case and fail in case of any exception? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61138> If vpc is disabled, ssh will fail here and can we add an assertion here? Then below steps are not required for EX: ifconfig ppp0? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61142> if ls fails , then also is a test case failure? or command failure? and as well this code is repetitied, move it to a function call for better usage? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61140> Totally we could see that 7 users are getting created, 6 through xrange loop and one thereafter? We mentioned a limit of 8, we want to touch 8+ to check the condition? test/integration/component/test_remotevpn_vpc.py <https://reviews.apache.org/r/16867/#comment61141> use self.fail or assertion, dont use exception. - Santhosh Edukulla On Jan. 16, 2014, 7:12 p.m., Chandan Purushothama wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16867/ > ----------------------------------------------------------- > > (Updated Jan. 16, 2014, 7:12 p.m.) > > > Review request for cloudstack, Girish Shilamkar, SrikanteswaraRao Talluri, > and Sheng Yang. > > > Repository: cloudstack-git > > > Description > ------- > > Test Suite for Testing Remote Access VPN on VPC. > > I successfully automated the following components: > > 1.VPN Client Installation on Linux > 2.Component that efficiently verifies Remote VPN Access between client and > the Server. > 3.Developed a component that automates VPN Client Configuration and services. > 4.Test Cases of the Feature > 5.Tested the Code multiple times on XenServer and fixed the bugs. > 6.Requirement on VMWare: Default Template should be CentOS 5.5 or higher for > the test suite to work. VPN Client cannot be installed on CentOS 5.3 Default > Template on VMWare. > > > Diffs > ----- > > test/integration/component/test_remotevpn_vpc.py PRE-CREATION > > Diff: https://reviews.apache.org/r/16867/diff/ > > > Testing > ------- > > Test case no : Enable VPN for Public IP Address on the VPC ... ok > Test case no : Remote a VPN User ... ok > Test case no : Add a Different VPN User and Test Access with already existing > VPN User ... ok > Test case no : Add a Previously Removed VPN User from the VPC and Test the > VPN Connectivity ... ok > Test case no : Disable the VPN Service on the VPC ... ok > Test case no : Enabled Previously Dsiabled VPN Access to VPC. ... ok > Test case no : Create Nine VPN Users to test the remote.vpn.user.limit=8 > Configuration parameter ... ok > > ---------------------------------------------------------------------- > Ran 7 tests in 645.787s > > OK > > > File Attachments > ---------------- > > 0001-Test-Suite-for-Remote-Access-VPN-on-VPC.patch > > https://reviews.apache.org/media/uploaded/files/2014/01/14/c6d3f593-d0eb-407c-aad2-574ebf9ca0f8__0001-Test-Suite-for-Remote-Access-VPN-on-VPC.patch > > > Thanks, > > Chandan Purushothama > >
