We should make the bots post more information about errors and their status so the dashboard can present this information.
Created attachment 45286 [details] work in progress
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.
> 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.
Created attachment 45301 [details] Patch
style-queue ran check-webkit-style on attachment 45301 [details] without any errors.
Created attachment 45302 [details] Patch
style-queue ran check-webkit-style on attachment 45302 [details] without any errors.
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.
Attachment 45302 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.
I'm shocked an appalled! Already landed as r52430. Closing.