devserver: Revise integration tests server startup logic.

* We shift to having the devserver bind to an arbitrary port by default
  for test purposes. This is more robust and saves us from relying on
  heuristic, iterative attempts.

* Revise the test classes hierarchy to distinguish "auto-started server"
  tests from others; this will be needed for specifically testing the
  devserver's port binding behavior.

* Small refactoring for better code reusability, clarity.

BUG=chromium:322436
TEST=Integration tests pass.

Change-Id: Ia17b580c393ecc641dccc8e88608e9807d0eea87
Reviewed-on: https://chromium-review.googlesource.com/186697
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
Tested-by: Gilad Arnold <garnold@chromium.org>
diff --git a/devserver_integration_test.py b/devserver_integration_test.py
index d52ff4a..3dbddb6 100755
--- a/devserver_integration_test.py
+++ b/devserver_integration_test.py
@@ -65,57 +65,90 @@
 API_TEST_IP_ADDR = '127.0.0.1'
 
 DEVSERVER_START_TIMEOUT = 15
+DEVSERVER_START_SLEEP = 1
 
 
 class DevserverFailedToStart(Exception):
   """Raised if we could not start the devserver."""
 
 
-class DevserverTestCommon(unittest.TestCase):
+class DevserverTestBase(unittest.TestCase):
   """Class containing common logic between devserver test classes."""
 
   def setUp(self):
-    """Copies in testing files."""
+    """Creates and populates a test directory, temporary files."""
     self.test_data_path = tempfile.mkdtemp()
     self.src_dir = os.path.dirname(__file__)
 
     # Current location of testdata payload.
     image_src = os.path.join(self.src_dir, TEST_IMAGE_PATH, TEST_IMAGE_NAME)
 
-    # Copy the payload to the location of the update label "devserver."
-    os.makedirs(os.path.join(self.test_data_path, LABEL))
-    shutil.copy(image_src, os.path.join(self.test_data_path, LABEL,
-                                        TEST_IMAGE_NAME))
+    # Copy the payload to the location of the update label.
+    self._CreateLabelAndCopyImage(LABEL, image_src)
 
     # Copy the payload to the location of forced label.
-    os.makedirs(os.path.join(self.test_data_path, API_SET_UPDATE_REQUEST))
-    shutil.copy(image_src, os.path.join(self.test_data_path,
-                                        API_SET_UPDATE_REQUEST,
-                                        TEST_IMAGE_NAME))
+    self._CreateLabelAndCopyImage(API_SET_UPDATE_REQUEST, image_src)
 
-    self.pidfile = tempfile.mktemp('devserver_unittest')
-    self.logfile = tempfile.mktemp('devserver_unittest')
+    # Allocate temporary files for various devserver outputs.
+    self.pidfile = self._MakeTempFile('pid')
+    self.portfile = self._MakeTempFile('port')
+    self.logfile = self._MakeTempFile('log')
 
-    # Devserver url set in start server.
-    self.devserver_url = None
-    self.pid = self._StartServer()
+    # Initialize various runtime values.
+    self.devserver_url = self.port = self.pid = None
 
   def tearDown(self):
-    """Removes testing files."""
-    shutil.rmtree(self.test_data_path)
+    """Kill the server, remove the test directory and temporary files."""
     if self.pid:
       os.kill(self.pid, signal.SIGKILL)
 
-    if os.path.exists(self.pidfile):
-      os.remove(self.pidfile)
+    self._RemoveFile(self.pidfile)
+    self._RemoveFile(self.portfile)
+    self._RemoveFile(self.logfile)
+    shutil.rmtree(self.test_data_path)
 
   # Helper methods begin here.
 
