Bug 32797 – [bzt] Optimize status updates for new dashboard

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Barth]

We should make the bots post more information about errors and their status so the dashboard can present this information.

[Adam Barth]

Created attachment 45286 [details]
work in progress

[Eric Seidel (no email)]

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]

> 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]

Created attachment 45301 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45301 [details] without any errors.

[Adam Barth]

Created attachment 45302 [details]
Patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45302 [details] without any errors.

[Eric Seidel (no email)]

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)]

Attachment 45302 [details] was posted by a committer and has review+, assigning to Adam Barth for commit.

[Eric Seidel (no email)]

I'm shocked an appalled!  Already landed as r52430.  Closing.