[webkit-reviews] review granted: [Bug 32797] [bzt] Optimize status updates for new dashboard : [Attachment 45302] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 32797: [bzt] Optimize status updates for new dashboard
https://bugs.webkit.org/show_bug.cgi?id=32797

Attachment 45302: Patch
https://bugs.webkit.org/attachment.cgi?id=45302&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
Should this have a period?
 45		self._update_status("Building.", patch)

The expected_stderr repeition in EarlyWarningSytemTest could be avoided by
using a shared function.

Said shared function could also pre-compute what the expected absolute path
should be for the begin_work_queue message.  The relative path change I think
makes it harder to read.  The purpose of that message was to make sure that I
was in the right directory before I let the commit-queue loose on my working
directory.  Maybe the message isn't as useful anymore now that these run most
commonly on bots?

I'd prefer we pre-computed the absolute path in the unittests than make this
relative:
 83	    log("CAUTION: %s will discard all local changes in \"%s\"" %
(self.name, os.path.relpath(self.tool.scm().checkout_root)))

Why not just do:
engine(foo, bar).run() on one line here:
	 work_queue = QueueEngine(self.name, self)
 118	     work_queue = engine(self.name, self)
108119	       return work_queue.run()

I like the did_pass, did_fail changes!

assert_queue_outputs could be re-written much more consisely.  If you don't
plan to do it in this patch, please add a FIXME for us to do it later.

Please make some of the above adjustments before landing.