diff --git a/master/buildbot/process/build.py b/master/buildbot/process/build.py index 99748f5f7d2..b33e1733078 100644 --- a/master/buildbot/process/build.py +++ b/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 diff --git a/master/buildbot/process/buildstep.py b/master/buildbot/process/buildstep.py index c6cf8629434..2aae5da508c 100644 --- a/master/buildbot/process/buildstep.py +++ b/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. diff --git a/master/buildbot/steps/shell.py b/master/buildbot/steps/shell.py index 0f57185fc65..7f7e5b20ab4 100644 --- a/master/buildbot/steps/shell.py +++ b/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 diff --git a/master/buildbot/steps/source/base.py b/master/buildbot/steps/source/base.py index 7582515c6f2..9edc8ccc01e 100644 --- a/master/buildbot/steps/source/base.py +++ b/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: diff --git a/master/buildbot/test/fake/fakedata.py b/master/buildbot/test/fake/fakedata.py index 779c081479c..d7873ceb873 100644 --- a/master/buildbot/test/fake/fakedata.py +++ b/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): diff --git a/master/buildbot/test/unit/test_process_build.py b/master/buildbot/test/unit/test_process_build.py index 130e399123e..eeec8c987b2 100644 --- a/master/buildbot/test/unit/test_process_build.py +++ b/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): diff --git a/master/buildbot/test/unit/test_process_buildstep.py b/master/buildbot/test/unit/test_process_buildstep.py index d6b1e9d1585..f8e33f97f2a 100644 --- a/master/buildbot/test/unit/test_process_buildstep.py +++ b/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): diff --git a/master/buildbot/test/unit/test_steps_python.py b/master/buildbot/test/unit/test_steps_python.py index 2111d9b6efe..5dbb5e33074 100644 --- a/master/buildbot/test/unit/test_steps_python.py +++ b/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 diff --git a/master/buildbot/test/unit/test_steps_shell.py b/master/buildbot/test/unit/test_steps_shell.py index 16f107bff15..acc9c2c5a53 100644 --- a/master/buildbot/test/unit/test_steps_shell.py +++ b/master/buildbot/test/unit/test_steps_shell.py @@ -972,7 +972,7 @@ 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, @@ -980,7 +980,7 @@ def test_setTestResults(self): }) # 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']) diff --git a/master/buildbot/test/unit/test_steps_transfer.py b/master/buildbot/test/unit/test_steps_transfer.py index ac7eeab2486..5545c316f60 100644 --- a/master/buildbot/test/unit/test_steps_transfer.py +++ b/master/buildbot/test/unit/test_steps_transfer.py @@ -223,7 +223,7 @@ def testURL(self): self.setupStep( transfer.FileUpload(slavesrc=__file__, masterdest=self.destfile, url="http://server/file")) - self.step.step_status.addURL = Mock() + self.step.addURL = Mock() self.expectCommands( Expect('uploadFile', dict( @@ -240,7 +240,7 @@ def testURL(self): @d.addCallback def checkURL(_): - self.step.step_status.addURL.assert_called_once_with( + self.step.addURL.assert_called_once_with( os.path.basename(self.destfile), "http://server/file") return d diff --git a/master/buildbot/test/unit/test_steps_trigger.py b/master/buildbot/test/unit/test_steps_trigger.py index 5b543f5023e..3291c33c7fc 100644 --- a/master/buildbot/test/unit/test_steps_trigger.py +++ b/master/buildbot/test/unit/test_steps_trigger.py @@ -161,11 +161,15 @@ def check(_): self.exp_a_trigger) self.assertEqual(self.scheduler_b.triggered_with, self.exp_b_trigger) - for i in xrange(len(self.exp_added_urls)): - self.assertEqual(self.step_status.addURL.call_args_list[i], - self.exp_added_urls[i]) - self.assertEqual(self.step_status.addURL.call_args_list, - self.exp_added_urls) + + # check the URLs + stepUrls = self.master.data.updates.stepUrls + if stepUrls: + got_added_urls = stepUrls[stepUrls.keys()[0]] + else: + got_added_urls = [] + self.assertEqual(sorted(got_added_urls), + sorted(self.exp_added_urls)) if self.exp_add_sourcestamp: self.assertEqual(self.addSourceStamp_kwargs, @@ -195,19 +199,19 @@ def expectAddedSourceStamp(self, **kwargs): def expectTriggeredLinks(self, *args): if 'a_br' in args: self.exp_added_urls.append( - (('a #11', 'baseurl/#buildrequests/11'), {})) + ('a #11', 'baseurl/#buildrequests/11')) if 'b_br' in args: self.exp_added_urls.append( - (('b #22', 'baseurl/#buildrequests/22'), {})) + ('b #22', 'baseurl/#buildrequests/22')) if 'a' in args: - self.exp_added_urls.append((('success: A #4011', - 'baseurl/#builders/1/builds/4011'), {})) + self.exp_added_urls.append( + ('success: A #4011', 'baseurl/#builders/1/builds/4011')) if 'b' in args: - self.exp_added_urls.append((('success: B #4022', - 'baseurl/#builders/2/builds/4022'), {})) + self.exp_added_urls.append( + ('success: B #4022', 'baseurl/#builders/2/builds/4022')) if 'afailed' in args: - self.exp_added_urls.append((('failure: A #4011', - 'baseurl/#builders/1/builds/4011'), {})) + self.exp_added_urls.append( + ('failure: A #4011', 'baseurl/#builders/1/builds/4011')) # tests def test_no_schedulerNames(self): diff --git a/master/buildbot/test/util/steps.py b/master/buildbot/test/util/steps.py index ec6a54be692..e7385fdf5e0 100644 --- a/master/buildbot/test/util/steps.py +++ b/master/buildbot/test/util/steps.py @@ -45,7 +45,6 @@ class BuildStepMixin(object): @ivar build: the fake build containing the step @ivar progress: mock progress object @ivar buildslave: mock buildslave object - @ivar step_status: mock StepStatus object @ivar properties: build properties (L{Properties} instance) """ @@ -115,37 +114,11 @@ def getSlaveVersion(cmd, oldversion): self.buildslave = step.buildslave = slave.FakeSlave(self.master) - # step.step_status - - ss = self.step_status = mock.Mock(name="step_status") - - ss.status_text = None - ss.logs = {} - - def ss_setText(strings): - ss.status_text = strings - ss.old_setText = ss_setText - - def ss_setText2(strings): - ss.status_text2 = strings - ss.old_setText2 = ss_setText2 - - ss.getLogs = lambda: ss.logs.values() - - self.step_statistics = {} - self.step.setStatistic = self.step_statistics.__setitem__ - self.step.getStatistic = self.step_statistics.get - self.step.hasStatistic = self.step_statistics.__contains__ - - self.step.setStepStatus(ss) - # step overrides def addLog(name, type='s', logEncoding=None): - assert type == 's', "type must be 's' until Data API backend is in place" l = logfile.FakeLogFile(name, step) self.step.logs[name] = l - ss.logs[name] = l return defer.succeed(l) step.addLog = addLog step.addLog_newStyle = addLog @@ -153,14 +126,13 @@ def addLog(name, type='s', logEncoding=None): def addHTMLLog(name, html): l = logfile.FakeLogFile(name, step) l.addStdout(html) - ss.logs[name] = l return defer.succeed(None) step.addHTMLLog = addHTMLLog def addCompleteLog(name, text): l = logfile.FakeLogFile(name, step) + self.step.logs[name] = l l.addStdout(text) - ss.logs[name] = l return defer.succeed(None) step.addCompleteLog = addCompleteLog @@ -252,7 +224,7 @@ def check(result): self.assertEqual(self.expected_remote_commands, [], "assert all expected commands were run") got_outcome = dict(result=result, - status_text=self.step_status.status_text) + status_text=None) self.assertEqual(got_outcome['result'], self.exp_outcome['result'], "expected step outcome") for pn, (pv, ps) in self.exp_properties.iteritems(): @@ -268,8 +240,9 @@ def check(result): "unexpected property '%s'" % pn) for l, contents in self.exp_logfiles.iteritems(): self.assertEqual( - self.step_status.logs[l].stdout, contents, "log '%s' contents" % l) - self.step_status.setHidden.assert_called_once_with(self.exp_hidden) + self.step.logs[l].stdout, contents, "log '%s' contents" % l) + # XXX TODO: hidden + # self.step_status.setHidden.assert_called_once_with(self.exp_hidden) return d # callbacks from the running step