Stop using BuildStepStatus when running steps

master/buildbot/process/build.py

@@ -354,11 +354,6 @@ def setupBuild(self, expectations):
             step.name = name
             self.steps.append(step)
 
-            # tell the BuildStatus about the step. This will create a
-            # BuildStepStatus and bind it to the Step.
-            step_status = self.build_status.addStepWithName(name)
-            step.setStepStatus(step_status)
-
             sp = None
             if self.useProgress:
                 # XXX: maybe bail if step.progressMetrics is empty? or skip

master/buildbot/process/buildstep.py

@@ -223,6 +223,11 @@ def waitUntilFinished(self):
         self._maybeFinished()
 
 
+class BuildStepStatus(object):
+    # used only for old-style steps
+    pass
+
+
 class BuildStep(results.ResultComputingConfigMixin,
                 properties.PropertiesMixin):
 
@@ -279,6 +284,7 @@ class BuildStep(results.ResultComputingConfigMixin,
     logEncoding = None
     cmd = None
     rendered = False  # true if attributes are rendered
+    _waitingForLocks = False
 
     def __init__(self, **kwargs):
         for p in self.__class__.parms:
@@ -338,16 +344,12 @@ def addFactoryArguments(self, **kwargs):
     def _getStepFactory(self):
         return self._factory
 
-    def setStepStatus(self, step_status):
-        self.step_status = step_status
-
     def setupProgress(self):
         if self.useProgress:
             # XXX this uses self.name, but the name may change when the
             # step is started..
             sp = progress.StepProgress(self.name, self.progressMetrics)
             self.progress = sp
-            self.step_status.setProgress(sp)
             return sp
         return None
 
@@ -387,9 +389,6 @@ def methodInfo(m):
             buildResult = summary.get('build', None)
             if buildResult and not isinstance(buildResult, unicode):
                 raise TypeError("build result must be unicode")
-            self.step_status.old_setText([stepResult])
-            self.step_status.old_setText2([buildResult] if buildResult else [])
-
     # updateSummary gets patched out for old-style steps, so keep a copy we can
     # call internally for such steps
     realUpdateSummary = updateSummary
@@ -400,7 +399,6 @@ def startStep(self, remote):
 
         # create and start the step, noting that the name may be altered to
         # ensure uniqueness
-        # XXX self.number != self.step_status.number..
         self.stepid, self.number, self.name = yield self.master.data.updates.newStep(
             buildid=self.build.buildid,
             name=util.ascii2unicode(self.name))
@@ -420,8 +418,6 @@ def startStep(self, remote):
                         " parent Build (%s)" % (l, self, self.build))
                 raise RuntimeError("lock claimed by both Step and Build")
 
-        self.step_status.stepStarted()
-
         try:
             # set up locks
             yield self.acquireLocks()
@@ -464,7 +460,6 @@ def setRenderable(res, attr):
                 finally:
                     self._running = False
             else:
-                self.step_status.setSkipped(True)
                 results = SKIPPED
 
         except BuildStepCancelled:
@@ -502,8 +497,6 @@ def setRenderable(res, attr):
         if self.progress:
             self.progress.finish()
 
-        self.step_status.stepFinished(results)
-
         yield self.master.data.updates.finishStep(self.stepid, results)
 
         hidden = self.hideStepIf
@@ -513,7 +506,7 @@ def setRenderable(res, attr):
             except Exception:
                 results = EXCEPTION
                 hidden = False
-        self.step_status.setHidden(hidden)
+        # TODO: hidden
 
         self.releaseLocks()
 
@@ -528,7 +521,7 @@ def acquireLocks(self, res=None):
         log.msg("acquireLocks(step %s, locks %s)" % (self, self.locks))
         for lock, access in self.locks:
             if not lock.isAvailable(self, access):
-                self.step_status.setWaitingForLocks(True)
+                self._waitingForLocks = True
                 log.msg("step %s waiting for lock %s" % (self, lock))
                 d = lock.waitUntilMaybeAvailable(self, access)
                 d.addCallback(self.acquireLocks)
