Stop sending the testfilter= flag to the try server and process it locally.

The previous behavior would cause inproper parsing;
- the bot=bot1 flag would add 'bot1': ['defaulttests'] to the dictionary.
- the testfilter=test1 flag would append 'test1' to the list, causing the value
  to look like 'bot1': ['defaulttests', 'test1'].

I think on the long run the best is to keep --testfilter as a user interface but
not on the try server. This effectively stops sending testfilter=foo to the
server.

Add unit test to verify it's working properly. Refactor a bit to make unit
testing simpler.

R=petermayo@chromium.org
BUG=
TEST=

Review URL: http://codereview.chromium.org/9599012

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@125405 0039d316-1c4b-4281-b951-d872f2087c98

tests/trychange_unittest.py

 #!/usr/bin/env python
-# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Copyright (c) 2012 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.
 
@@ -7,6 +7,7 @@
 
 import os
 import sys
+import unittest
 
 sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
@@ -49,7 +50,7 @@ class TryChangeUnittest(TryChangeTestsBase):
       'InvalidScript', 'NoTryServerAccess', 'PrintSuccess', 'SCM', 'SVN',
       'TryChange', 'USAGE',
       'breakpad', 'datetime', 'errno', 'fix_encoding', 'gcl', 'gclient_utils',
-      'getpass',
+      'getpass', 'gen_parser',
       'json', 'logging', 'optparse', 'os', 'posixpath', 're', 'scm', 'shutil',
       'subprocess2', 'sys', 'tempfile', 'urllib',
     ]
@@ -57,6 +58,46 @@ class TryChangeUnittest(TryChangeTestsBase):
     self.compareMembers(trychange, members)
 
 
+class TryChangeSimpleTest(unittest.TestCase):
+  # Doesn't require supermox to run.
+  def test_flags(self):
+    cmd = [
+      '--bot', 'bot1',
+      '--testfilter', 'test1',
+      '--bot', 'bot2',
+      '--testfilter', 'test2',
+      '--user', 'joe',
+      '--email', 'joe@example.com',
+    ]
+    options, args = trychange.gen_parser(None).parse_args(cmd)
+    self.assertEquals([], args)
+    # pylint: disable=W0212
+    values = trychange._ParseSendChangeOptions(options)
+    self.assertEquals(
+        [
+          ('user', 'joe'),
+          ('name', None),
+          ('email', 'joe@example.com'),
+          ('bot', 'bot1:test1,test2'),
+          ('bot', 'bot2:test1,test2'),
+        ],
+        values)
+
+  def test_flags_bad_combination(self):
+    cmd = [
+      '--bot', 'bot1:test1',
+      '--testfilter', 'test2',
+    ]
+    options, args = trychange.gen_parser(None).parse_args(cmd)
+    self.assertEquals([], args)
+    try:
+      # pylint: disable=W0212
+      trychange._ParseSendChangeOptions(options)
+      self.fail()
+    except ValueError:
+      pass
+
+
 class SVNUnittest(TryChangeTestsBase):
   """trychange.SVN tests."""
   def testMembersChanged(self):
@@ -111,5 +152,4 @@ class GITUnittest(TryChangeTestsBase):
 
 
 if __name__ == '__main__':
-  import unittest
   unittest.main()

trychange.py

@@ -313,10 +313,18 @@ def _ParseSendChangeOptions(options):
   if options.project:
     values.append(('project', options.project))
 
-  for bot in options.bot:
-    values.append(('bot', bot))
-  for t in options.testfilter:
-    values.append(('testfilter', t))
+  filters = ','.join(options.testfilter)
+  if filters:
+    for bot in options.bot:
+      if ':' in bot:
+        raise ValueError(
+            'Can\'t use both --testfilter and --bot builder:test formats '
+            'at the same time')
+      else:
+        values.append(('bot', '%s:%s' % (bot, filters)))
+  else:
+    for bot in options.bot:
+      values.append(('bot', bot))
   return values
 
 
@@ -488,27 +496,11 @@ def GetMungedDiff(path_diff, diff):
   return (diff, changed_files)
 
 
-def TryChange(argv,
-              change,
-              swallow_exception,
-              prog=None,
-              extra_epilog=None):
-  """
-  Args:
-    argv: Arguments and options.
-    change: Change instance corresponding to the CL.
-    swallow_exception: Whether we raise or swallow exceptions.
-  """
+def gen_parser(prog):
   # Parse argv
   parser = optparse.OptionParser(usage=USAGE,
                                  version=__version__,
                                  prog=prog)
-  epilog = EPILOG % { 'prog': prog }
-  if extra_epilog:
-    epilog += extra_epilog
-  parser.epilog = epilog
-  # Remove epilog formatting
-  parser.format_epilog = lambda x: parser.epilog
   parser.add_option("-v", "--verbose", action="count", default=0,
                     help="Prints debugging infos")
   group = optparse.OptionGroup(parser, "Result and status")
@@ -636,6 +628,27 @@ def TryChange(argv,
                    help="SVN url to use to write the changes in; --use_svn is "
                         "implied when using --svn_repo")
   parser.add_option_group(group)
+  return parser
+
+
+def TryChange(argv,
+              change,
+              swallow_exception,
+              prog=None,
+              extra_epilog=None):
+  """
+  Args:
+    argv: Arguments and options.
+    change: Change instance corresponding to the CL.
+    swallow_exception: Whether we raise or swallow exceptions.
+  """
+  parser = gen_parser(prog)
+  epilog = EPILOG % { 'prog': prog }
+  if extra_epilog:
+    epilog += extra_epilog
+  parser.epilog = epilog
+  # Remove epilog formatting
+  parser.format_epilog = lambda x: parser.epilog
 
   options, args = parser.parse_args(argv)