Reftest html report should display the screenshot when test fails

RESOLVED FIXED in Firefox 48

Status

Testing
Reftest
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: shinglyu, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla48
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
== 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"
(Reporter)

Comment 1

2 years ago
Created attachment 8726155 [details] [diff] [review]
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'
(Reporter)

Comment 2

2 years ago
Created attachment 8726156 [details]
log.html

An example of html report with images.
(Reporter)

Updated

2 years ago
Attachment #8726155 - Flags: review?(jmaher)
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') ?
Attachment #8726155 - Flags: review?(jmaher) → review-
(Reporter)

Comment 4

2 years ago
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?
Flags: needinfo?(jmaher)
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.
Flags: needinfo?(jmaher)
(Reporter)

Comment 6

2 years ago
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.
(Reporter)

Comment 7

2 years ago
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?
Flags: needinfo?(jmaher)
yes, I think your approach for looping on all 3 fields makes sense!
Flags: needinfo?(jmaher)
(Reporter)

Comment 9

2 years ago
Created attachment 8730162 [details] [diff] [review]
Bug-1253219-Add-reftest-screeshots-to-html-report.patch

I updated the patch and handles 'screenshots', 'image1', and 'image2' in one loop.
Attachment #8726155 - Attachment is obsolete: true
Attachment #8730162 - Flags: review?(jmaher)
(Reporter)

Updated

2 years ago
Blocks: 1256276
Comment on attachment 8730162 [details] [diff] [review]
Bug-1253219-Add-reftest-screeshots-to-html-report.patch

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

thanks!
Attachment #8730162 - Flags: review?(jmaher) → review+
(Reporter)

Updated

2 years ago
Keywords: checkin-needed

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/75422250c55f
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.