@@ -537,7 +530,7 @@ def acquireLocks(self, res=None):
         # all locks are available, claim them all
         for lock, access in self.locks:
             lock.claim(self, access)
-        self.step_status.setWaitingForLocks(False)
+        self._waitingForLocks = False
         return defer.succeed(None)
 
     @defer.inlineCallbacks
@@ -550,11 +543,10 @@ def run(self):
             # aren't bothered by any of this equipment
 
             # monkey-patch self.step_status.{setText,setText2} back into
-            # existence for old steps; when these write to the data API,
-            # the monkey patches will stash their deferreds on the unhandled
-            # list
-            self.step_status.setText = self.step_status.old_setText
-            self.step_status.setText2 = self.step_status.old_setText2
+            # existence for old steps, signalling an update to the summary
+            self.step_status = BuildStepStatus()
+            self.step_status.setText = lambda text: self.realUpdateSummary()
+            self.step_status.setText2 = lambda text: self.realUpdateSummary()
 
             # monkey-patch in support for old statistics functions
             self.step_status.setStatistic = self.setStatistic
@@ -575,10 +567,9 @@ def updateSummary():
             self.updateSummary = updateSummary
 
             results = yield self.start()
-            if results == SKIPPED:
-                self.step_status.setSkipped(True)
-            else:
-                results = yield self._start_deferred
+            if results is not None:
+                self._start_deferred.callback(results)
+            results = yield self._start_deferred
         finally:
             self._start_deferred = None
             unhandled = self._start_unhandled_deferreds
@@ -733,7 +724,6 @@ def _connectPendingLogObservers(self):
     @defer.inlineCallbacks
     def addURL(self, name, url):
         yield self.master.data.updates.addStepURL(self.stepid, unicode(name), unicode(url))
-        self.step_status.addURL(name, url)
         defer.returnValue(None)
 
     @defer.inlineCallbacks
@@ -812,7 +802,6 @@ def startCommand(self, cmd, errorMessages=[]):
         log.msg("ShellCommand.startCommand(cmd=%s)" % (cmd,))
         log.msg("  cmd.args = %r" % (cmd.args))
         self.cmd = cmd  # so we can interrupt it
-        self.step_status.setText(self.describe(False))
 
         # stdio is the first log
         self.stdio_log = stdio_log = self.addLog("stdio")
@@ -858,7 +847,7 @@ def setupLogfiles(self, cmd, logfiles):
                     local_logname)
                 cmd.useLogDelayed(logname, callback, True)
             else:
-                # tell the BuildStepStatus to add a LogFile
+                # add a LogFile
                 newlog = self.addLog(logname)
                 # and tell the RemoteCommand to feed it
                 cmd.useLog(newlog, True)
@@ -868,7 +857,7 @@ def interrupt(self, reason):
         # instead of FAILURE, might make the text a bit more clear.
         # 'reason' can be a Failure, or text
         BuildStep.interrupt(self, reason)
-        if self.step_status.isWaitingForLocks():
+        if self._waitingForLocks:
             self.addCompleteLog(
                 'cancelled while waiting for locks', str(reason))
         else:
@@ -892,6 +881,8 @@ def createSummary(self, stdio):
     def evaluateCommand(self, cmd):
         # NOTE: log_eval_func is undocumented, and will die with LoggingBuildStep/ShellCOmmand
         if self.log_eval_func:
+            # self.step_status probably doesn't have the desired behaviors, but
+            # those were never well-defined..
             return self.log_eval_func(cmd, self.step_status)
         return cmd.results()
 
@@ -926,10 +917,7 @@ def maybeGetText2(self, cmd, results):
         return []
 
     def setStatus(self, cmd, results):
-        # this is good enough for most steps, but it can be overridden to
-        # get more control over the displayed text
-        self.step_status.setText(self.getText(cmd, results))
-        self.step_status.setText2(self.maybeGetText2(cmd, results))
+        self.realUpdateSummary()
         return defer.succeed(None)
 
 
