Tests for GSM compliance.

functional/chromeos_gsm_compliance.py

+#!/usr/bin/python
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+
+import subprocess as sub
+import time

[stanleyw, 2011/10/10 21:21:15]

Remove if you can find a way without using a sleep.  

[deepakg, 2011/10/12 22:37:22]

Done.

+import re

[dennis_jeffrey, 2011/10/10 21:48:32]

Move this up before "subprocess" so that the imports are in alphabetical order.

[deepakg, 2011/10/12 22:37:22]

Done.

+
+import pyauto_functional

[dennis_jeffrey, 2011/10/10 21:48:32]

Add a comment saying that this must occur before importing pyauto:

import pyauto_functional  # Must be imported before pyauto.

[deepakg, 2011/10/12 22:37:22]

Done.

+import pyauto
+
+
+class ChromeosGSMCompliance(pyauto.PyUITest):
+  """Tests for ChromeOS GSM compliance, by checking the name of the
+     connected network."""

[dennis_jeffrey, 2011/10/10 21:48:32]

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.
"""

[deepakg, 2011/10/12 22:37:22]

Done.

+
+  ethernet = None

[dennis_jeffrey, 2011/10/10 21:48:32]

Prefix this variable name with an underscore since it's used only internally to
this class:

_ethernet

[deepakg, 2011/10/12 22:37:22]

Done.

+
+

[dennis_jeffrey, 2011/10/10 21:48:32]

Remove one of these blank lines

[deepakg, 2011/10/12 22:37:22]

Done.

+  def _Ethernet(self):

[dennis_jeffrey, 2011/10/10 21:48:32]

Recommend changing this function name to "_GetEthernet"

[deepakg, 2011/10/12 22:37:22]

Done.

+    """Get the ethernet connected to."""

[dennis_jeffrey, 2011/10/10 21:48:32]

'connected to' --> 'to which the device is connected'

[deepakg, 2011/10/12 22:37:22]

Done.

+    process = sub.Popen('ifconfig | cut -d\' \' -f 1 | grep eth',
+                        stdout=sub.PIPE, shell=True).communicate()
+    self.assertNotEqual(process[0], '', 'Not connected to Ethernet.')

[dennis_jeffrey, 2011/10/10 21:48:32]

msg='Not connected to Ethernet.'

[deepakg, 2011/10/12 22:37:22]

Done.

+    self.ethernet = process[0].rstrip()
+
+  def setUp(self):
+    pyauto.PyUITest.setUp(self)
+    sub.Popen(['sudo', 'stop', 'cromo'])

[dennis_jeffrey, 2011/10/10 21:48:32]

Do we need to wait for this process to complete before proceeding?

[deepakg, 2011/10/12 22:37:22]

Using sub.call() instead of sub.Popen() to make sure that subprocess waits.
Done.

+    ethernet = self._Ethernet()
+    if self.ethernet:
+      sub.Popen(['sudo', '/bin/sh', '/usr/local/lib/flimflam/test/backchannel',
+                 'setup', self.ethernet, 'pseudo-modem0'])

[dennis_jeffrey, 2011/10/10 21:48:32]

Do we need to wait for this process to complete before proceeding?

[deepakg, 2011/10/12 22:37:22]

Done.

+    else:
+      # if there is no eth check if we have pseudo-modem0

[dennis_jeffrey, 2011/10/10 21:48:32]

Capitalize the 'i' in 'if', and add a period at the end of the sentence.

[deepakg, 2011/10/12 22:37:22]

Done.

+      process = sub.Popen('ifconfig | cut -d\' \' -f 1 | grep pseudo',
+                        stdout=sub.PIPE, shell=True).communicate()

[dennis_jeffrey, 2011/10/10 21:48:32]

indent this line by 2 more spaces

[dennis_jeffrey, 2011/10/10 21:48:32]

Do we need to wait for this process to complete before proceeding?

[deepakg, 2011/10/12 22:37:22]

Done.

+      self.assertNotEqual(process[0].rstrip(), 'pseudo-modem0',
+                          'Could not setup a pseudo-modem')

[dennis_jeffrey, 2011/10/10 21:48:32]

msg='Could not setup a pseudo-modem'

[deepakg, 2011/10/12 22:37:22]

Done.

+
+  def tearDown(self):
+    pyauto.PyUITest.tearDown(self)

[dennis_jeffrey, 2011/10/10 21:48:32]

nit: call PyUITest.tearDown as the last step in this function, since it was the
first step in the setUp() function above.

[deepakg, 2011/10/12 22:37:22]

Done.

+    if self.ethernet:
+      sub.Popen(['sudo', '/bin/sh', '/usr/local/lib/flimflam/test/backchannel',
+                 'teardown', self.ethernet, 'pseudo-modem0'])

[dennis_jeffrey, 2011/10/10 21:48:32]

Should we wait for this process to terminate before proceeding?

[deepakg, 2011/10/12 22:37:22]

Done.

+
+  def _BasicCarrierCompliance(self, carrier_name, cellular_name):

[dennis_jeffrey, 2011/10/10 21:48:32]

I recommend changing this function name to _VerifyBasicCarrierCompliance

[deepakg, 2011/10/12 22:37:22]

Done.

+    """Faking the specified carrier and checking the name.
+
+    Args:
+       carrier_name: The name of the carrier.
+       cellular_name: The name in the icon of the cellular network.
+    """
+    fake_gsm = sub.Popen(['sudo', '/usr/bin/python', 
+                          '/usr/local/lib/flimflam/test/fake-gsm-modem',
+                          '-c', carrier_name])
+    time.sleep(3)  # fake-gsm-modem needs time

[stanleyw, 2011/10/10 21:21:15]

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.  

[dennis_jeffrey, 2011/10/10 21:48:32]

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?

[deepakg, 2011/10/12 22:37:22]

Done.

[deepakg, 2011/10/12 22:37:22]

Done.

+    ps = sub.Popen('ps -ef | grep fake-gsm-modem | grep -v grep',
+                      shell=True, stdout=sub.PIPE)

[dennis_jeffrey, 2011/10/10 21:48:32]

indent this line by 3 fewer spaces

[deepakg, 2011/10/12 22:37:22]

Done.

+    self.assertNotEqual(re.search('fake-gsm-modem', ps.stdout.read()),
+                          None, 'fake-gsm is not running.')

[dennis_jeffrey, 2011/10/10 21:48:32]

indent this line by 2 fewer spaces

[dennis_jeffrey, 2011/10/10 21:48:32]

msg='fake-gsm is not running'

[deepakg, 2011/10/12 22:37:22]

Done.

[deepakg, 2011/10/12 22:37:22]

Done.

+    network_info = self.NetworkScan()['cellular_networks']
+    for key in network_info.keys():
+      fake_gsm.terminate()

[stanleyw, 2011/10/10 21:21:15]

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.

[deepakg, 2011/10/12 22:37:22]

Done.

+      self.assertEqual(network_info[key]['name'], cellular_name,
+                       'Did not successfully connect to cellular network %s.')

[dennis_jeffrey, 2011/10/10 21:48:32]

msg='Did not...'

Also, you have a %s in the string, but forgot to specify its value.

[deepakg, 2011/10/12 22:37:22]

Done.

+
+  def testConnectThree(self):
+    """Testing connection to Three."""
+    self._BasicCarrierCompliance('three', '3')
+
+  def testConnectSFR(self):
+    """Testing connection to SFR."""
+    self._BasicCarrierCompliance('SFR', 'SFR')
+
+  def testConnectKPN(self):
+    """Testing connection to KPN."""
+    self._BasicCarrierCompliance('KPN', 'KPN NL')
+
+  def testConnectSimyo(self):
+    """Testing connection to Simyo."""
+    self._BasicCarrierCompliance('Simyo', 'E-Plus')
+
+  def testConnectMovistar(self):
+    """Testing connection to Movistar."""
+    self._BasicCarrierCompliance('Movistar', 'Movistar')
+
+  def testConnectThreeita(self):
+    """Testing connection to threeita."""
+    self._BasicCarrierCompliance('threeita', '3ITA')
+
+  def testConnectAtt(self):
+    """Testing connection to att."""
+    self._BasicCarrierCompliance('att', 'AT&T')
+
+  def testConnectTmobile(self):
+    """Testing connection to Tmobile."""
+    self._BasicCarrierCompliance('Tmobile', 'T-Mobile')
+
+
+if __name__ == '__main__':
+  pyauto_functional.Main()
+

[dennis_jeffrey, 2011/10/10 21:48:32]

remove this blank line.

[deepakg, 2011/10/12 22:37:22]

Done.