RESOLVED FIXED 32797
[bzt] Optimize status updates for new dashboard
https://bugs.webkit.org/show_bug.cgi?id=32797
Summary [bzt] Optimize status updates for new dashboard
Adam Barth
Reported 2009-12-20 10:08:41 PST
We should make the bots post more information about errors and their status so the dashboard can present this information.
Attachments
work in progress (15.75 KB, patch)
2009-12-20 10:55 PST, Adam Barth
no flags
Patch (22.09 KB, patch)
2009-12-20 17:01 PST, Adam Barth
no flags
Patch (21.99 KB, patch)
2009-12-20 17:07 PST, Adam Barth
eric: review+
eric: commit-queue-
Adam Barth
Comment 1 2009-12-20 10:55:24 PST
Created attachment 45286 [details] work in progress
Eric Seidel (no email)
Comment 2 2009-12-20 11:15:23 PST
Comment on attachment 45286 [details] work in progress I'm surprised this isn't an error in python: 45 self._update_status("Building." % (patch["id"], patch["bug_id"]), patch) Personally I like storing state['patch'] in a local patch variable: 74 message = "Attachment %s did not build on %s:\nBuild output: %s" % (state["patch"]["id"], cls.port_name, results_link) Again, should erorr: 230 self._update_status("Checking style." % (patch["id"], patch["bug_id"]), patch) I'm not sure we should even output that much: 248 message = "Attachment %s did not pass %s:\n\n%s" % (state["patch"]["id"], cls.name, script_error.message_with_output(output_limit=3*1024)) You could re-use an OutputCapture object: 50 OutputCapture().assert_outputs(self, queue.queue_log_path, expected_stdout=expected_stdout.get("queue_log_path", ""), expected_stderr=expected_stderr.get("queue_log_path", "")) The test seems kinda confusing... but OK.
Adam Barth
Comment 3 2009-12-20 11:24:54 PST
> I'm surprised this isn't an error in python: Oh, it probably does. I wasn't posting it for review. Hopefully the tests will find these bugs though. :) > Personally I like storing state['patch'] in a local patch variable: Ok. > I'm not sure we should even output that much: > 248 message = "Attachment %s did not pass %s:\n\n%s" % > (state["patch"]["id"], cls.name, > script_error.message_with_output(output_limit=3*1024)) Hum... Pick a number. :) > The test seems kinda confusing... but OK. Yeah, I thought of a better way to do this. Basically, we want to make a MockQueueEngine to do all the callbacks. :) Anyway, I've got to run. Thanks for the comments.
Adam Barth
Comment 4 2009-12-20 17:01:19 PST
WebKit Review Bot
Comment 5 2009-12-20 17:03:18 PST
style-queue ran check-webkit-style on attachment 45301 [details] without any errors.
Adam Barth
Comment 6 2009-12-20 17:07:08 PST
WebKit Review Bot
Comment 7 2009-12-20 17:08:42 PST
style-queue ran check-webkit-style on attachment 45302 [details] without any errors.
Eric Seidel (no email)
Comment 8 2009-12-20 18:42:44 PST
Comment on attachment 45302 [details] Patch 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.
Eric Seidel (no email)
Comment 9 2009-12-28 22:41:12 PST
Attachment 45302 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.
Eric Seidel (no email)
Comment 10 2009-12-28 23:43:39 PST
I'm shocked an appalled! Already landed as r52430. Closing.
Note You need to log in before you can comment on or make changes to this bug.