Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8198024:
Tests for GSM compliance. (Closed)
|
Created:
9 years, 2 months ago by deepakg Modified:
9 years, 2 months ago CC:
chromium-reviews Visibility:
Public. |
DescriptionTests for GSM compliance.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107019
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 51
Patch Set 4 : '' #
Total comments: 6
Patch Set 5 : '' #
Total comments: 23
Patch Set 6 : '' #
Total comments: 12
Patch Set 7 : '' #Patch Set 8 : '' #Patch Set 9 : '' #Messages
Total messages: 13 (0 generated)
This tests checks if we can connect to a particular network and get the correct badge.
http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:8: import time Remove if you can find a way without using a sleep. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:59: time.sleep(3) # fake-gsm-modem needs time What time is required? Can we do without the sleep? For. Eg. Use WaitUntil(<line 60)) and fail if wait until doesn't receive the correct modem response in 10 seconds. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:66: fake_gsm.terminate() From my understanding, there should only be one fake gsm modem ever running? In the event network_info has more than one entry (not sure if this will ever happen), then we're calling terminate twice. We might be able to jsut get rid of the for loop altogether. Otherwise, move the terminate code outside of the loop.
Modify the description of this CL to specify that these are pyauto tests for GSM compliance. Also, I think you'll need to add a "BUG=" and a "TEST=" field to the CL description. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:9: import re Move this up before "subprocess" so that the imports are in alphabetical order. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:11: import pyauto_functional Add a comment saying that this must occur before importing pyauto: import pyauto_functional # Must be imported before pyauto. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:17: connected network.""" Make this sentence fit on a single line, since the first line of a class docstring should be a 1-line summary. If you need to write more than 1 line, the details should go on another line down below, like this: """One-line summary here. Further details go here, if necessary. """ http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:19: ethernet = None Prefix this variable name with an underscore since it's used only internally to this class: _ethernet http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:21: Remove one of these blank lines http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:22: def _Ethernet(self): Recommend changing this function name to "_GetEthernet" http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:23: """Get the ethernet connected to.""" 'connected to' --> 'to which the device is connected' http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:26: self.assertNotEqual(process[0], '', 'Not connected to Ethernet.') msg='Not connected to Ethernet.' http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:31: sub.Popen(['sudo', 'stop', 'cromo']) Do we need to wait for this process to complete before proceeding? http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:35: 'setup', self.ethernet, 'pseudo-modem0']) Do we need to wait for this process to complete before proceeding? http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:37: # if there is no eth check if we have pseudo-modem0 Capitalize the 'i' in 'if', and add a period at the end of the sentence. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:39: stdout=sub.PIPE, shell=True).communicate() indent this line by 2 more spaces http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:39: stdout=sub.PIPE, shell=True).communicate() Do we need to wait for this process to complete before proceeding? http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:41: 'Could not setup a pseudo-modem') msg='Could not setup a pseudo-modem' http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:44: pyauto.PyUITest.tearDown(self) nit: call PyUITest.tearDown as the last step in this function, since it was the first step in the setUp() function above. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:47: 'teardown', self.ethernet, 'pseudo-modem0']) Should we wait for this process to terminate before proceeding? http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:49: def _BasicCarrierCompliance(self, carrier_name, cellular_name): I recommend changing this function name to _VerifyBasicCarrierCompliance http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:59: time.sleep(3) # fake-gsm-modem needs time A blind sleep of hard-coded time is dangerous. Is there a way that we can poll and keep checking until things are ready, like what Stan recommends? http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:61: shell=True, stdout=sub.PIPE) indent this line by 3 fewer spaces http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:63: None, 'fake-gsm is not running.') indent this line by 2 fewer spaces http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:63: None, 'fake-gsm is not running.') msg='fake-gsm is not running' http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:68: 'Did not successfully connect to cellular network %s.') msg='Did not...' Also, you have a %s in the string, but forgot to specify its value. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:105: remove this blank line.
http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:8: import time On 2011/10/10 21:21:15, stanleyw wrote: > Remove if you can find a way without using a sleep. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:9: import re On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Move this up before "subprocess" so that the imports are in alphabetical order. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:11: import pyauto_functional On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Add a comment saying that this must occur before importing pyauto: > > import pyauto_functional # Must be imported before pyauto. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:17: connected network.""" On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Make this sentence fit on a single line, since the first line of a class > docstring should be a 1-line summary. If you need to write more than 1 line, > the details should go on another line down below, like this: > > """One-line summary here. > > Further details go here, if necessary. > """ Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:19: ethernet = None On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Prefix this variable name with an underscore since it's used only internally to > this class: > > _ethernet Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:21: On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Remove one of these blank lines Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:22: def _Ethernet(self): On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Recommend changing this function name to "_GetEthernet" Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:23: """Get the ethernet connected to.""" On 2011/10/10 21:48:32, dennis_jeffrey wrote: > 'connected to' --> 'to which the device is connected' Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:26: self.assertNotEqual(process[0], '', 'Not connected to Ethernet.') On 2011/10/10 21:48:32, dennis_jeffrey wrote: > msg='Not connected to Ethernet.' Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:31: sub.Popen(['sudo', 'stop', 'cromo']) On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Do we need to wait for this process to complete before proceeding? Using sub.call() instead of sub.Popen() to make sure that subprocess waits. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:35: 'setup', self.ethernet, 'pseudo-modem0']) On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Do we need to wait for this process to complete before proceeding? Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:37: # if there is no eth check if we have pseudo-modem0 On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Capitalize the 'i' in 'if', and add a period at the end of the sentence. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:39: stdout=sub.PIPE, shell=True).communicate() On 2011/10/10 21:48:32, dennis_jeffrey wrote: > indent this line by 2 more spaces Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:41: 'Could not setup a pseudo-modem') On 2011/10/10 21:48:32, dennis_jeffrey wrote: > msg='Could not setup a pseudo-modem' Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:44: pyauto.PyUITest.tearDown(self) On 2011/10/10 21:48:32, dennis_jeffrey wrote: > nit: call PyUITest.tearDown as the last step in this function, since it was the > first step in the setUp() function above. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:47: 'teardown', self.ethernet, 'pseudo-modem0']) On 2011/10/10 21:48:32, dennis_jeffrey wrote: > Should we wait for this process to terminate before proceeding? Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:49: def _BasicCarrierCompliance(self, carrier_name, cellular_name): On 2011/10/10 21:48:32, dennis_jeffrey wrote: > I recommend changing this function name to _VerifyBasicCarrierCompliance Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:59: time.sleep(3) # fake-gsm-modem needs time On 2011/10/10 21:21:15, stanleyw wrote: > What time is required? Can we do without the sleep? For. Eg. Use > WaitUntil(<line 60)) and fail if wait until doesn't receive the correct modem > response in 10 seconds. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:59: time.sleep(3) # fake-gsm-modem needs time On 2011/10/10 21:48:32, dennis_jeffrey wrote: > A blind sleep of hard-coded time is dangerous. Is there a way that we can poll > and keep checking until things are ready, like what Stan recommends? Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:61: shell=True, stdout=sub.PIPE) On 2011/10/10 21:48:32, dennis_jeffrey wrote: > indent this line by 3 fewer spaces Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:63: None, 'fake-gsm is not running.') On 2011/10/10 21:48:32, dennis_jeffrey wrote: > indent this line by 2 fewer spaces Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:63: None, 'fake-gsm is not running.') On 2011/10/10 21:48:32, dennis_jeffrey wrote: > msg='fake-gsm is not running' Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:66: fake_gsm.terminate() On 2011/10/10 21:21:15, stanleyw wrote: > From my understanding, there should only be one fake gsm modem ever running? In > the event network_info has more than one entry (not sure if this will ever > happen), then we're calling terminate twice. > > We might be able to jsut get rid of the for loop altogether. Otherwise, move > the terminate code outside of the loop. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:68: 'Did not successfully connect to cellular network %s.') On 2011/10/10 21:48:32, dennis_jeffrey wrote: > msg='Did not...' > > Also, you have a %s in the string, but forgot to specify its value. Done. http://codereview.chromium.org/8198024/diff/3001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:105: On 2011/10/10 21:48:32, dennis_jeffrey wrote: > remove this blank line. Done.
Mostly good with the exception of the poll loop which can be simplified. http://codereview.chromium.org/8198024/diff/5001/functional/chromeos_gsm_comp... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/5001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:6: import re re does not appear to be used so I think we can remove it. http://codereview.chromium.org/8198024/diff/5001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:31: _ethernet = self._GetEthernet() GetEthernet returns nothing. You don't need _ethernet here. http://codereview.chromium.org/8198024/diff/5001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:68: for counter in range(10): # Wait for the fake GSM to connect. We have a subroutine for this in pyauto.py. It's called WaitUntil. Look at WaitUntilWifiNetworkAvailable in pyautolib/chromeos_network.py for an example of this being used.
http://codereview.chromium.org/8198024/diff/5001/functional/chromeos_gsm_comp... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/5001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:6: import re On 2011/10/18 01:24:47, stanleyw wrote: > re does not appear to be used so I think we can remove it. Done. http://codereview.chromium.org/8198024/diff/5001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:31: _ethernet = self._GetEthernet() On 2011/10/18 01:24:47, stanleyw wrote: > GetEthernet returns nothing. You don't need _ethernet here. Done. http://codereview.chromium.org/8198024/diff/5001/functional/chromeos_gsm_comp... functional/chromeos_gsm_compliance.py:68: for counter in range(10): # Wait for the fake GSM to connect. On 2011/10/18 01:24:47, stanleyw wrote: > We have a subroutine for this in pyauto.py. It's called WaitUntil. Look at > WaitUntilWifiNetworkAvailable in pyautolib/chromeos_network.py for an example of > this being used. Done.
http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:22: process = sub.Popen('ifconfig | cut -d\' \' -f 1 | grep eth', nit: maybe change the variable name "process" to something like "result", since you're calling communicate() on the result of sub.Popen() (the thing stored into variable "process" is not actually a process object, but the result of that process after it completes). http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:34: else: Will we ever enter the "else" case? Because at line 24 above, it looks like the test will fail if there is no ethernet. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:35: # If there is no eth check if we have pseudo-modem0. nit: 'eth' --> 'ethernet' http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:36: process = sub.Popen('ifconfig | cut -d\' \' -f 1 | grep pseudo', Similar comment as line 22 above. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:47: def _CheckFakeGSM(self): Maybe this function name is more descriptive: _IsFakeGSMRunning http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:48: """Check if fake-gsm-modem is running.""" Add a "Returns:" section to this docstring, since this function returns a value. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:49: ps = sub.Popen('ps -ef | grep fake-gsm-modem | grep -v grep', change to call? http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:50: shell=True, stdout=sub.PIPE).stdout.read() Does calling ".stdout.read()" here ensure that the process will terminate before proceeding? Why not just use ".communicate()" here like you do at lines 23 and 37 above? http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:53: return False I think it'll be have the same if we replace lines 51-53 with this single line: return len(ps.split('fake-gsm-modem')) > 0 http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:68: self.WaitUntil(lambda:cellular in self.NetworkScan().keys(), nit: add a space right after the ':' http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:68: self.WaitUntil(lambda:cellular in self.NetworkScan().keys(), The WaitUntil function will return False if the thing we're waiting for doesn't happen before the timeout. We should probably wrap the call to WaitUntil inside of a call to assertTrue, so that the test fails if the WaitUntil hits the timeout http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:74: break I think can replace lines 71-74 with this: result = any([network_info[key]['name'] == cellular_name for key in network_info.keys()]) And by doing so, we can also remove the definition of "result" at line 65 above.
http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:22: process = sub.Popen('ifconfig | cut -d\' \' -f 1 | grep eth', On 2011/10/20 23:49:14, dennis_jeffrey wrote: > nit: maybe change the variable name "process" to something like "result", since > you're calling communicate() on the result of sub.Popen() (the thing stored into > variable "process" is not actually a process object, but the result of that > process after it completes). Done. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:34: else: On 2011/10/20 23:49:14, dennis_jeffrey wrote: > Will we ever enter the "else" case? Because at line 24 above, it looks like the > test will fail if there is no ethernet. Done. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:35: # If there is no eth check if we have pseudo-modem0. On 2011/10/20 23:49:14, dennis_jeffrey wrote: > nit: 'eth' --> 'ethernet' This part is removed. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:36: process = sub.Popen('ifconfig | cut -d\' \' -f 1 | grep pseudo', On 2011/10/20 23:49:14, dennis_jeffrey wrote: > Similar comment as line 22 above. This part is removed. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:47: def _CheckFakeGSM(self): On 2011/10/20 23:49:14, dennis_jeffrey wrote: > Maybe this function name is more descriptive: > > _IsFakeGSMRunning Done. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:48: """Check if fake-gsm-modem is running.""" On 2011/10/20 23:49:14, dennis_jeffrey wrote: > Add a "Returns:" section to this docstring, since this function returns a value. Done. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:49: ps = sub.Popen('ps -ef | grep fake-gsm-modem | grep -v grep', On 2011/10/20 23:49:14, dennis_jeffrey wrote: > change to call? Done. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:50: shell=True, stdout=sub.PIPE).stdout.read() On 2011/10/20 23:49:14, dennis_jeffrey wrote: > Does calling ".stdout.read()" here ensure that the process will terminate before > proceeding? Why not just use ".communicate()" here like you do at lines 23 and > 37 above? Done. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:53: return False On 2011/10/20 23:49:14, dennis_jeffrey wrote: > I think it'll be have the same if we replace lines 51-53 with this single line: > > return len(ps.split('fake-gsm-modem')) > 0 Done. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:68: self.WaitUntil(lambda:cellular in self.NetworkScan().keys(), On 2011/10/20 23:49:14, dennis_jeffrey wrote: > The WaitUntil function will return False if the thing we're waiting for doesn't > happen before the timeout. We should probably wrap the call to WaitUntil inside > of a call to assertTrue, so that the test fails if the WaitUntil hits the > timeout Done. http://codereview.chromium.org/8198024/diff/10001/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:74: break On 2011/10/20 23:49:14, dennis_jeffrey wrote: > I think can replace lines 71-74 with this: > > result = any([network_info[key]['name'] == cellular_name > for key in network_info.keys()]) > > And by doing so, we can also remove the definition of "result" at line 65 above. Done. Thanks, I didn't know about any().
LGTM Just a few nits to consider. Please also wait for Stan to approve before you submit. Thanks! http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:44: True if fake-gsm-modem process is running and False if not. nit: indent this line by 1 fewer space (should only be indented 2 spaces underneath the previous line). http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:46: call = sub.Popen('ps -ef | grep fake-gsm-modem | grep -v grep', sorry, my previous comment where I said "change to call?" was a temporary comment for myself that I meant to delete, but forgot to delete it. I think the variable name "ps" that you originally had here is ok. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:55: cellular_name: The name in the icon of the cellular network. nit: indent the above 2 lines by 1 fewer space each
Thanks Dennis. Will wait till Stan approves it as well. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:44: True if fake-gsm-modem process is running and False if not. On 2011/10/21 01:02:16, dennis_jeffrey wrote: > nit: indent this line by 1 fewer space (should only be indented 2 spaces > underneath the previous line). Done. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:46: call = sub.Popen('ps -ef | grep fake-gsm-modem | grep -v grep', On 2011/10/21 01:02:16, dennis_jeffrey wrote: > sorry, my previous comment where I said "change to call?" was a temporary > comment for myself that I meant to delete, but forgot to delete it. I think the > variable name "ps" that you originally had here is ok. Done. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:55: cellular_name: The name in the icon of the cellular network. On 2011/10/21 01:02:16, dennis_jeffrey wrote: > nit: indent the above 2 lines by 1 fewer space each Done.
Sorry, overlooked one problem in the code. Also included some other simple fixes. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:64: msg='Did not connect to any cellular network') "No cellular networks appeared on scan.". No connection is happening here. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:68: self.assertTrue(self._IsFakeGSMRunning(), msg='Fake GSM is not running.') This assertion should probably happen before we perform the network scan but after we've set up the fake-gsm. No purpose in scanning if fake-gsm isn't even running. Also, referring to the line below. We probably want to terminate on every assertion. If one of the above assertion fails, this could lead to several instances of fake-gsm-modem being run. It might be safe to use popen.poll to make sure the process has actually terminated so other tests are not affected by a failure. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:70: self.assertTrue(result, msg='Did not connect to cellular network %s.' We're testing if name displayed in the menu matches the desired display name of the network, not if the network connects. This assertion/message should probably reflect that.
http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... File functional/chromeos_gsm_compliance.py (right): http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:64: msg='Did not connect to any cellular network') On 2011/10/21 01:21:16, stanleyw wrote: > "No cellular networks appeared on scan.". No connection is happening here. Done. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:68: self.assertTrue(self._IsFakeGSMRunning(), msg='Fake GSM is not running.') On 2011/10/21 01:21:16, stanleyw wrote: > This assertion should probably happen before we perform the network scan but > after we've set up the fake-gsm. No purpose in scanning if fake-gsm isn't even > running. > > Also, referring to the line below. We probably want to terminate on every > assertion. If one of the above assertion fails, this could lead to several > instances of fake-gsm-modem being run. It might be safe to use popen.poll to > make sure the process has actually terminated so other tests are not affected by > a failure. Done. http://codereview.chromium.org/8198024/diff/11002/functional/chromeos_gsm_com... functional/chromeos_gsm_compliance.py:70: self.assertTrue(result, msg='Did not connect to cellular network %s.' On 2011/10/21 01:21:16, stanleyw wrote: > We're testing if name displayed in the menu matches the desired display name of > the network, not if the network connects. This assertion/message should > probably reflect that. Done.
lgtm |
