Closed
Bug 1253219
Opened 8 years ago
Closed 8 years ago
Reftest html report should display the screenshot when test fails
Categories
(Testing :: Reftest, defect)
Testing
Reftest
Tracking
(firefox48 fixed)
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: shinglyu, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
49.67 KB,
text/html
|
Details | |
2.13 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
== 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•8 years ago
|
||
Add the image1 and image2 to additional_html section. Fix the link so it displays the image instead of 'test/plain'
Reporter | ||
Comment 2•8 years ago
|
||
An example of html report with images.
Reporter | ||
Updated•8 years ago
|
Attachment #8726155 -
Flags: review?(jmaher)
Comment 3•8 years ago
|
||
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•8 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)
Comment 5•8 years ago
|
||
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•8 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•8 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)
Comment 8•8 years ago
|
||
yes, I think your approach for looping on all 3 fields makes sense!
Flags: needinfo?(jmaher)
Reporter | ||
Comment 9•8 years ago
|
||
I updated the patch and handles 'screenshots', 'image1', and 'image2' in one loop.
Attachment #8726155 -
Attachment is obsolete: true
Attachment #8730162 -
Flags: review?(jmaher)
Comment 10•8 years ago
|
||
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•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/75422250c55f
Keywords: checkin-needed
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/75422250c55f
Status: NEW → RESOLVED
Closed: 8 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.
Description
•