Make Get Page Source spec conformant

RESOLVED FIXED in Firefox 51

Status

Testing
Marionette
RESOLVED FIXED
a year ago
7 months ago

People

(Reporter: ato, Assigned: Vangelis Katsikaros)

Tracking

(Blocks: 1 bug)

Version 3
mozilla53
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

a year ago
The Marionette implementation uses the XMLSerializer on the document object, but should be returning the outerHTML untouched.
(Reporter)

Updated

a year ago
Blocks: 721859
(Reporter)

Comment 1

7 months ago
The relevant spec: https://w3c.github.io/webdriver/webdriver-spec.html#dfn-get-page-source
(Reporter)

Updated

7 months ago
Summary: Make Get Page Source spec compatible → Make Get Page Source spec conformant
(Assignee)

Comment 2

7 months ago
Created attachment 8814983 [details] [diff] [review]
1279203.diff

Make the Get Page Source webdriver command spec conformant.

Freshly built firefox on top of "957458d No bug, Automated blocklist update from host bld-linux64-spot-409 - a=blocklist-update", and successfully passes 

./mach test testing/marionette/harness/marionette/tests/unit/test_pagesource.py
Attachment #8814983 - Flags: review?(ato)
(Reporter)

Updated

7 months ago
Assignee: nobody → vkatsikaros
Status: NEW → ASSIGNED
(Reporter)

Comment 3

7 months ago
Comment on attachment 8814983 [details] [diff] [review]
1279203.diff

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

This looks correct, although you don’t have to assign it to the `source` variable before returning it.

Did you run the test_pagesource.py tests?  Are they passing?

It would also be good if you could add a test there that compares the return value of `self.marionette.execute_script("return document.documentElement.outerHTML")` to that of `self.marionette.page_source`.

For the next patch you upload, please make it a .patch file and not a .diff.  If you use hg, do `hg export -r REV` for this.
Attachment #8814983 - Flags: review?(ato) → review-
(Assignee)

Comment 4

7 months ago
Created attachment 8815011 [details] [diff] [review]
1279203.patch

Thanks for the review!

Yep, the test passes as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=1279203#c2

For the updated patch:
1) I am using git and created a patch with `git format-patch master --stdout > ~/1279203.patch` (I had my work on a separate branch)
2) Taken care of the useless `source` variable assignment.
3) Taken care of the test & execute_script (completely forgot about this, sorry). I rerun `./mach test testing/marionette/harness/marionette/tests/unit/test_pagesource.py` and passes ok (output below).

```
gecko-dev$ git log -1 --oneline 
d5b571e Bug 1279203 - Make Get Page Source spec conformant
gecko-dev$ ./mach test testing/marionette/harness/marionette/tests/unit/test_pagesource.py 
 0:00.00 LOG: MainThread INFO Using workspace for temporary data: "/home/vag/projects/gecko-dev"
 0:00.00 LOG: MainThread mozversion INFO application_buildid: 20161128185016
 0:00.00 LOG: MainThread mozversion INFO application_display_name: Nightly
 0:00.00 LOG: MainThread mozversion INFO application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
 0:00.00 LOG: MainThread mozversion INFO application_name: Firefox
 0:00.00 LOG: MainThread mozversion INFO application_remotingname: firefox
 0:00.00 LOG: MainThread mozversion INFO application_vendor: Mozilla
 0:00.00 LOG: MainThread mozversion INFO application_version: 53.0a1
 0:00.00 LOG: MainThread mozversion INFO platform_buildid: 20161128185016
 0:00.00 LOG: MainThread mozversion INFO platform_version: 53.0a1
 0:00.01 LOG: MainThread INFO Application command: /home/vag/projects/gecko-dev/obj-x86_64-pc-linux-gnu/dist/bin/firefox -no-remote -marionette -profile /tmp/tmpfSHwqn.mozrunner
 0:02.37 LOG: MainThread INFO Profile path is /tmp/tmpfSHwqn.mozrunner
 0:02.37 LOG: MainThread INFO Starting fixture servers
 0:02.42 LOG: MainThread INFO Fixture server listening on http://192.168.1.50:41412/
 0:02.42 LOG: MainThread INFO Fixture server listening on https://192.168.1.50:46170/
 0:02.42 LOG: MainThread INFO e10s is enabled
 0:02.42 LOG: MainThread mozversion INFO application_buildid: 20161128185016
 0:02.42 LOG: MainThread mozversion INFO application_display_name: Nightly
 0:02.42 LOG: MainThread mozversion INFO application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
 0:02.42 LOG: MainThread mozversion INFO application_name: Firefox
 0:02.42 LOG: MainThread mozversion INFO application_remotingname: firefox
 0:02.42 LOG: MainThread mozversion INFO application_vendor: Mozilla
 0:02.42 LOG: MainThread mozversion INFO application_version: 53.0a1
 0:02.42 LOG: MainThread mozversion INFO platform_buildid: 20161128185016
 0:02.42 LOG: MainThread mozversion INFO platform_version: 53.0a1
 0:02.42 SUITE_START: MainThread 1
 0:04.35 TEST_START: MainThread test_pagesource.py TestPageSource.testShouldReturnAXMLDocumentSource
 0:04.69 TEST_END: MainThread PASS
 0:04.69 TEST_START: MainThread test_pagesource.py TestPageSource.testShouldReturnTheSourceOfAPage
 0:04.87 TEST_END: MainThread PASS
 0:04.87 TEST_START: MainThread test_pagesource.py TestPageSource.testShouldReturnTheSourceOfAPageWhenThereAreUnicodeChars
 0:05.20 TEST_END: MainThread PASS
 0:05.20 LOG: MainThread INFO 
SUMMARY
-------
 0:05.20 LOG: MainThread INFO passed: 3
 0:05.20 LOG: MainThread INFO failed: 0
 0:05.20 LOG: MainThread INFO todo: 0
 0:05.20 SUITE_END: MainThread 
Summary
=======

Ran 3 tests
Expected results: 3
Unexpected results: 0

OK
```
Attachment #8814983 - Attachment is obsolete: true
Attachment #8815011 - Flags: review?(ato)
Comment hidden (mozreview-request)
(Reporter)

Updated

7 months ago
Attachment #8815011 - Flags: review?(ato) → review+
(Reporter)

Comment 6

7 months ago
mozreview-review
Comment on attachment 8815676 [details]
Bug 1279203 - Make Get Page Source command spec conformant;

https://reviewboard.mozilla.org/r/96538/#review96734
Attachment #8815676 - Flags: review?(ato) → review+
(Reporter)

Comment 7

7 months ago
I’ve submitted the patch to mozreview and triggered a try run.  Will land once green.

Comment 8

7 months ago
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f707a4378942
Make Get Page Source command spec conformant; r=ato

Comment 9

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f707a4378942
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Comment 10

7 months ago
Sheriffs: Test-only uplift because Marionette is hidden behind flag.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]

Comment 11

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/5805d86370c2
status-firefox52: --- → fixed
Flags: in-testsuite+
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]

Comment 12

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/8adf693e9a0c
status-firefox51: --- → fixed
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.