Closed Bug 1287019 Opened 3 years ago Closed 3 years ago

[mozlog] Make mozlog's HTML format support wptrunner screenshots

Categories

(Testing :: Mozbase, defect)

Version 3
defect
Not set

Tracking

(firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: shinglyu, Assigned: jeremychen, Mentored)

Details

Attachments

(3 files)

WPTRunner export this kind of extra data:
https://github.com/w3c/wptrunner/blob/5d0510b231bcbc0b98314b70c1a6d173074ce64a/wptrunner/executors/base.py#L296

{
  "reftest_screenshots": [
      {"url": nodes[0].url, "screenshot": screenshots[0]}, 
      relation,
      {"url": nodes[1].url, "screenshot": screenshots[1]}
    ]
}

which is not correctly handled by mozlog's HTML formatter.

Servo uses wptrunner + mozlog, it would be nice if we can make them work together.

See also: https://github.com/servo/servo/pull/12403
Assignee: nobody → jeremychen
Mentor: slyu
Great, what you need to do is:

1. https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozlog/mozlog/formatters/html/html.py#113-155 Change this code, make it take the format described above. You need to make sure the two screenshots are formatted as base64 encoded image in the additional_html element, and clean up the wrong link_html element. 
2. Merge this into m-c, and ask the A-team to publish this mozlog onto PiPy
3. After the PiPy version updates, create a PR to servo that updates the mozlog version in requirements.txt

I'm not quite sure if we want duplicated images in additional_html and link_html, since it increase the file size, but we can discuss it offline.
You can test it by applying the mozlog patch to the one in Servo's python/_virtualenv/lib/python2.7/site-packages/mozlog, then run this command:

./mach test-css --release --log-html test.html tests/wpt/css-tests/css-flexbox-1_dev/html/flex-direction.htm --log-raw raw.log

And open the test.html
Attached file raw.log
Also, this is the raw log that mozlog take as input. You can search for the "reftest_screenshots" string to find the images.
Current mozlog (v3.2) doesn't support screenshot logs exported from wptrunner.
Add this support so we could run css test with --log-html to see more detail
information, such as screenshots of test/reference pages.

Review commit: https://reviewboard.mozilla.org/r/64564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64564/
Attachment #8771342 - Flags: review?(jmaher)
Comment on attachment 8771342 [details]
Bug 1287019 - part1: make mozlog's HTML format support wptrunner screenshot.

https://reviewboard.mozilla.org/r/64564/#review61570

I need more context before r+

