Constrained Network test does not fail fast under fatal conditions.

chrome/test/functional/media/media_constrained_network_perf.py

Index: chrome/test/functional/media/media_constrained_network_perf.py
diff --git a/chrome/test/functional/media/media_constrained_network_perf.py b/chrome/test/functional/media/media_constrained_network_perf.py
index 83a4583f2f7dc71349098e55bd17fc8391d11c0a..b688b6d1a8bbdbbce41cfd0b7520a4e56554dd25 100755
--- a/chrome/test/functional/media/media_constrained_network_perf.py
+++ b/chrome/test/functional/media/media_constrained_network_perf.py
@@ -18,6 +18,7 @@ The CNS code is located under: <root>/src/media/tools/constrained_network_server
 """
 
 import itertools
+import logging
 import os
 import Queue
 import subprocess
@@ -28,7 +29,7 @@ import pyauto_media
 import pyauto
 import pyauto_paths
 import pyauto_utils
-
+import urllib2

[DaleCurtis, 2012/01/06 23:19:42]

This should go under threading.

[shadi, 2012/01/07 04:56:19]

Done.

 
 # Settings for each network constraint.
 _BANDWIDTH_SETTINGS_KBPS = {'None': 0, 'Low': 256, 'Medium': 2000, 'High': 5000}
@@ -103,18 +104,21 @@ class TestWorker(threading.Thread):
       if tab['url'] == url:
         return tab['index']
 
-  def _HaveMetrics(self, unique_url):
-    """Returns true if metrics are ready.  Set self.{_epp,_ttp} < 0 pre-run."""
+  def _HaveTTPMetric(self, unique_url):

[DaleCurtis, 2012/01/06 23:19:42]

These methods are very similar, you should create a generic _HaveMetric() method
which takes a value name.

[shadi, 2012/01/07 04:56:19]

Done.

+    """Returns true if ttp metrics is ready. Set self._ttp < 0 pre-run."""
     with self._automation_lock:
       tab = self._FindTabLocked(unique_url)
-
-      if self._epp < 0:
-        self._epp = int(self._pyauto.GetDOMValue('extra_play_percentage',
+      self._ttp = int(self._pyauto.GetDOMValue('time_to_playback',
                                                  tab_index=tab))

[DaleCurtis, 2012/01/06 23:19:42]

Bad indent. Please run gpylint before publishing.

[shadi, 2012/01/07 04:56:19]

Done.

-      if self._ttp < 0:
-        self._ttp = int(self._pyauto.GetDOMValue('time_to_playback',
+    return self._ttp >= 0
+
+  def _HaveEPPMetric(self, unique_url):
+    """Returns true if epp metric is ready. Set self._epp < 0 pre-run."""
+    with self._automation_lock:
+      tab = self._FindTabLocked(unique_url)
+      self._epp = int(self._pyauto.GetDOMValue('extra_play_percentage',
                                                  tab_index=tab))
-    return self._epp >= 0 and self._ttp >= 0
+    return self._epp >= 0
 
   def run(self):
     """Opens tab, starts HTML test, and records metrics for each queue entry.
@@ -155,8 +159,14 @@ class TestWorker(threading.Thread):
       # here since pyauto.WaitUntil doesn't call into Chrome.
       self._epp = self._ttp = -1
       self._pyauto.WaitUntil(
-        self._HaveMetrics, args=[unique_url], retry_sleep=2,
-        timeout=_TEST_VIDEO_DURATION_SEC * 10)
+        self._HaveTTPMetric, args=[unique_url], retry_sleep=1, timeout= 5,

[DaleCurtis, 2012/01/06 23:19:42]

Bad spacing. Again gpylint is your friend here :)

[shadi, 2012/01/07 04:56:19]

Done.

+        debug=False)
+
+      # Do not wait for epp if ttp is not available.
+      if self._ttp >= 0:

[DaleCurtis, 2012/01/06 23:19:42]

Should we log an error? Skip logging of epp, ttp below?

[shadi, 2012/01/07 04:56:19]

Done.

+        self._pyauto.WaitUntil(
+          self._HaveEPPMetric, args=[unique_url], retry_sleep=2,
+          timeout=_TEST_VIDEO_DURATION_SEC * 10, debug=False)
 
       # Record results.
       # TODO(dalecurtis): Support reference builds.
@@ -173,6 +183,26 @@ class TestWorker(threading.Thread):
       self._tasks.task_done()
 
 
+class ProcessLogger(threading.Thread):
+  """A thread to log a process's stderr output."""
+  def __init__(self, process):

[DaleCurtis, 2012/01/06 23:19:42]

Need blank line above this. gpylint... :)

[shadi, 2012/01/07 04:56:19]

Done.

+    """Starts the process logger thread.
+
+    Args:
+      process: The process to log.
+    """
+    threading.Thread.__init__(self)
+    self._process = process
+    self.start()
+
+  def run(self):
+    """Adds debug statements for the process's stderr output."""
+    line = True
+    while line:
+      line = self._process.stderr.readline()
+      logging.debug(line)
+
+
 class MediaConstrainedNetworkPerfTest(pyauto.PyUITest):
   """PyAuto test container.  See file doc string for more information."""
 
@@ -182,7 +212,8 @@ class MediaConstrainedNetworkPerfTest(pyauto.PyUITest):
            '--port', str(_CNS_PORT),
            '--interface', 'lo',
            '--www-root', os.path.join(
-               self.DataDir(), 'pyauto_private', 'media')]
+               self.DataDir(), 'pyauto_private', 'media'),
+           '-v']
 
     process = subprocess.Popen(cmd, stderr=subprocess.PIPE)
 
@@ -190,12 +221,25 @@ class MediaConstrainedNetworkPerfTest(pyauto.PyUITest):
     line = True
     while line:
       line = process.stderr.readline()
+      logging.debug(line)
       if 'STARTED' in line:
         self._server_pid = process.pid
         pyauto.PyUITest.setUp(self)
+        ProcessLogger(process)
+        self.CheckServerAccess()
         return
     self.fail('Failed to start CNS.')
 
+  def CheckServerAccess(self):
+    """Checks is the CNS server can serve a file with no network constraints."""
+    test_url = ''.join([_CNS_BASE_URL, 'f=' ,_TEST_VIDEO])
+    try:
+      urllib2.urlopen(test_url)
+    except:
+      # Need to call teardown since the server has already started.
+      self.tearDown()

[DaleCurtis, 2012/01/06 23:19:42]

This should be called automatically. Did you find it not being called?

[shadi, 2012/01/07 04:56:19]

If setup() fails, tearDown() doesn't get called automatically. 

http://docs.python.org/library/unittest.html#unittest.TestCase.tearDown

On 2012/01/06 23:19:42, DaleCurtis wrote:

+      self.fail('Cannot connect to CNS server.')

[DaleCurtis, 2012/01/06 23:19:42]

Instead of having a new fail here, why not just return False and in the setUp
phase above do something like:

if self._CanAccessServer():
  return

[shadi, 2012/01/07 04:56:19]

Done.

+
   def tearDown(self):
     """Stops the Constrained Network Server (CNS)."""
     pyauto.PyUITest.tearDown(self)