Bug 1531886 - [tryselect] Delete template context from kwargs, r=gbrown
authorAndrew Halberstadt <ahalberstadt@mozilla.com>
Tue, 05 Mar 2019 23:49:22 +0000
changeset 520560 ac6bdcb49765b3234f32293ace73987223f65659
parent 520559 ba832d587ae117729d39b1040f1f87e2655b31ac
child 520561 b5d943805f64c1b3dd97f3fbb31f5a0acfe465b7
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgbrown
bugs1531886
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1531886 - [tryselect] Delete template context from kwargs, r=gbrown When we parse template arguments, we stuff them all in kwargs['templates'], however we don't delete the old argument. This results in all kinds of unused variables lying around in kwargs. E.g we would have both kwargs['templates']['env'] and kwargs['env'] (the latter being unused). This is the main reason why all the selector's run functions need to have a **kwargs at the end of them. Depends on D22022 Differential Revision: https://phabricator.services.mozilla.com/D22023
tools/tryselect/mach_commands.py
tools/tryselect/templates.py
tools/tryselect/test/test_preset.t
--- a/tools/tryselect/mach_commands.py
+++ b/tools/tryselect/mach_commands.py
@@ -149,16 +149,19 @@ class TrySelect(MachCommandBase):
 
     def handle_templates(self, **kwargs):
         kwargs.setdefault('templates', {})
         for cls in self.parser.templates.itervalues():
             context = cls.context(**kwargs)
             if context is not None:
                 kwargs['templates'].update(context)
 
+            for name in cls.dests:
+                del kwargs[name]
+
         return kwargs
 
     def run(self, **kwargs):
         if 'preset' in self.parser.common_groups:
             kwargs = self.handle_presets(**kwargs)
 
         if self.parser.templates:
             kwargs = self.handle_templates(**kwargs)