::: testing/mozbase/mozlog/mozlog/formatters/html/html.py:107
(Diff revision 1)
> +            log_data = debug.get("reftest_screenshots", {})
> +            debug = {
> +                'image1':'data:image/png;base64,' + log_data[0].get("screenshot", {}),
> +                'image2':'data:image/png;base64,' + log_data[2].get("screenshot", {}),
> +                'differences': "Not Implemented",
> +                'reftest_screenshots':log_data

why will we store both image1/image2 and reftest_screenshots?

I am unfamiliar with how this data will be used.
Attachment #8771342 - Flags: review?(jmaher) → review-
(In reply to Joel Maher ( :jmaher ) from comment #5)
> Comment on attachment 8771342 [details]
> Bug 1287019 - make mozlog's HTML format support wptrunner screenshot.
> 
> https://reviewboard.mozilla.org/r/64564/#review61570
> 
> I need more context before r+
> 
> ::: testing/mozbase/mozlog/mozlog/formatters/html/html.py:107
> (Diff revision 1)
> > +            log_data = debug.get("reftest_screenshots", {})
> > +            debug = {
> > +                'image1':'data:image/png;base64,' + log_data[0].get("screenshot", {}),
> > +                'image2':'data:image/png;base64,' + log_data[2].get("screenshot", {}),
> > +                'differences': "Not Implemented",
> > +                'reftest_screenshots':log_data
> 
> why will we store both image1/image2 and reftest_screenshots?
> 
> I am unfamiliar with how this data will be used.

You're right, the reftest_screenshots part seems like a redundancy. Will remove it in the next version.

So far, I think provide screenshots in html report should be enough in this bug.
It might be more useful if we provide a link to show results in reftest analyzer, which seems already being taken care of in Bug 1287026.
Comment on attachment 8771342 [details]
Bug 1287019 - part1: make mozlog's HTML format support wptrunner screenshot.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64564/diff/1-2/
Attachment #8771342 - Flags: review- → review?(jmaher)
Comment on attachment 8771342 [details]
Bug 1287019 - part1: make mozlog's HTML format support wptrunner screenshot.

Will discuss w/ Shing to check if I'm on the right direction. Clear review request first.
Attachment #8771342 - Flags: review?(jmaher)
Comment on attachment 8771342 [details]
Bug 1287019 - part1: make mozlog's HTML format support wptrunner screenshot.

Hi Shing,

It appears that additional_html and link_html both have a pair of hyperlinks to image1/image2. Maybe we could removed the hyperlinks (but keep the title texts remain) in additional_html, and just keep the hyperlinks in link_html. I'm not sure if this is what you mean the duplicated images in additional_html and link_html (as mentioned in comment 1). Please correct me if I misread you.

Also, please take a look at this patch and give me some feedback. Thank you.

One more thing, since current version of mozlog doesn't support wptrunner screenshot, we can simply verify this patch in gecko. Just run the following command:

./mach web-platform-tests --log-html test.html ./testing/web-platform/tests/[ANY_FAILED_TEST]

Then, open test.html.
Attachment #8771342 - Flags: feedback?(slyu)
Comment on attachment 8771342 [details]
Bug 1287019 - part1: make mozlog's HTML format support wptrunner screenshot.

LGTM. 

We should probably consider splitting the logic into chunks by the test type (wpt-reftest, gecko reftest, general unit test result, etc) in future refactors.
Attachment #8771342 - Flags: feedback?(slyu) → feedback+
There are two identical pairs of screenshots (test/reference pair) which are
png/base64 raw files generated from mozlog's HTML formatter. One pair is stored
in the img element to present the visual result; the other pair is stored as a
hyperlink source in the visual result's title.

After part1 patch, we may have one more pair. It appears that the hyperlinks of
the visual result's titles could be eliminated since they are visually closed to
the visual results, and clicking the visual results provides the exact same
function.

DONTBUILD (NPOTB)

Review commit: https://reviewboard.mozilla.org/r/64824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64824/
Attachment #8771342 - Attachment description: Bug 1287019 - make mozlog's HTML format support wptrunner screenshot. → Bug 1287019 - part1: make mozlog's HTML format support wptrunner screenshot.
Attachment #8771342 - Flags: feedback+
Comment on attachment 8771342 [details]
Bug 1287019 - part1: make mozlog's HTML format support wptrunner screenshot.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64564/diff/2-3/
Hi Joel,

The part1 patch is for adding support to wptrunner screenshots. Your comment has been addressed accordingly.

The part2 patch is for eliminating redundant base64 image data in the generated HTML log report. It would be trivial to just remove the hyperlink from the title of the visual result. As to the rest 2 identical pairs of redundant data, it's not clear to me how to reduce them. I think maybe we could file another bug for this followup investigation.
Attachment #8771863 - Flags: review?(jmaher)
Attachment #8771342 - Flags: review?(jmaher)
Comment on attachment 8771863 [details]
Bug 1287019 - part2: prevent saving redundant screenshots in mozlog's HTML format result.

https://reviewboard.mozilla.org/r/64824/#review61838

part 2 looks great!
Attachment #8771863 - Flags: review?(jmaher) → review+
Comment on attachment 8771342 [details]
Bug 1287019 - part1: make mozlog's HTML format support wptrunner screenshot.

https://reviewboard.mozilla.org/r/64564/#review61840

::: testing/mozbase/mozlog/mozlog/formatters/html/html.py:101
(Diff revision 3)
>      def make_result_html(self, data):
>          tc_time = (data["time"] - self.start_times.pop(data["test"])) / 1000.
>          additional_html = []
>          debug = data.get("extra", {})
> +        # Add support for log exported from wptrunner. The structure of
> +        # reftest_screenshots is listed in wptrunner/executors/base.py.

this assumes wptrunner is the only tool using this format.  I was under the impression reftests (non wpt) used this as well.  Please confirm if this is only used by wptrunner.
Attachment #8771342 - Flags: review?(jmaher) → review-
https://reviewboard.mozilla.org/r/64564/#review61840

> this assumes wptrunner is the only tool using this format.  I was under the impression reftests (non wpt) used this as well.  Please confirm if this is only used by wptrunner.

I searched the whole code base with keyword "reftest_screenshots". The only use is in wptrunner:
http://searchfox.org/mozilla-central/rev/bfcc10319e4e3ce78367fa9bba9316f7eb5248b6/testing/web-platform/harness/wptrunner/executors/base.py#296.

I also verified the assumption by running the following command:

./mach reftest --log-html test.html [PATH_TO_ANY_FAILED_REFTEST]

The test.html remains the same under both cases (w and w/o the patch). If the non-wpt reftest is affected, the Links column would be reduced from {Image2 Image1 Differences Status_Msg Max_Difference} to {Image2 Image1 Differences}. So, I'm pretty sure that reftest_screenshots is a wptrunner-specific log keyword at present.
Flags: needinfo?(jmaher)
thanks for the explanation, let me r+
Flags: needinfo?(jmaher)
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/124addcb422f
part1: make mozlog's HTML format support wptrunner screenshot. r=jmaher
https://hg.mozilla.org/integration/autoland/rev/248eb6469a61
part2: prevent saving redundant screenshots in mozlog's HTML format result. r=jmaher
(In reply to Joel Maher ( :jmaher ) from comment #17)
> thanks for the explanation, let me r+

Thank you for the review. :)

Since this bug is originally reported from servo, which is highly relying on wpt test at present, I'm wondering if there exists a protocol that I could follow to publish this change onto PiPy? So, servo can be benefited by this update as well.
Flags: needinfo?(jmaher)
Status: NEW → ASSIGNED
I am not familiar with a specific process, my understanding is you can take the sources from mozbase in-tree and upload a package using a pypi account.  I am not sure who is the owner of the account- possibly :ahal might know how to update.
Flags: needinfo?(jmaher) → needinfo?(ahalberstadt)
Set leave-open for now. I'll close this once the change is updated on PiPy.
Keywords: leave-open
We cannot release a new version of mozlog until there was a version bump for it. So please file a new bug and make sure it gets bumped up correctly. As an example see bug 1146292.
Flags: needinfo?(ahalberstadt)
Summary: Make mozlog's HTML format support wptrunner screenshots → [mozlog] Make mozlog's HTML format support wptrunner screenshots
(In reply to Henrik Skupin (:whimboo) from comment #23)
> We cannot release a new version of mozlog until there was a version bump for
> it. So please file a new bug and make sure it gets bumped up correctly. As
> an example see bug 1146292.

Filed Bug 1287480. Thank you the pointers. :)
I think we can remove leave-open in that case.
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/124addcb422f
https://hg.mozilla.org/mozilla-central/rev/248eb6469a61
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.