Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8586013:
Added 4 new pyauto tests. (Closed)
|
Created:
9 years, 1 month ago by deepakg Modified:
9 years, 1 month ago CC:
chromium-reviews Visibility:
Public. |
DescriptionAdded 4 new pyauto tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111079
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 24
Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #Patch Set 8 : '' #
Total comments: 8
Patch Set 9 : '' #
Total comments: 10
Patch Set 10 : '' #
Total comments: 1
Messages
Total messages: 10 (0 generated)
These are the initial few functional tests that have been long pending. Will be adding more.
I recommend having Stan look over this too, since he's more familiar with wifi automation than I am ;-) http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... File functional/chromeos_wifi_functional.py (right): http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:1: #!/usr/bin/python Please modify the description of this CL so that it is more specific, maybe something like this: "Adding 4 new pyauto wifi tests for ChromeOS." http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:21: """Logs into the device and cleans up flimflam profile.""" Add an "Args:" section to this docstring that describes the new input parameter. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:100: """Can connect to a shared open network and are connected to this 'shared' --> 'non-shared' http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:101: even after logout. The first line of the docstring should be a 1-line summary that fits on a single line. If you have any more details to add, then you should add 1 blank line, followed by the rest of the details. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:114: if path == service_path: If this condition is never true, the test will still pass. Should we fail the test after the "for" loop, so that the test fails if this condition is never true? http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:117: ' shared open network after logout.') I think it would be a little cleaner to indent like this: self.assertTrue( self.GetNetworkInfo()['wifi_networks'][path]['status'] == 'Association' msg='We are not connected to the non-shared open network after logout.') http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:117: ' shared open network after logout.') 'shared' --> 'non-shared' http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:122: network list for all the users. The first line of the docstring should fit onto a single line. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:139: """Can connect to shared hidden network and verify that its shared.""" 'its' --> 'it's' http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:152: msg='Shared Hidden network is not in other users ' 'users' --> 'user's' http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:157: verify that its not shared. 'its' --> 'it's' http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:157: verify that its not shared. The first line of the docstring should fit onto a single line
http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... File functional/chromeos_wifi_functional.py (right): http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:1: #!/usr/bin/python On 2011/11/17 02:38:25, dennis_jeffrey wrote: > Please modify the description of this CL so that it is more specific, maybe > something like this: > > "Adding 4 new pyauto wifi tests for ChromeOS." Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:21: """Logs into the device and cleans up flimflam profile.""" On 2011/11/17 02:38:25, dennis_jeffrey wrote: > Add an "Args:" section to this docstring that describes the new input parameter. Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:100: """Can connect to a shared open network and are connected to this On 2011/11/17 02:38:25, dennis_jeffrey wrote: > 'shared' --> 'non-shared' Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:101: even after logout. On 2011/11/17 02:38:25, dennis_jeffrey wrote: > The first line of the docstring should be a 1-line summary that fits on a single > line. If you have any more details to add, then you should add 1 blank line, > followed by the rest of the details. Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:114: if path == service_path: On 2011/11/17 02:38:25, dennis_jeffrey wrote: > If this condition is never true, the test will still pass. Should we fail the > test after the "for" loop, so that the test fails if this condition is never > true? Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:117: ' shared open network after logout.') On 2011/11/17 02:38:25, dennis_jeffrey wrote: > I think it would be a little cleaner to indent like this: > > self.assertTrue( > self.GetNetworkInfo()['wifi_networks'][path]['status'] == 'Association' > msg='We are not connected to the non-shared open network after logout.') This will push the characters beyond the 80 character limit. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:117: ' shared open network after logout.') On 2011/11/17 02:38:25, dennis_jeffrey wrote: > 'shared' --> 'non-shared' Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:122: network list for all the users. On 2011/11/17 02:38:25, dennis_jeffrey wrote: > The first line of the docstring should fit onto a single line. Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:139: """Can connect to shared hidden network and verify that its shared.""" On 2011/11/17 02:38:25, dennis_jeffrey wrote: > 'its' --> 'it's' Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:152: msg='Shared Hidden network is not in other users ' On 2011/11/17 02:38:25, dennis_jeffrey wrote: > 'users' --> 'user's' Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:157: verify that its not shared. On 2011/11/17 02:38:25, dennis_jeffrey wrote: > 'its' --> 'it's' Done. http://codereview.chromium.org/8586013/diff/3001/functional/chromeos_wifi_fun... functional/chromeos_wifi_functional.py:157: verify that its not shared. On 2011/11/17 02:38:25, dennis_jeffrey wrote: > The first line of the docstring should fit onto a single line Done.
Hi Dennis I have updated the script. I am having issue with the test - testSharedOpenNetworkConnectedAfterLogout. We are not connected to a open wifi network after logout. Can you please take a look?
I have removed the function which was failing, and am submitting the remaining 4 tests now.
http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... File functional/chromeos_wifi_functional.py (right): http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:67: What do the function arguments mean? http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:68: The test calling this function will fail for one of these three reasons: extra space between will and fail http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:80: network_available = True We can get rid of the network_available variable if we use for/else statement. We go to the else statement only if we've hit the end of the for statement (ie. no breaks have been called) for path in self.NetworkScan()....: .... else // only gets called for a loop that iterates to the end .... http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:189: router_name = 'Belkin_G' We should probably check that if we're logged in, we log out first.
http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... File functional/chromeos_wifi_functional.py (right): http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:67: On 2011/11/21 19:32:45, stanleyw wrote: > What do the function arguments mean? Done. http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:68: The test calling this function will fail for one of these three reasons: On 2011/11/21 19:32:45, stanleyw wrote: > extra space between will and fail Done. http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:80: network_available = True On 2011/11/21 19:32:45, stanleyw wrote: > We can get rid of the network_available variable if we use for/else statement. > We go to the else statement only if we've hit the end of the for statement (ie. > no breaks have been called) > > for path in self.NetworkScan()....: > .... > else > // only gets called for a loop that iterates to the end > .... Thanks, didn't know this was possible. http://codereview.chromium.org/8586013/diff/12001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:189: router_name = 'Belkin_G' On 2011/11/21 19:32:45, stanleyw wrote: > We should probably check that if we're logged in, we log out first. Done.
Looking good - I just have a few more comments. http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... File functional/chromeos_wifi_functional.py (right): http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:86: msg='Not connected to the network %s.' % network_ssid) Maybe this error should be something like "Unexpected network status", and then it can show the expected and actual status message. http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:89: self.assertTrue(False, msg='Did not find the network %s in the' You can just directly do a "self.fail" here. http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:89: self.assertTrue(False, msg='Did not find the network %s in the' add a space right after 'the' at the end of this string. http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:152: router_name = "Netgear_WGR614" prefer single quotes over double quotes when defining python strings http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:172: router_name = "Linksys_WRT54GL" prefer single quotes over double quotes when defining python strings
http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... File functional/chromeos_wifi_functional.py (right): http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:86: msg='Not connected to the network %s.' % network_ssid) On 2011/11/22 00:32:23, dennis_jeffrey wrote: > Maybe this error should be something like "Unexpected network status", and then > it can show the expected and actual status message. Done. http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:89: self.assertTrue(False, msg='Did not find the network %s in the' On 2011/11/22 00:32:23, dennis_jeffrey wrote: > You can just directly do a "self.fail" here. Done. http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:89: self.assertTrue(False, msg='Did not find the network %s in the' On 2011/11/22 00:32:23, dennis_jeffrey wrote: > You can just directly do a "self.fail" here. Done. http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:152: router_name = "Netgear_WGR614" On 2011/11/22 00:32:23, dennis_jeffrey wrote: > prefer single quotes over double quotes when defining python strings Done. http://codereview.chromium.org/8586013/diff/18001/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:172: router_name = "Linksys_WRT54GL" On 2011/11/22 00:32:23, dennis_jeffrey wrote: > prefer single quotes over double quotes when defining python strings Done.
LGTM Just 1 more minor comment. Thank you! http://codereview.chromium.org/8586013/diff/19003/functional/chromeos_wifi_fu... File functional/chromeos_wifi_functional.py (right): http://codereview.chromium.org/8586013/diff/19003/functional/chromeos_wifi_fu... functional/chromeos_wifi_functional.py:87: msg='Unexpected network status %s, Network %s should have' add a space right after 'have' near the end of this line. |
