Bug 108430 – Improve how patch start, processing and finish times are shown on the EWS charts

• Component: Tools / Tests
• Status: NEW

[Alan Cutter]

Currently only patches that have completed have their waiting time and process duration shown.
The dots are situated at the time the patch was created.

It would be better if they were situated at the time of their representative events. Their height can indicate the total run time of the patch so they line up horizontally.

[Alan Cutter]

Created attachment 186045 [details]
Patch

[Alan Cutter]

Created attachment 186046 [details]
Patch

[Alan Cutter]

(In reply to comment #2)
> Created an attachment (id=186046) [details]
> Patch

Preview can be seen here: http://108430.webkit-commit-queue.appspot.com/queue-charts/cr-linux-debug-ews
The state of the data it's presenting isn't in the best shape unfortunately.

[Alan Cutter]

Looks like this patch is conflicting with another one. Rebasing to resolve...

[Alan Cutter]

Created attachment 186049 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 186049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186049&action=review

> Tools/QueueStatusServer/handlers/queuecharts.py:72
> -    def _get_min_med_max(cls, values, defaults=(0, 0, 0)):
> +    def _get_min_med_max(cls, values, defaults=(None, None, None)):

Um.  You want None here, not a list.  http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument

[Alan Cutter]

(In reply to comment #6)
> (From update of attachment 186049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186049&action=review
> 
> > Tools/QueueStatusServer/handlers/queuecharts.py:72
> > -    def _get_min_med_max(cls, values, defaults=(0, 0, 0)):
> > +    def _get_min_med_max(cls, values, defaults=(None, None, None)):
> 
> Um.  You want None here, not a list.  http://stackoverflow.com/questions/1132941/least-astonishment-in-python-the-mutable-default-argument

Tuples are immutable, the default value cannot change state.

[Eric Seidel (no email)]

Oh, you're so right.  :)

[Eric Seidel (no email)]

Comment on attachment 186049 [details]
Patch

OK.

[Eric Seidel (no email)]

Comment on attachment 186049 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=186049&action=review

> Tools/QueueStatusServer/templates/queuecharts.html:348
> +                // data.addRows([
> +                //     {% for patch_datum in patch_data %}
> +                //         [
> +                //             {{ patch_datum.seconds_ago }} / {{ time_unit }},
> +                //             {{ patch_datum.status_update_count }},
> +                //             "Patch {{ patch_datum.attachment_id }}\nStatus updates: " + {{ patch_datum.status_update_count }} + "\n" + secondsToString({{ patch_datum.seconds_ago }}) + " ago",
> +                //             {{ patch_datum.retry_count }},
> +                //             "Patch {{ patch_datum.attachment_id }}\nRetries: " + {{ patch_datum.retry_count }} + "\n" + secondsToString({{ patch_datum.seconds_ago }}) + " ago",
> +                //         ],
> +                //     {% endfor %}
> +                // ]);

Missed this.  We dont' normally commit commented out code, did you mean to leave this?

[Alan Cutter]

Created attachment 186784 [details]
Patch

[Alan Cutter]

(In reply to comment #10)
> (From update of attachment 186049 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186049&action=review
> 
> > Tools/QueueStatusServer/templates/queuecharts.html:348
> > +                // data.addRows([
> > +                //     {% for patch_datum in patch_data %}
> > +                //         [
> > +                //             {{ patch_datum.seconds_ago }} / {{ time_unit }},
> > +                //             {{ patch_datum.status_update_count }},
> > +                //             "Patch {{ patch_datum.attachment_id }}\nStatus updates: " + {{ patch_datum.status_update_count }} + "\n" + secondsToString({{ patch_datum.seconds_ago }}) + " ago",
> > +                //             {{ patch_datum.retry_count }},
> > +                //             "Patch {{ patch_datum.attachment_id }}\nRetries: " + {{ patch_datum.retry_count }} + "\n" + secondsToString({{ patch_datum.seconds_ago }}) + " ago",
> > +                //         ],
> > +                //     {% endfor %}
> > +                // ]);
> 
> Missed this.  We dont' normally commit commented out code, did you mean to leave this?

Ah, certainly not. That was a mistake, thanks for spotting it.

[Build Bot]

Comment on attachment 186784 [details]
Patch

Attachment 186784 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16393251

[Alan Cutter]

(In reply to comment #13)
> (From update of attachment 186784 [details])
> Attachment 186784 [details] did not pass win-ews (win):
> Output: http://queues.webkit.org/results/16393251

Build failure for win-ews unrelated to this patch. It is not part of the Webkit build.
Working version can be seen here: http://108430.webkit-commit-queue.appspot.com/queue-charts/cr-linux-debug-ews

[Andreas Kling]

Comment on attachment 186784 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.