686679 - Display graphics info/failures in endurance dashboard report

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Result Dashboard, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Display the graphics info/failures gathered in the endurance shared module.

Comment 1

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Display graphics info/failures in endurance dashboard report. v1.0

Initial version of patch for feedback. Graphics section is hidden by default. Would appreciate feedback/suggestions.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 560256 [details] [diff] [review]
Display graphics info/failures in endurance dashboard report. v1.0

>+        if (resp.graphics !== undefined) {
>+          context.graphics = resp.graphics;
>+        }
>         context.system = resp.system_info.system,
>         context.system_version = resp.system_info.version,
>         context.service_pack = resp.system_info.service_pack,


I would like to see the graphic information being added below system_info. That's also part of the patch on the other bug but I want to call it out here, because you were asking for feedback. Otherwise looks fine. We should probably bump the report version for all report types and check that one instead of graphics undefined.

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Display graphics info/failures in endurance dashboard report. v1.1

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 560256 [details] [diff] [review]
> Display graphics info/failures in endurance dashboard report. v1.0
> 
> >+        if (resp.graphics !== undefined) {
> >+          context.graphics = resp.graphics;
> >+        }
> >         context.system = resp.system_info.system,
> >         context.system_version = resp.system_info.version,
> >         context.service_pack = resp.system_info.service_pack,
> 
> 
> I would like to see the graphic information being added below system_info.
> That's also part of the patch on the other bug but I want to call it out
> here, because you were asking for feedback. Otherwise looks fine. We should
> probably bump the report version for all report types and check that one
> instead of graphics undefined.

Thanks for the feedback. Patches updated.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 560352 [details] [diff] [review]
Display graphics info/failures in endurance dashboard report. v1.1

>+        context.cpu = resp.system_info.processor;
>+        if (resp.report_version >= 1.2) {
>+          context.graphics = resp.graphics;
>+        }

resp.system_info.graphic please.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Comment on attachment 560352 [details] [diff] [review]
> Display graphics info/failures in endurance dashboard report. v1.1
> 
> >+        context.cpu = resp.system_info.processor;
> >+        if (resp.report_version >= 1.2) {
> >+          context.graphics = resp.graphics;
> >+        }
> 
> resp.system_info.graphic please.

Perhaps I shouldn't have asked for review just yet, as this comment refers to a change that's been made in the patch on bug 686677.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Display graphics info/failures in endurance dashboard report. v1.2

Comment 7

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Comment on attachment 563390 [details] [diff] [review]
Display graphics info/failures in endurance dashboard report. v1.2

All earlier review comments have been satisfied, code looks good to me as does sample output at:

http://mozmill.blargon7.com/#/endurance/report/64cceae8f12ef60cd4077db5e8008e1d

r+'ing. If whimboo finds changes after the fact we can always re-update.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Thanks Geo!

Landed as: https://github.com/whimboo/mozmill-dashboard/commit/884dad5a97a34e8ac6e1b22a24640ccb7dbb0930

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 563390 [details] [diff] [review]
Display graphics info/failures in endurance dashboard report. v1.2

Thanks Geo for the review. I have only a nit to add:

>+          context.graphics.info.forEach(function(info) {
[..]
>+          $("#toggle_graphics").toggle(function() {

Please remember to add a space between function and the brackets. We don't have a function called 'function', which we call here. It's a definition instead. No need to reopen the bug.

Comment 10

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Good catch, sorry for missing that!