--- a/tools/tryselect/templates.py
+++ b/tools/tryselect/templates.py
@@ -7,47 +7,63 @@ Templates provide a way of modifying the
 tasks. They live under taskcluster/taskgraph/templates.
 """
 
 from __future__ import absolute_import, print_function, unicode_literals
 
 import json
 import os
 import sys
-from abc import ABCMeta, abstractmethod
+from abc import ABCMeta, abstractmethod, abstractproperty
 from argparse import Action, SUPPRESS
 
 import mozpack.path as mozpath
 from mozbuild.base import BuildEnvironmentNotFoundException, MozbuildObject
 from .tasks import resolve_tests_by_suite
 
 here = os.path.abspath(os.path.dirname(__file__))
 build = MozbuildObject.from_environment(cwd=here)
 
 
 class Template(object):
     __metaclass__ = ABCMeta
 
-    @abstractmethod
+    def __init__(self):
+        self.dests = set()
+
     def add_arguments(self, parser):
+        for cli, kwargs in self.arguments:
+            action = parser.add_argument(*cli, **kwargs)
+            self.dests.add(action.dest)
+
+    @abstractproperty
+    def arguments(self):
         pass
 
     @abstractmethod
     def context(self, **kwargs):
         pass
 
 
 class Artifact(Template):
 
+    arguments = [
+        [['--artifact'],
+         {'action': 'store_true',
+          'help': 'Force artifact builds where possible.'
+          }],
+        [['--no-artifact'],
+         {'action': 'store_true',
+          'help': 'Disable artifact builds even if being used locally.',
+          }],
+    ]
+
     def add_arguments(self, parser):
         group = parser.add_mutually_exclusive_group()
-        group.add_argument('--artifact', action='store_true',
-                           help='Force artifact builds where possible.')
-        group.add_argument('--no-artifact', action='store_true',
-                           help='Disable artifact builds even if being used locally.')
+        return super(Artifact, self).add_arguments(group)
 
     def context(self, artifact, no_artifact, **kwargs):
         if artifact:
             return {
                 'artifact': {'enabled': '1'}
             }
 
         if no_artifact:
@@ -60,19 +76,23 @@ class Artifact(Template):
                     'artifact': {'enabled': '1'}
                 }
         except BuildEnvironmentNotFoundException:
             pass
 
 
 class Path(Template):
 
-    def add_arguments(self, parser):
-        parser.add_argument('paths', nargs='*', default=[],
-                            help='Run tasks containing tests under the specified path(s).')
+    arguments = [
+        [['paths'],
+         {'nargs': '*',
+          'default': [],
+          'help': 'Run tasks containing tests under the specified path(s).',
+          }],
+    ]
 
     def context(self, paths, **kwargs):
         if not paths:
             return
 
         for p in paths:
             if not os.path.exists(p):
                 print("error: '{}' is not a valid path.".format(p), file=sys.stderr)
@@ -83,20 +103,24 @@ class Path(Template):
             'env': {
                 'MOZHARNESS_TEST_PATHS': json.dumps(resolve_tests_by_suite(paths)),
             }
         }
 
 
 class Environment(Template):
 
-    def add_arguments(self, parser):
-        parser.add_argument('--env', action='append', default=None,
-                            help='Set an environment variable, of the form FOO=BAR. '
-                                 'Can be passed in multiple times.')
+    arguments = [
+        [['--env'],
+         {'action': 'append',
+          'default': None,
+          'help': 'Set an environment variable, of the form FOO=BAR. '
+                  'Can be passed in multiple times.',
+          }],
+    ]
 
     def context(self, env, **kwargs):
         if not env:
             return
         return {
             'env': dict(e.split('=', 1) for e in env),
         }
 
@@ -114,53 +138,75 @@ class RangeAction(Action):
             parser.error('{} can not be less than {}'.format(name, self.min))
         if values > self.max:
             parser.error('{} can not be more than {}'.format(name, self.max))
         setattr(namespace, self.dest, values)
 
 
 class Rebuild(Template):
 
-    def add_arguments(self, parser):
-        parser.add_argument('--rebuild', action=RangeAction, min=2, max=20, default=None, type=int,
-                            help='Rebuild all selected tasks the specified number of times.')
+    arguments = [
+        [['--rebuild'],
+         {'action': RangeAction,
+          'min': 2,
+          'max': 20,
+          'default': None,
+          'type': int,
+          'help': 'Rebuild all selected tasks the specified number of times.',
+          }],
+    ]
 
     def context(self, rebuild, **kwargs):
         if not rebuild:
             return
 
         return {
             'rebuild': rebuild,
         }
 
 
 class ChemspillPrio(Template):
 
-    def add_arguments(self, parser):
-        parser.add_argument('--chemspill-prio', action='store_true',
-                            help='Run at a higher priority than most try jobs (chemspills only).')
+    arguments = [
+        [['--chemspill-prio'],
+         {'action': 'store_true',
+          'help': 'Run at a higher priority than most try jobs (chemspills only).',
+          }],
+    ]
 
     def context(self, chemspill_prio, **kwargs):
         if chemspill_prio:
             return {
                 'chemspill-prio': {}
             }
 
 
 class GeckoProfile(Template):
-
-    def add_arguments(self, parser):
-        parser.add_argument('--gecko-profile', dest='profile', action='store_true', default=False,
-                            help='Create and upload a gecko profile during talos/raptor tasks.')
+    arguments = [
+        [['--gecko-profile'],
+         {'dest': 'profile',
+          'action': 'store_true',
+          'default': False,
+          'help': 'Create and upload a gecko profile during talos/raptor tasks.',
+          }],
         # For backwards compatibility
-        parser.add_argument('--talos-profile', dest='profile', action='store_true', default=False,
-                            help='Create and upload a gecko profile during talos tasks.')
+        [['--talos-profile'],
+         {'dest': 'profile',
+          'action': 'store_true',
+          'default': False,
+          'help': SUPPRESS,
+          }],
         # This is added for consistency with the 'syntax' selector
-        parser.add_argument('--geckoProfile', dest='profile', action='store_true', default=False,
-                            help=SUPPRESS)
+        [['--geckoProfile'],
+         {'dest': 'profile',
+          'action': 'store_true',
+          'default': False,
+          'help': SUPPRESS,
+          }],
+    ]
 
     def context(self, profile, **kwargs):
         if not profile:
             return
         return {'gecko-profile': profile}
 
 
 all_templates = {
--- a/tools/tryselect/test/test_preset.t
+++ b/tools/tryselect/test/test_preset.t
@@ -116,71 +116,79 @@ Test preset with syntax subcommand
     - foo
     talos:
     - none
     tests:
     - mochitests
 
 Test preset with fuzzy subcommand
 
-  $ ./mach try fuzzy $testargs --save baz -q "'baz"
+  $ ./mach try fuzzy $testargs --save baz -q "'foo" --rebuild 5
   preset saved, run with: --preset=baz
   $ ./mach try fuzzy $testargs --preset baz
   Commit message:
-  Fuzzy query='baz
+  Fuzzy query='foo
   
   Pushed via `mach try fuzzy`
   Calculated try_task_config.json:
   {
       "tasks": [
-          "build-baz"
+          "test/foo-debug",
+          "test/foo-opt"
       ],
       "templates": {
           "env": {
               "TRY_SELECTOR": "fuzzy"
-          }
+          },
+          "rebuild": 5
       },
       "version": 1
   }
   
 
-  $ ./mach try fuzzy $testargs --preset baz -q "'foo"
+  $ ./mach try $testargs --preset baz
   Commit message:
-  Fuzzy query='baz&query='foo
+  Fuzzy query='foo
+  
+  Pushed via `mach try fuzzy`
+  Calculated try_task_config.json:
+  {
+      "tasks": [
+          "test/foo-debug",
+          "test/foo-opt"
+      ],
+      "templates": {
+          "env": {
+              "TRY_SELECTOR": "fuzzy"
+          },
+          "rebuild": 5
+      },
+      "version": 1
+  }
+  
+ 
+Queries can be appended to presets
+
+  $ ./mach try fuzzy $testargs --preset baz -q "'build"
+  Commit message:
+  Fuzzy query='foo&query='build
   
   Pushed via `mach try fuzzy`
   Calculated try_task_config.json:
   {
       "tasks": [
           "build-baz",
           "test/foo-debug",
           "test/foo-opt"
       ],
       "templates": {
           "env": {
               "TRY_SELECTOR": "fuzzy"
-          }
-      },
-      "version": 1
-  }
-  
-  $ ./mach try $testargs --preset baz
-  Commit message:
-  Fuzzy query='baz
-  
-  Pushed via `mach try fuzzy`
-  Calculated try_task_config.json:
-  {
-      "tasks": [
-          "build-baz"
-      ],
-      "templates": {
-          "env": {
-              "TRY_SELECTOR": "fuzzy"
-          }
+          },
+          "rebuild": 5
       },
       "version": 1
   }
   
   $ ./mach try fuzzy $testargs --list-presets
   Presets from */mozbuild/try_presets.yml: (glob)
   
     bar:
@@ -194,17 +202,18 @@ Test preset with fuzzy subcommand
       talos:
       - all
       tests:
       - none
     baz:
       no_artifact: true
       push: false
       query:
-      - '''baz'
+      - '''foo'
+      rebuild: 5
       selector: fuzzy
     foo:
       no_artifact: true
       platforms:
       - linux
       selector: syntax
       tags:
       - foo
@@ -225,17 +234,18 @@ Test preset with fuzzy subcommand
     talos:
     - all
     tests:
     - none
   baz:
     no_artifact: true
     push: false
     query:
-    - '''baz'
+    - '''foo'
+    rebuild: 5
     selector: fuzzy
   foo:
     no_artifact: true
     platforms:
     - linux
     selector: syntax
     tags:
     - foo