Don't duplicate test server arguments in Android test setup.

build/android/pylib/chrome_test_server_spawner.py

 # Copyright 2013 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.
 
 """A "Test Server Spawner" that handles killing/stopping per-test test servers.
 
 It's used to accept requests from the device to spawn and kill instances of the
 chrome test server on the host.
 """
 # pylint: disable=W0702
@@ skipping 161 matching lines @@
     return False
 
   def _GenerateCommandLineArguments(self):
     """Generates the command line to run the test server.
 
     Note that all options are processed by following the definitions in
     testserver.py.
     """
     if self.command_line:
       return
-    # The following arguments must exist.
-    type_cmd = _GetServerTypeCommandLine(self.arguments['server-type'])
+
+    args_copy = dict(self.arguments)
+
+    # Translate the server type.
+    type_cmd = _GetServerTypeCommandLine(args_copy.pop('server-type'))
     if type_cmd:
       self.command_line.append(type_cmd)
-    self.command_line.append('--port=%d' % self.host_port)
+
     # Use a pipe to get the port given by the instance of Python test server
     # if the test does not specify the port.
+    assert self.host_port == args_copy['port']
     if self.host_port == 0:
       (self.pipe_in, self.pipe_out) = os.pipe()
       self.command_line.append('--startup-pipe=%d' % self.pipe_out)
-    self.command_line.append('--host=%s' % self.arguments['host'])
-    data_dir = self.arguments['data-dir'] or 'chrome/test/data'
+
+    # --data-dir gets mangled.
+    data_dir = args_copy.pop('data-dir') or 'chrome/test/data'
     if not os.path.isabs(data_dir):
       data_dir = os.path.join(constants.DIR_SOURCE_ROOT, data_dir)
     self.command_line.append('--data-dir=%s' % data_dir)
-    # The following arguments are optional depending on the individual test.
-    if self.arguments.has_key('log-to-console'):
-      self.command_line.append('--log-to-console')
-    if self.arguments.has_key('auth-token'):
-      self.command_line.append('--auth-token=%s' % self.arguments['auth-token'])
-    if self.arguments.has_key('https'):
-      self.command_line.append('--https')
-      if self.arguments.has_key('cert-and-key-file'):

[bulach, 2014/04/22 09:09:48]

nit: just want to point out: these arguments would previously only be added if
it had --https, with the new implementation they'd always be included. I think
this won't be problem :) but just wanted to highlight..

[davidben, 2014/04/22 16:23:13]

The C++ side contains that logic anyway. It shouldn't be a problem. :-)

Though, I did notice that I missed some of the other filename munging, so I've
updated that. The missing data-dir case is no longer handled, but
BaseTestServer::GenerateArguments always includes it anyway.

-        self.command_line.append('--cert-and-key-file=%s' % os.path.join(
-            constants.DIR_SOURCE_ROOT, self.arguments['cert-and-key-file']))
-      if self.arguments.has_key('ocsp'):
-        self.command_line.append('--ocsp=%s' % self.arguments['ocsp'])
-      if self.arguments.has_key('https-record-resume'):
-        self.command_line.append('--https-record-resume')
-      if self.arguments.has_key('ssl-client-auth'):
-        self.command_line.append('--ssl-client-auth')
-      if self.arguments.has_key('tls-intolerant'):
-        self.command_line.append('--tls-intolerant=%s' %
-                                 self.arguments['tls-intolerant'])
-      if self.arguments.has_key('ssl-client-ca'):
-        for ca in self.arguments['ssl-client-ca']:
-          self.command_line.append('--ssl-client-ca=%s' %
-                                   os.path.join(constants.DIR_SOURCE_ROOT, ca))
-      if self.arguments.has_key('ssl-bulk-cipher'):
-        for bulk_cipher in self.arguments['ssl-bulk-cipher']:
-          self.command_line.append('--ssl-bulk-cipher=%s' % bulk_cipher)
+
+    # Pass the remaining arguments as-is.
+    for key, values in args_copy.iteritems():
+      if not isinstance(values, list):
+        values = [values]
+      for value in values:
+        if value is None:

[bulach, 2014/04/22 09:09:48]

nit: no need for "is None":
if value:
  self.command_line.append('--%s=%s' % (key, value))
else:
  self.command_line.append('--%s' % key)

[davidben, 2014/04/22 16:23:13]

That would probably cause troubles if value is 0.

+          self.command_line.append('--%s' % key)
+        else:
+          self.command_line.append('--%s=%s' % (key, value))
 
   def _CloseUnnecessaryFDsForTestServerProcess(self):
     # This is required to avoid subtle deadlocks that could be caused by the
     # test server child process inheriting undesirable file descriptors such as
     # file lock file descriptors.
     for fd in xrange(0, 1024):
       if fd != self.pipe_out:
         try:
           os.close(fd)
         except:
@@ skipping 197 matching lines @@
 
   def CleanupState(self):
     """Cleans up the spawning server state.
 
     This should be called if the test server spawner is reused,
     to avoid sharing the test server instance.
     """
     if self.server.test_server_instance:
       self.server.test_server_instance.Stop()
       self.server.test_server_instance = None