-  def _StartServerOnPort(self, port):
+  def _CreateLabelAndCopyImage(self, label, image):
+    """Creates a label location and copies an image to it."""
+    label_dir = os.path.join(self.test_data_path, label)
+    os.makedirs(label_dir)
+    shutil.copy(image, os.path.join(label_dir, TEST_IMAGE_NAME))
+
+  def _MakeTempFile(self, suffix):
+    """Return path of a newly created temporary file."""
+    with tempfile.NamedTemporaryFile(suffix='-devserver-%s' % suffix) as f:
+      name = f.name
+      f.close()
+
+    return name
+
+  def _RemoveFile(self, filename):
+    """Removes a file if it is present."""
+    if os.path.isfile(filename):
+      os.remove(filename)
+
+  def _ReadIntValueFromFile(self, path, desc):
+    """Reads a string from file and returns its conversion into an integer."""
+    if not os.path.isfile(path):
+      raise DevserverFailedToStart('Devserver did not drop %s (%r).' %
+                                   (desc, path))
+
+    with open(path) as f:
+      value_str = f.read()
+
+    try:
+      return int(value_str)
+    except ValueError:
+      raise DevserverFailedToStart('Devserver did not drop a valid value '
+                                   'in %s (%r).' % (desc, value_str))
+
+  def _StartServer(self, port=0):
     """Attempts to start devserver on |port|.
 
-    Returns:
-      The pid of the devserver.
+    In the default case where port == 0, the server will bind to an arbitrary
+    availble port. If successful, this method will set the devserver's pid
+    (self.pid), actual listening port (self.port) and URL (self.devserver_url).
 
     Raises:
       DevserverFailedToStart: If the devserver could not be started.
@@ -126,54 +159,30 @@
         'devserver.py',
         '--static_dir', self.test_data_path,
         '--pidfile', self.pidfile,
+        '--portfile', self.portfile,
         '--port', str(port),
         '--logfile', self.logfile]
 
     # Pipe all output. Use logfile to get devserver log.
     subprocess.Popen(cmd, stderr=subprocess.PIPE, stdout=subprocess.PIPE)
 
-    # wait for devserver to start
+    # Wait for devserver to start, determining its actual serving port and URL.
     current_time = time.time()
     deadline = current_time + DEVSERVER_START_TIMEOUT
     while current_time < deadline:
-      current_time = time.time()
       try:
-        self.devserver_url = 'http://127.0.0.1:%d' % port
+        self.port = self._ReadIntValueFromFile(self.portfile, 'portfile')
+        self.devserver_url = 'http://127.0.0.1:%d' % self.port
         self._MakeRPC(CHECK_HEALTH, timeout=0.1)
         break
       except Exception:
-        continue
+        time.sleep(DEVSERVER_START_SLEEP)
+        current_time = time.time()
     else:
       raise DevserverFailedToStart('Devserver failed to start within timeout.')
 
-    if not os.path.exists(self.pidfile):
-      raise DevserverFailedToStart('Devserver did not drop a pidfile.')
-    else:
-      pid_value = open(self.pidfile).read()
-      try:
-        return int(pid_value)
-      except ValueError:
-        raise DevserverFailedToStart('Devserver did not drop a pid in the '
-                                     'pidfile.')
-
-  def _StartServer(self):
-    """Starts devserver on a port, returns pid.
-
-    Raises:
-      DevserverFailedToStart: If all attempts to start the devserver fail.
-    """
-    # TODO(sosa): Fixing crbug.com/308686 will allow the devserver to do this
-    # for us and be cleaner.
-    for port in range(8080, 8090):
-      try:
-        return self._StartServerOnPort(port)
-      except DevserverFailedToStart:
-        logging.error('Devserver could not start on port %s', port)
-        continue
-    else:
-      logging.error(open(self.logfile).read())
-      raise DevserverFailedToStart('Devserver could not be started on all '
-                                   'ports.')
+    # Retrieve PID.
+    self.pid = self._ReadIntValueFromFile(self.pidfile, 'pidfile')
 
   def VerifyHandleUpdate(self, label, use_test_payload=True):
     """Verifies that we can send an update request to the devserver.
@@ -186,6 +195,7 @@
       use_test_payload: If set to true, expects to serve payload under
         testdata/ and does extra checks i.e. compares hash and content of
         payload.
+
     Returns:
       url of the update payload if we verified the update.
     """
@@ -234,13 +244,16 @@
     return url
 
   def _MakeRPC(self, rpc, data=None, timeout=None, **kwargs):
-    """Makes a RPC to the devserver using the kwargs and returns output.
+    """Makes an RPC call to the devserver.
 
     Args:
+      rpc: The function to run on the devserver, e.g. 'stage'.
       data: Optional post data to send.
       timeout: Optional timeout to pass to urlopen.
+      kwargs: Optional arguments to the function, e.g. artifact_url='foo/bar'.
 
-    For example: localhost:8080/stage with artifact_url=blah/blah.
+    Returns:
+      The function output.
     """
     request = '/'.join([self.devserver_url, rpc])
     if kwargs:
@@ -264,8 +277,17 @@
     return output
 
 
-class DevserverUnittests(DevserverTestCommon):
-  """Short running integration/unittests for the devserver (no remote deps).
+class AutoStartDevserverTestBase(DevserverTestBase):
+  """Test base class that automatically starts the devserver."""
+
+  def setUp(self):
+    """Initialize everything, then start the server."""
+    super(AutoStartDevserverTestBase, self).setUp()
+    self._StartServer()
+
+
+class DevserverBasicTests(AutoStartDevserverTestBase):
+  """Short running tests for the devserver (no remote deps).
 
   These are technically not unittests because they depend on being able to
   start a devserver locally which technically requires external resources so
@@ -373,7 +395,7 @@
     self.assertTrue('devserver.py' in process.cmdline)
 
 
-class DevserverIntegrationTests(DevserverTestCommon):
+class DevserverExtendedTests(AutoStartDevserverTestBase):
   """Longer running integration tests that test interaction with Google Storage.
 
   Note: due to the interaction with Google Storage, these tests both require