1253219 - Reftest html report should display the screenshot when test fails

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Shing Lyu [:shinglyu]]

== Step to reproduce ==
* Edit some reftest in layout/reftests/inline to make it fail
* ./mach reftest --log-html test.html layout/reftests/inline/reftest.list

== Expected ==
Since the test has the image1 and image2 for the html under test and the reference html, I would expect to see them included in the reftest html report

== Actual == 
No such image exist. Also the image1 and image2 links has the type "text/plain" instead of "image/png"

Comment 1

[Shing Lyu [:shinglyu]]

Attachment: bug-1253219-fix.patch

Add the image1 and image2 to additional_html section. Fix the link so it displays the image instead of 'test/plain'

Comment 2

[Shing Lyu [:shinglyu]]

Attachment: log.html

An example of html report with images.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8726155 [details] [diff] [review]
bug-1253219-fix.patch

Review of attachment 8726155 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozlog/mozlog/formatters/html/html.py
@@ +116,5 @@
>                      html.a(html.img(src=screenshot), href="#"),
>                      class_='screenshot'))
> +
> +            for image_name in ['image1', 'image2']:
> +                if debug.get(image_name):

before we were doing 'debug.get('screenshot'), now we are doing debug.get('image1').  Do we have debug('image1') and debug('image2') ?  

Also the img src before was debug('screenshot'), now it would be debug('image1') ?

Comment 4

[Shing Lyu [:shinglyu]]

The debug struct from reftest only contains 'image1' and 'image2', there is no 'screenshot'. Their payload is a little different so I keep the 'screenshot' code as-is because I don't want to break backward-compatibility. Perhaps I should just merge them into one loop like:

for image_name in ['screenshot', 'image1', 'image2']:
    if debug.get(image_name):
        # create the html 

How do you think?

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

I still don't understand what has changed between screenshot and image1/image2?  It would make more sense to have image1/image2.  I assume screenshot doesn't exist at all and we could get rid of it?

The earlier comments in the bug don't help me understand why we keep screenshot around.

Comment 6

[Shing Lyu [:shinglyu]]

I don't know if anyone use 'screenshot' so I kept it. I'll do some research, if no one is using it I'll remove 'screenshot' and use 'image1/image2' instead.

Comment 7

[Shing Lyu [:shinglyu]]

After some investigation, B2G marionette test uses the 'screenshot' field. http://lxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette/runner/base.py#587

I'll keep the screenshot around, but try to keep 'screenshot', 'image1' and 'image2' in one loop. What do you think?

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

yes, I think your approach for looping on all 3 fields makes sense!

Comment 9

[Shing Lyu [:shinglyu]]

Attachment: Bug-1253219-Add-reftest-screeshots-to-html-report.patch

I updated the patch and handles 'screenshots', 'image1', and 'image2' in one loop.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8730162 [details] [diff] [review]
Bug-1253219-Add-reftest-screeshots-to-html-report.patch

Review of attachment 8730162 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/75422250c55f

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/75422250c55f