Closed
Bug 1342162
Opened 7 years ago
Closed 7 years ago
Align page load duration key with spec
Categories
(Remote Protocol :: Marionette, defect)
Tracking
(firefox53 fixed, firefox54 fixed, firefox55 fixed)
RESOLVED
FIXED
mozilla55
People
(Reporter: ato, Assigned: ato)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
The WebDriver specification recently changed to use the key "pageLoad" instead of "page load" in the session timeout configuration object. This makes Marionette out of sync with the specification. Fortunately, the current format we used landed in https://bugzilla.mozilla.org/show_bug.cgi?id=1316622 so if we patch this quickly and uplift the patch to Aurora (53) and Beta (52) we could avoid the backwards compatibility dance on this.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
Comment on attachment 8840527 [details] Bug 1342162 - Align pageLoad key with WebDriver; https://reviewboard.mozilla.org/r/115014/#review116732 ::: testing/marionette/client/marionette_driver/timeout.py:66 (Diff revision 1) > """Get the session's page load timeout. This specifies the time > to wait for the page loading to complete. It is by default 5 > minutes (or 300 seconds). > > """ > - return self._get("page load") > + return self._get("pageLoad") Judging by the try run, some tests need to be updated, too. e.g. https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py#38
Attachment #8840527 -
Flags: review?(mjzffr) → review-
Assignee | ||
Comment 3•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8840527 [details] Bug 1342162 - Align pageLoad key with WebDriver; https://reviewboard.mozilla.org/r/115014/#review116732 > Judging by the try run, some tests need to be updated, too. e.g. https://dxr.mozilla.org/mozilla-central/rev/5069348353f8fc1121e632e3208da33900627214/testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py#38 Thanks, fixed.
Comment hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8840527 [details] Bug 1342162 - Align pageLoad key with WebDriver; https://reviewboard.mozilla.org/r/115014/#review117194
Attachment #8840527 -
Flags: review?(mjzffr) → review+
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/01bc4c9ad3b5 Align pageLoad key with WebDriver; r=maja_zf
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/01bc4c9ad3b5
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 8•7 years ago
|
||
Sheriffs: Please uplift to Aurora and Beta under test-only.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Comment 9•7 years ago
|
||
It's too late for beta given that the merge to release has already happened. You can try esr52 if needed later.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-aurora]
Assignee | ||
Comment 10•7 years ago
|
||
According to http://whattrainisitnow.com/, the next uplift is scheduled to happen 6 March?
Assignee | ||
Comment 11•7 years ago
|
||
In any case, this needs to be uplifted to 52, lest we want to provide backwards compatibility fixes to the Marionette Python client.
Comment 12•7 years ago
|
||
(In reply to Andreas Tolfsen from comment #10) > According to http://whattrainisitnow.com/, the next uplift is scheduled to > happen 6 March? Beta -> Release merge is always done a complete week before all the other branches are getting merged. This is to produce the RC builds and to be ready for a release on Merge Day + 1. The only realistic chance to get it in for 52.0 release is if another RC build has to be build and this patch gets landed before. But I doubt that it is getting accepted. So I will set the landing flag for esr52 instead.
Whiteboard: [checkin-needed-aurora] → [checkin-needed-aurora][checkin-needed-esr52]
Comment 13•7 years ago
|
||
So we have bug 1343520 for update tests now. Given that we cannot land this patch for 52.0 release anymore, I request a backout of this patch given that we definitely need backward compatible code. This will apply to releases for Firefox 52, 53, 54, and 55.
Status: RESOLVED → REOPENED
Flags: needinfo?(ato)
Resolution: FIXED → ---
Whiteboard: [checkin-needed-aurora][checkin-needed-esr52]
Assignee | ||
Comment 14•7 years ago
|
||
It’s unfortunate we were unable to uplift this to beta, which I understand is causing these issues because it now tries to use a Python client which is incompatible with the server to do upgrade tests. This is partly my fault because I didn’t realise beta was closed for uplifts a week before merge day. QA tests upgrade from 52b (last commit before freeze) → 52rcN (where N is latest RC), meaning an uplift of this to latest 52rc2 wouldn’t be of much help, since the client in 52b would be incompatible with the server API in 52rc2. This inevitably means we need to work around the issue by providing some backwards compatibility in the client so that it supports both the new ("pageLoad") and the old ("page load") key. I don’t think this is a big job, but it will probably be rather ugly. Normally unknown keys in JSON Objects are ignored, but the old implementation iterated over entries and threw on an unknown key: https://hg.mozilla.org/mozilla-central/file/FIREFOX_AURORA_52_BASE/testing/marionette/driver.js#l1572
Comment 15•7 years ago
|
||
Sheriffs, can one of you please backout the patch from central until we figured out a good solution for backward compatibility? Thanks.
Flags: needinfo?(sheriffs)
Comment 16•7 years ago
|
||
Backed out. https://hg.mozilla.org/integration/mozilla-inbound/rev/da17b80bf276fc131c18d438233f2bed7ef6f742
Merged backout: https://hg.mozilla.org/mozilla-central/rev/da17b80bf276
Comment 18•7 years ago
|
||
It looks like People are having issues now with Firefox 52.0.1 when using Selenium 3.3.1 and GeckoDriver 0.15.0. Does Selenium or GeckoDriver force `pageLoad` now? We don't have that in Marionette yet. Failure report: --------------- https://www.hskupin.info/2017/01/23/using-selenium-and-webdriver-to-interact-with-insecure-ssl-pages-in-firefox/#comment-69772 Stack looks like: ----------------- File “/Users/dmankellow/git/ssc-product/mgmt/appliances/rbt_ssc/src/test/baxter/env/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py”, line 727, in set_page_load_timeout ‘pageLoad’: int(float(time_to_wait) * 1000)}) File “/Users/dmankellow/git/ssc-product/mgmt/appliances/rbt_ssc/src/test/baxter/env/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py”, line 238, in execute self.error_handler.check_response(response)
Flags: needinfo?(dburns)
Comment 19•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #18) > It looks like People are having issues now with Firefox 52.0.1 when using > Selenium 3.3.1 and GeckoDriver 0.15.0. Does Selenium or GeckoDriver force > `pageLoad` now? We don't have that in Marionette yet. > > Failure report: > --------------- > https://www.hskupin.info/2017/01/23/using-selenium-and-webdriver-to-interact- > with-insecure-ssl-pages-in-firefox/#comment-69772 > > Stack looks like: > ----------------- > File > “/Users/dmankellow/git/ssc-product/mgmt/appliances/rbt_ssc/src/test/baxter/ > env/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py”, > line 727, in set_page_load_timeout > ‘pageLoad’: int(float(time_to_wait) * 1000)}) > File > “/Users/dmankellow/git/ssc-product/mgmt/appliances/rbt_ssc/src/test/baxter/ > env/lib/python2.7/site-packages/selenium/webdriver/remote/webdriver.py”, > line 238, in execute > self.error_handler.check_response(response) This is a selenium bug and the people need to raise a bug with Selenium to get it fixed. It has been fixed already in the python client but not have been released.
Flags: needinfo?(dburns)
Comment 20•7 years ago
|
||
Thanks David.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•7 years ago
|
||
I have now added a second patch to make the changes to the session timeout duration object used in the Python client backwards compatible with earlier Firefoxen, which should address the concerns that caused this to be backed out.
Flags: needinfo?(ato)
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8850581 [details] Bug 1342162 - Make timeout API in client backwards compatible; https://reviewboard.mozilla.org/r/123126/#review125658 Nice solution. ::: testing/marionette/client/marionette_driver/timeout.py:45 (Diff revision 1) > self._marionette._send_message("timeouts", {"type": name, "ms": ms}) > > def _get(self, name): > - ms = self._marionette._send_message("getTimeouts", key=name) > + ts = self._marionette._send_message("getTimeouts") > + if name not in ts: > + raise UnknownKey() Why not throwing the internal KeyError? It would save us from having to define an extra exception class, for a single usage. ::: testing/marionette/client/marionette_driver/timeout.py:73 (Diff revision 1) > """Get the session's page load timeout. This specifies the time > to wait for the page loading to complete. It is by default 5 > minutes (or 300 seconds). > > """ > + # remove fallback when Firefox 55 is stable This only applies if we can get it into 52 release. Otherwise it will be 56. ::: testing/marionette/client/marionette_driver/timeout.py:85 (Diff revision 1) > def page_load(self, sec): > """Set the session's page load timeout. This specifies the time > to wait for the page loading to complete. > > """ > + # remove fallback when Firefox 55 is stable Same as above.
Attachment #8850581 -
Flags: review?(hskupin) → review+
Assignee | ||
Comment 25•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8850581 [details] Bug 1342162 - Make timeout API in client backwards compatible; https://reviewboard.mozilla.org/r/123126/#review125658 > Why not throwing the internal KeyError? It would save us from having to define an extra exception class, for a single usage. Good idea. > This only applies if we can get it into 52 release. Otherwise it will be 56. OK, addressed. > Same as above. And done.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 28•7 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/84948ee2d8ff Align pageLoad key with WebDriver; r=maja_zf https://hg.mozilla.org/integration/autoland/rev/a6380012133b Make timeout API in client backwards compatible; r=whimboo
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/84948ee2d8ff https://hg.mozilla.org/mozilla-central/rev/a6380012133b
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 30•7 years ago
|
||
Sheriffs: Please uplift to Aurora and Beta as test-only.
Whiteboard: [checkin-needed-aurora][checkin-needed-beta]
Comment 31•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/90b35818bd83 https://hg.mozilla.org/releases/mozilla-aurora/rev/fb88f196fe8a
status-firefox54:
--- → fixed
Whiteboard: [checkin-needed-aurora][checkin-needed-beta] → [checkin-needed-beta]
Comment 32•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8016d994c82a https://hg.mozilla.org/releases/mozilla-beta/rev/cd8a7b46639c
status-firefox53:
--- → fixed
Whiteboard: [checkin-needed-beta]
Comment 33•7 years ago
|
||
Did this not make it into 52 ESR?
Assignee | ||
Comment 34•7 years ago
|
||
It did not.
Comment 35•7 years ago
|
||
Andreas, I'm not sure what this exactly is but for the firefox-ui tests on the release candidate builds I see complete bustage due to those changes: https://treeherder.mozilla.org/#/jobs?repo=mozilla-release&revision=d345b657d381ade5195f1521313ac651618f54a2&filter-searchStr=fxfn%20linux&filter-tier=1&filter-tier=2&filter-tier=3 > TEST-UNEXPECTED-ERROR | test_appinfo.py TestAppInfo.test_invalid_properties | InvalidArgumentException: Unrecognised timeout: page load Is that a KeyError vs InvalidArgumentError issue when catching for the fallback? https://dxr.mozilla.org/mozilla-central/rev/20dff607fb88ee69135a280bbb7f32df75a86237/testing/marionette/client/marionette_driver/timeout.py#63 Btw this was never present on beta, and also for commit based tests via Taskcluster it works, but not for the final builds.
Flags: needinfo?(ato)
Comment 36•7 years ago
|
||
Oh wait. I had a look at the mozmill-ci job and have seen that we made use of an older test package which would totally explain this misbehavior. https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-release-linux/1491732920/firefox-52.0.3.en-US.linux-i686.test_packages.json So that should be a problem on our side. Sorry for the noise.
Flags: needinfo?(ato)
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•