Summary: | [bzt] Optimize status updates for new dashboard | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||
Component: | Tools / Tests | Assignee: | Adam Barth <abarth> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Adam Barth
2009-12-20 10:08:41 PST
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. |