Merge pull request #1806 from jaredgrubb/jgrubb-step-description

Buildstep: add type-checking on description et al

master/buildbot/process/buildstep.py

@@ -159,6 +159,13 @@ class BuildStep(object, properties.PropertiesMixin):
         if not isinstance(self.name, str):
             config.error("BuildStep name must be a string: %r" % (self.name,))
 
+        if isinstance(self.description, str):
+            self.description = [self.description]
+        if isinstance(self.descriptionDone, str):
+            self.descriptionDone = [self.descriptionDone]
+        if isinstance(self.descriptionSuffix, str):
+            self.descriptionSuffix = [self.descriptionSuffix]
+
         self._acquiringLock = None
         self.stopped = False
         self.master = None

master/buildbot/steps/http.py

@@ -137,7 +137,7 @@ class HTTPStep(BuildStep):
 
         log.finish()
 
-        self.descriptionDone = "Status code: %d" % r.status_code
+        self.descriptionDone = ["Status", "code:", str(r.status_code)]
         self.step_status.setText(self.describe(done=True))
         if (r.status_code < 400):
             self.finished(SUCCESS)
@@ -165,11 +165,6 @@ class HTTPStep(BuildStep):
         log.addStdout(' ------ Content ------\n%s' % response.text)
         self.addLog('content').addStdout(response.text)
 
-    def describe(self, done=False):
-        if done:
-            return self.descriptionDone.split()
-        return self.description.split()
-
 
 class POST(HTTPStep):
 

master/buildbot/steps/master.py

@@ -50,24 +50,11 @@ class MasterShellCommand(BuildStep):
     flunkOnFailure = True
 
     def __init__(self, command,
-                 description=None, descriptionDone=None, descriptionSuffix=None,
                  env=None, path=None, usePTY=0, interruptSignal="KILL",
                  **kwargs):
         BuildStep.__init__(self, **kwargs)
 
         self.command = command
-        if description:
-            self.description = description
-        if isinstance(self.description, str):
-            self.description = [self.description]
-        if descriptionDone:
-            self.descriptionDone = descriptionDone
-        if isinstance(self.descriptionDone, str):
-            self.descriptionDone = [self.descriptionDone]
-        if descriptionSuffix:
-            self.descriptionSuffix = descriptionSuffix
-        if isinstance(self.descriptionSuffix, str):
-            self.descriptionSuffix = [self.descriptionSuffix]
         self.env = env
         self.path = path
         self.usePTY = usePTY

master/buildbot/steps/shell.py

@@ -90,7 +90,6 @@ class ShellCommand(buildstep.LoggingBuildStep):
     flunkOnFailure = True
 
     def __init__(self, workdir=None,
-                 description=None, descriptionDone=None, descriptionSuffix=None,
                  command=None,
                  usePTY="slave-config",
                  **kwargs):
@@ -99,20 +98,6 @@ class ShellCommand(buildstep.LoggingBuildStep):
         # BuildStep (like haltOnFailure and friends), and a couple that we
         # consume ourselves.
 
-        if description:
-            self.description = description
-        if isinstance(self.description, str):
-            self.description = [self.description]
-        if descriptionDone:
-            self.descriptionDone = descriptionDone
-        if isinstance(self.descriptionDone, str):
-            self.descriptionDone = [self.descriptionDone]
-
-        if descriptionSuffix:
-            self.descriptionSuffix = descriptionSuffix
-        if isinstance(self.descriptionSuffix, str):
-            self.descriptionSuffix = [self.descriptionSuffix]
-
         if command:
             self.setCommand(command)
 

master/buildbot/steps/source/base.py

@@ -94,7 +94,23 @@ class Source(LoggingBuildStep, CompositeStepMixin):
         default value will then match all changes.
         """
 
-        LoggingBuildStep.__init__(self, **kwargs)
+        descriptions_for_mode = {
+            "clobber": "checkout",
+            "export": "exporting"}
+        descriptionDones_for_mode = {
+            "clobber": "checkout",
+            "export": "export"}
+
+        if not description:
+            description = [descriptions_for_mode.get(mode, "updating")]
+        if not descriptionDone:
+            descriptionDone = [descriptionDones_for_mode.get(mode, "update")]
+        if not descriptionSuffix and codebase:
+            descriptionSuffix = [codebase]
+
+        LoggingBuildStep.__init__(self, description=description,
+            descriptionDone=descriptionDone, descriptionSuffix=descriptionSuffix,
+            **kwargs)
 
         # This will get added to args later, after properties are rendered
         self.workdir = workdir
@@ -112,35 +128,6 @@ class Source(LoggingBuildStep, CompositeStepMixin):
         self.timeout = timeout
         self.retry = retry
 
-        descriptions_for_mode = {
-            "clobber": "checkout",
-            "export": "exporting"}
-        descriptionDones_for_mode = {
-            "clobber": "checkout",
-            "export": "export"}
-        if description:
-            self.description = description
-        else:
-            self.description = [
-                descriptions_for_mode.get(mode, "updating")]
-        if isinstance(self.description, str):
-            self.description = [self.description]
-
-        if descriptionDone:
-            self.descriptionDone = descriptionDone
-        else:
-            self.descriptionDone = [
-                descriptionDones_for_mode.get(mode, "update")]
-        if isinstance(self.descriptionDone, str):
-            self.descriptionDone = [self.descriptionDone]
-
-        if descriptionSuffix:
-            self.descriptionSuffix = descriptionSuffix
-        else:
-            self.descriptionSuffix = self.codebase or None  # want None in lieu of ''
-        if isinstance(self.descriptionSuffix, str):
-            self.descriptionSuffix = [self.descriptionSuffix]
-
     def updateSourceProperty(self, name, value, source=''):
         """
         Update a property, indexing the property by codebase if codebase is not

master/buildbot/test/unit/test_process_buildstep.py

@@ -276,9 +276,18 @@ class TestBuildStep(steps.BuildStepMixin, config.ConfigErrorsMixin, unittest.Tes
         self.assertEqual(step.describe(), description)
         self.assertEqual(step.describe(done=True), descriptionDone)
 
-        step2 = buildstep.BuildStep()
-        self.assertEqual(step2.describe(), [step2.name])
-        self.assertEqual(step2.describe(done=True), [step2.name])
+    def test_describe_empty(self):
+        description = 'oogaBooga'
+        descriptionDone = 'oogaBooga done!'
+        step = buildstep.BuildStep(description=description,
+                                   descriptionDone=descriptionDone)
+        self.assertEqual(step.describe(), [description])
+        self.assertEqual(step.describe(done=True), [descriptionDone])
+
+    def test_describe_str(self):
+        step = buildstep.BuildStep()
+        self.assertEqual(step.describe(), [step.name])
+        self.assertEqual(step.describe(done=True), [step.name])
 
     def test_describe_suffix(self):
         description = ['oogaBooga']
@@ -805,7 +814,7 @@ class TestShellMixin(steps.BuildStepMixin,
         # BuildStep arg
         self.assertEqual(MySubclass().description, None)
         self.assertEqual(MySubclass(description='charming').description,
-                         'charming')
+                         ['charming'])
 
     @defer.inlineCallbacks
     def test_example(self):