addStep(): reject spurious kwargs when using the BuildStep-instance f…

…orm.

Without this, the args are silently ignored, which is a hassle to debug.

buildbot/process/factory.py

@@ -12,6 +12,19 @@ def s(steptype, **kwargs):
     # specification tuples
     return (steptype, kwargs)
 
+class ArgumentsInTheWrongPlace(Exception):
+    """When calling BuildFactory.addStep(stepinstance), addStep() only takes
+    one argument. You passed extra arguments to addStep(), which you probably
+    intended to pass to your BuildStep constructor instead. For example, you
+    should do::
+
+     f.addStep(ShellCommand(command=['echo','stuff'], haltOnFailure=True))
+
+    instead of::
+
+     f.addStep(ShellCommand(command=['echo','stuff']), haltOnFailure=True)
+    """
+
 class BuildFactory(util.ComparableMixin):
     """
     @cvar  buildClass: class to use when creating builds
@@ -42,6 +55,8 @@ def newBuild(self, request):
 
     def addStep(self, step_or_factory, **kwargs):
         if isinstance(step_or_factory, BuildStep):
+            if kwargs:
+                raise ArgumentsInTheWrongPlace()
             s = step_or_factory.getStepFactory()
         else:
             s = (step_or_factory, dict(kwargs))

buildbot/test/test_config.py

@@ -13,7 +13,7 @@
 from twisted.web.server import Site
 from twisted.web.distrib import ResourcePublisher
 from buildbot.process.builder import Builder
-from buildbot.process.factory import BasicBuildFactory
+from buildbot.process.factory import BasicBuildFactory, ArgumentsInTheWrongPlace
 from buildbot.changes.pb import PBChangeSource
 from buildbot.changes.mail import SyncmailMaildirSource
 from buildbot.steps.source import CVS, Darcs
@@ -1209,6 +1209,28 @@ def _testStartService_4(self, res):
                   'builddir':'workdir', 'factory':f1}]
 """
 
+cfg1_bad = \
+"""
+from buildbot.process.factory import BuildFactory, s
+from buildbot.steps.shell import ShellCommand
+from buildbot.steps.source import Darcs
+from buildbot.buildslave import BuildSlave
+BuildmasterConfig = c = {}
+c['slaves'] = [BuildSlave('bot1', 'pw1')]
+c['schedulers'] = []
+c['slavePortnum'] = 9999
+f1 = BuildFactory([ShellCommand(command='echo yes'),
+                   s(ShellCommand, command='old-style'),
+                   ])
+# it should be this:
+#f1.addStep(ShellCommand(command='echo args'))
+# but an easy mistake is to do this:
+f1.addStep(ShellCommand(), command='echo args')
+# this test makes sure this error doesn't get ignored
+c['builders'] = [{'name':'builder1', 'slavename':'bot1',
+                  'builddir':'workdir', 'factory':f1}]
+"""
+
 class Factories(unittest.TestCase):
     def printExpecting(self, factory, args):
         factory_keys = factory[1].keys()
@@ -1272,6 +1294,10 @@ def testSteps(self):
         self.failUnlessExpectedShell(steps[3], defaults=False,
                                      command="echo old-style")
 
+    def testBadAddStepArguments(self):
+        m = BuildMaster(".")
+        self.failUnlessRaises(ArgumentsInTheWrongPlace, m.loadConfig, cfg1_bad)
+
     def _loop(self, orig):
         step_class, kwargs = orig.getStepFactory()
         newstep = step_class(**kwargs)