@@ -1083,7 +1071,7 @@ def makeRemoteShellCommand(self, collectStdout=False, collectStderr=False,
                     logname)
                 cmd.useLogDelayed(logname, callback, True)
             else:
-                # tell the BuildStepStatus to add a LogFile
+                # add a LogFile
                 newlog = yield self.addLog(logname)
                 # and tell the RemoteCommand to feed it
                 cmd.useLog(newlog, False)
@@ -1147,7 +1135,7 @@ def _describe(self, done=False):
 # NOTE: log_eval_func is undocumented, and will die with LoggingBuildStep/ShellCOmmand
 
 
-def regex_log_evaluator(cmd, step_status, regexes):
+def regex_log_evaluator(cmd, _, regexes):
     worst = cmd.results()
     for err, possible_status in regexes:
         # worst_status returns the worse of the two status' passed to it.

master/buildbot/steps/shell.py

@@ -146,9 +146,6 @@ def setBuild(self, build):
         # Set this here, so it gets rendered when we start the step
         self.slaveEnvironment = self.build.slaveEnvironment
 
-    def setStepStatus(self, step_status):
-        buildstep.LoggingBuildStep.setStepStatus(self, step_status)
-
     def setDefaultWorkdir(self, workdir):
         rkw = self.remote_kwargs
         rkw['workdir'] = rkw['workdir'] or workdir

master/buildbot/steps/source/base.py

@@ -164,9 +164,6 @@ def updateSourceProperty(self, name, value, source=''):
                 % self.name
             LoggingBuildStep.setProperty(self, name, value, source)
 
-    def setStepStatus(self, step_status):
-        LoggingBuildStep.setStepStatus(self, step_status)
-
     def setDefaultWorkdir(self, workdir):
         self.workdir = self.workdir or workdir
 
@@ -298,7 +295,6 @@ def start(self):
                 self.addCompleteLog("log",
                                     "No sourcestamp found in build for codebase '%s'"
                                     % self.codebase)
-                self.finished(FAILURE)
                 return FAILURE
 
         else:

master/buildbot/test/fake/fakedata.py

@@ -49,6 +49,7 @@ def __init__(self, master, testcase):
         self.logs = {}
         self.claimedBuildRequests = set([])
         self.stepStateStrings = {}  # { stepid : [strings] }
+        self.stepUrls = {}  # { stepid : [(name,url)] }
 
     # extra assertions
 
@@ -290,6 +291,7 @@ def addStepURL(self, stepid, name, url):
                               validation.StringValidator())
         validation.verifyType(self.testcase, 'url', url,
                               validation.StringValidator())
+        self.stepUrls.setdefault(stepid, []).append((name, url))
         return defer.succeed(None)
 
     def startStep(self, stepid):

master/buildbot/test/unit/test_process_build.py

@@ -482,14 +482,10 @@ def acquireLocks(res=None):
             b.stopBuild('stop it')
             return retval
         step.acquireLocks = acquireLocks
-        step.setStepStatus = Mock()
-        step.step_status = Mock()
-        step.step_status.getLogs.return_value = []
 
         b.startBuild(FakeBuildStatus(), None, self.slavebuilder)
 
         self.assertEqual(gotLocks, [True])
-        self.assert_(('stepStarted', (), {}) in step.step_status.method_calls)
         self.assertEqual(b.result, CANCELLED)
 
     def testStepDone(self):

master/buildbot/test/unit/test_process_buildstep.py

@@ -360,7 +360,6 @@ def test_updateSummary_running(self):
         self.clock.advance(1)
         self.assertEqual(step.master.data.updates.stepStateStrings[13],
                          [u'C'])
-        step.step_status.setText.assert_not_called()
 
     def test_updateSummary_running_empty_dict(self):
         step = self.setup_summary_test()
@@ -370,8 +369,6 @@ def test_updateSummary_running_empty_dict(self):
         self.clock.advance(1)
         self.assertEqual(step.master.data.updates.stepStateStrings[13],
                          [u'finished'])
-        step.step_status.setText.assert_not_called()
-        step.step_status.setText2.assert_not_called()
 
     def test_updateSummary_running_not_unicode(self):
         step = self.setup_summary_test()
@@ -396,8 +393,6 @@ def test_updateSummary_finished(self):
         self.clock.advance(1)
         self.assertEqual(step.master.data.updates.stepStateStrings[13],
                          [u'CS'])
-        step.step_status.old_setText.assert_called_with([u'CS'])
-        step.step_status.old_setText2.assert_called_with([u'CB'])
 
     def test_updateSummary_finished_empty_dict(self):
         step = self.setup_summary_test()
@@ -407,8 +402,6 @@ def test_updateSummary_finished_empty_dict(self):
         self.clock.advance(1)
         self.assertEqual(step.master.data.updates.stepStateStrings[13],
                          [u'finished'])
-        step.step_status.old_setText.assert_called_with([u'finished'])
-        step.step_status.old_setText2.assert_called_with([])
 
     def test_updateSummary_finished_not_dict(self):
         step = self.setup_summary_test()
@@ -497,11 +490,6 @@ def test_signature_setDefaultWorkdir(self):
         def setDefaultWorkdir(self, workdir):
             pass
 
-    def test_signature_setStepStatus(self):
-        @self.assertArgSpecMatches(self.step.setStepStatus)
-        def setStepStatus(self, step_status):
-            pass
-
     def test_signature_setupProgress(self):
         @self.assertArgSpecMatches(self.step.setupProgress)
         def setupProgress(self):

master/buildbot/test/unit/test_steps_python.py

@@ -475,7 +475,7 @@ def test_warnings(self):
         d = self.runStep()
 
         def check(_):
-            self.assertEqual(self.step_statistics, {'warnings': 2})
+            self.assertEqual(self.step.statistics, {'warnings': 2})
         d.addCallback(check)
         return d
 

master/buildbot/test/unit/test_steps_shell.py

@@ -972,15 +972,15 @@ def tearDown(self):
     def test_setTestResults(self):
         step = self.setupStep(shell.Test())
         step.setTestResults(total=10, failed=3, passed=5, warnings=3)
-        self.assertEqual(self.step_statistics, {
+        self.assertEqual(step.statistics, {
             'tests-total': 10,
             'tests-failed': 3,
             'tests-passed': 5,
             'tests-warnings': 3,
         })
         # ensure that they're additive
         step.setTestResults(total=1, failed=2, passed=3, warnings=4)
-        self.assertEqual(self.step_statistics, {
+        self.assertEqual(step.statistics, {
             'tests-total': 11,
             'tests-failed': 5,
             'tests-passed': 8,
@@ -995,20 +995,20 @@ def test_describe_not_done(self):
     def test_describe_done(self):
         step = self.setupStep(shell.Test())
         step.rendered = True
-        self.step_statistics['tests-total'] = 93
-        self.step_statistics['tests-failed'] = 10
-        self.step_statistics['tests-passed'] = 20
-        self.step_statistics['tests-warnings'] = 30
+        step.statistics['tests-total'] = 93
+        step.statistics['tests-failed'] = 10
+        step.statistics['tests-passed'] = 20
+        step.statistics['tests-warnings'] = 30
         self.assertEqual(step.describe(done=True), ['test', '93 tests',
                                                     '20 passed', '30 warnings', '10 failed'])
 
     def test_describe_done_no_total(self):
         step = self.setupStep(shell.Test())
         step.rendered = True
-        self.step_statistics['tests-total'] = 0
-        self.step_statistics['tests-failed'] = 10
-        self.step_statistics['tests-passed'] = 20
-        self.step_statistics['tests-warnings'] = 30
+        step.statistics['tests-total'] = 0
+        step.statistics['tests-failed'] = 10
+        step.statistics['tests-passed'] = 20
+        step.statistics['tests-warnings'] = 30
         # describe calculates 60 = 10+20+30
         self.assertEqual(step.describe(done=True), ['test', '60 tests',
                                                     '20 passed', '30 warnings', '10 failed'])