Closed Bug 1326534 Opened 8 years ago Closed 8 years ago

Capabilities parsing that conform to WebDriver

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(13 files)

Bug 1326534 - Correct tests for mandated and supported capabilities;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Exclude array and null from being counted as objects;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Preserve stacks when passing error protos to WebDriverError;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Add test for assert.array;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Add assert.in for own properties;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Lint a few statements in driver.js;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Propagate stacktraces for WebDriver errors;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Correct capability wrapping in tests;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Rewrite capabilities parsing in Marionette;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette;
59 bytes, text/x-review-board-request
automatedtester
: review+
whimboo
: review+
impossibus
: review+
Details
Bug 1326534 - Reset session capabilities state on teardown;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1326534 - Use session.Capabilities representation in listener;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
Bug 1326534 - Rename sessionCapabilities variable for brevity;
59 bytes, text/x-review-board-request
whimboo
: review+
Details
This is a ‘super-bug’ to track the fixing of bug 1282873 and bug 1323450. Because they are related and many of the changes needed in both cases are similar, I have decided to address them both at the same time. For the sake of clarity, I will use this bug to do that. The way Marionette currently processes capabilities is not conforming to how they are specified in WebDriver. This change will move us closer to having a conforming implementation, but will not introduce the new firstMatch/alwaysMatch primitives that were recently added. This is considered to be too big of a breaking change, but will be addressed in a follow-up change to be announced. It is in other words not expected that the changes associated here will be backwards incompatible with existing clients. I did however discover whilst working on this that Marionette currently accepts a flat top-level {capabilities: {foo: "bar"}} object, which is not conforming to either Selenium or WebDriver. I have rectified this.
Blocks: 1282873, 1323450
Assignee: nobody → ato
Blocks: webdriver
Status: NEW → ASSIGNED
Comment on attachment 8822861 [details] Bug 1326534 - Correct tests for mandated and supported capabilities; https://reviewboard.mozilla.org/r/101618/#review102200
Attachment #8822861 - Flags: review?(dburns) → review+
Comment on attachment 8822862 [details] Bug 1326534 - Exclude array and null from being counted as objects; https://reviewboard.mozilla.org/r/101620/#review102204
Attachment #8822862 - Flags: review?(dburns) → review+
Comment on attachment 8822863 [details] Bug 1326534 - Preserve stacks when passing error protos to WebDriverError; https://reviewboard.mozilla.org/r/101622/#review102208
Attachment #8822863 - Flags: review?(dburns) → review+
Comment on attachment 8822864 [details] Bug 1326534 - Add test for assert.array; https://reviewboard.mozilla.org/r/101624/#review102210
Attachment #8822864 - Flags: review?(dburns) → review+
Comment on attachment 8822865 [details] Bug 1326534 - Add assert.in for own properties; https://reviewboard.mozilla.org/r/101626/#review102212
Attachment #8822865 - Flags: review?(dburns) → review+
Comment on attachment 8822866 [details] Bug 1326534 - Lint a few statements in driver.js; https://reviewboard.mozilla.org/r/101628/#review102216
Attachment #8822866 - Flags: review?(dburns) → review+
Comment on attachment 8822867 [details] Bug 1326534 - Propagate stacktraces for WebDriver errors; https://reviewboard.mozilla.org/r/101630/#review102218
Attachment #8822867 - Flags: review?(dburns) → review+
Comment on attachment 8822868 [details] Bug 1326534 - Correct capability wrapping in tests; https://reviewboard.mozilla.org/r/101632/#review102226
Attachment #8822868 - Flags: review?(dburns) → review+
Comment on attachment 8822873 [details] Bug 1326534 - Rename sessionCapabilities variable for brevity; https://reviewboard.mozilla.org/r/101642/#review102248
Attachment #8822873 - Flags: review?(hskupin) → review+
Comment on attachment 8822872 [details] Bug 1326534 - Use session.Capabilities representation in listener; https://reviewboard.mozilla.org/r/101640/#review102294 ::: testing/marionette/listener.js:62 (Diff revision 8) > element.Strategy.PartialLinkText, > element.Strategy.TagName, > element.Strategy.XPath, > ]); > > -var capabilities = {}; > +var capabilities; So we declare it with a default value of undefined now. Can we ensure that no other code tries to access it via .get() before the variable correctly gets initialized?
Comment on attachment 8822871 [details] Bug 1326534 - Reset session capabilities state on teardown; https://reviewboard.mozilla.org/r/101638/#review102296
Attachment #8822871 - Flags: review?(hskupin) → review+
Comment on attachment 8822872 [details] Bug 1326534 - Use session.Capabilities representation in listener; https://reviewboard.mozilla.org/r/101640/#review102294 > So we declare it with a default value of undefined now. Can we ensure that no other code tries to access it via .get() before the variable correctly gets initialized? We can’t prove that, but generally you can’t do that in JavaScript. Calling `capabilities.get(…)` on it before would have returned undefined instead of throwing an error, so you could argue that this is _safer_ because it alerts you about a problem earlier than before.
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review102298 ::: testing/marionette/driver.js:448 (Diff revision 8) > > this.wins.set(reg.id, listenerWindow); > if (nullPrevious && (this.curBrowser.curFrameId !== null)) { > - this.sendAsync("newSession", this.sessionCapabilities, this.newSessionCommandId); > + this.sendAsync( > + "newSession", > + this.sessionCapabilities.toJSON(), Maybe I miss something but why do we need toJSON() now? ::: testing/marionette/driver.js:500 (Diff revision 8) > + get: function () { return this.sessionCapabilities.get("timeouts"); }, > + > + set: function (newTimeouts) { > + if (!(newTimeouts instanceof session.Timeouts)) { > + throw new TypeError(); > + } Shouldn't this check be contained in the timeout setter of Capabilities? ::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:85 (Diff revision 8) > > + def test_capability_types(self): > + for value in ["", "invalid", True, 42, []]: > + print("testing value {}".format(value)) > + with self.assertRaises(SessionNotCreatedException): > + print(" with desiredCapabilities") Those print statements are not helpful as long as we do not also output the value we test. As of now we would have a list of: with desiredCapabilities with requiredCapabilities with desiredCapabilities with requiredCapabilities with desiredCapabilities with requiredCapabilities Also can we use a logger instead? ::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:214 (Diff revision 8) > + self.marionette.delete_session() > + self.marionette.start_session({"requiredCapabilities": {"proxy": {"proxyType": "manual"}}}) > + self.assertIn("proxy", self.marionette.session_capabilities) > + self.assertEqual(self.marionette.session_capabilities["proxy"]["proxyType"], "manual") > + self.assertEqual(self.marionette.get_pref("network.proxy.type"), 1) > + I know we need those tests but could you check how much additional time it requires? Starting a new session takes kinda long in debug and ASAN builds, so I wonder if we somehow can reduce the amount of application restarts.
Attachment #8822870 - Flags: review?(hskupin)
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review102298 > Maybe I miss something but why do we need toJSON() now? Because `this.sessionCapabilities` is a complex object (`session.Capabilities`) and `sendAsync` doesn’t implicitly call `toJSON` or `JSON.stringify` (which would call `toJSON`).
Comment on attachment 8822869 [details] Bug 1326534 - Rewrite capabilities parsing in Marionette; https://reviewboard.mozilla.org/r/101634/#review102324
Attachment #8822869 - Flags: review?(dburns) → review+
Comment on attachment 8822872 [details] Bug 1326534 - Use session.Capabilities representation in listener; https://reviewboard.mozilla.org/r/101640/#review102294 > We can’t prove that, but generally you can’t do that in JavaScript. Calling `capabilities.get(…)` on it before would have returned undefined instead of throwing an error, so you could argue that this is _safer_ because it alerts you about a problem earlier than before. Going to resolve this as (1) I don’t like to the mix object literal type with `session.Capabilities`, and (2) it could argued to be safer.
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review102298 > Those print statements are not helpful as long as we do not also output the value we test. As of now we would have a list of: > > with desiredCapabilities > with requiredCapabilities > with desiredCapabilities > with requiredCapabilities > with desiredCapabilities > with requiredCapabilities > > Also can we use a logger instead? The value is being printed two lines earlier. This is only for debugging purposes if the test should fail, so a print is fine. > I know we need those tests but could you check how much additional time it requires? Starting a new session takes kinda long in debug and ASAN builds, so I wonder if we somehow can reduce the amount of application restarts. As I said in another review, performance optimisations is not a priority at this point. Correctness and conformity to the specification, however, is.
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review102690 ::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:83 (Diff revision 9) > # same state it was before it started the test > self.marionette.start_session() > > + def test_capability_types(self): > + for value in ["", "invalid", True, 42, []]: > + print("testing value {}".format(value)) debug statements?
Comment on attachment 8822872 [details] Bug 1326534 - Use session.Capabilities representation in listener; https://reviewboard.mozilla.org/r/101640/#review102862
Attachment #8822872 - Flags: review?(dburns) → review+
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review102690 > debug statements? I think it is used here to show which value we actually check. Way better here would be to use the @parametrized decorator from marionette_test.
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review102690 > I think it is used here to show which value we actually check. Way better here would be to use the @parametrized decorator from marionette_test. Yeah, just to give some indication how far into the test we are.
Attachment #8822872 - Flags: review?(hskupin) → review+
Comment on attachment 8822872 [details] Bug 1326534 - Use session.Capabilities representation in listener; whimboo: Not sure what happened to the r+ on this one, and I can’t seem to be able to reset it to r+.
Attachment #8822872 - Flags: review+ → review?(hskupin)
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review103198 ::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:131 (Diff revision 10) > + self.marionette.delete_session() > + self.marionette.start_session({"requiredCapabilities": {"browserName": firefox}}) > + self.assertEqual(self.marionette.session_capabilities["browserName"], firefox) > + > + # TODO(ato): version comparison not implemented yet > + def test_browser_version(self): Could you comment out this empty test so that it doesn't appear as passing in the Mn job logs? ::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:134 (Diff revision 10) > + > + # TODO(ato): version comparison not implemented yet > + def test_browser_version(self): > + pass > + > + def test_platform_name(self): I think `test_browser_name` and `test_platform_name` should be broken down into smaller test methods and be moved to a separate test class so you can add `delete_session` to setUp or tearDown and share test data like `allowed`, capability types, etc. I wish we could parameterize as easily as in pytest =/
Attachment #8822870 - Flags: review?(mjzffr)
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review103198 > Could you comment out this empty test so that it doesn't appear as passing in the Mn job logs? Sure. > I think `test_browser_name` and `test_platform_name` should be broken down into smaller test methods and be moved to a separate test class so you can add `delete_session` to setUp or tearDown and share test data like `allowed`, capability types, etc. > > I wish we could parameterize as easily as in pytest =/ This would increase the test runtime, since whimboo claims session creation is expensive. But if it gives better granularity in the tests, I’m all for it.
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review103198 > This would increase the test runtime, since whimboo claims session creation is expensive. But if it gives better granularity in the tests, I’m all for it. Actually, I have to confess that I was wrong. Deleting a session doesn't cause a shutdown of the application. So creating a new session is quite fast. Sorry.
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review103396
Attachment #8822870 - Flags: review?(dburns) → review+
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review103436 ::: testing/marionette/driver.js:495 (Diff revision 11) > this.mm.addMessageListener(li, cb); > }); > }; > > +Object.defineProperty(GeckoDriver.prototype, "timeouts", { > + get: function () { return this.sessionCapabilities.get("timeouts"); }, I would prefer the same style of definition by moving the method body into the next line. IMO it reads better. Same for proxy below. ::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:82 (Diff revision 11) > # Start a new session just to make sure we leave the browser in the > # same state it was before it started the test > self.marionette.start_session() > > + def test_capability_types(self): > + for value in ["", "invalid", True, 42, []]: So @parametrized does not work? I haven't seen a response to this proposal yet. May can still defer this to later.
Attachment #8822870 - Flags: review?(hskupin) → review+
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review103436 > So @parametrized does not work? I haven't seen a response to this proposal yet. May can still defer this to later. As a matter of personal preference, I like to write the tests out because I find it easier to debug when a test goes wrong. But yes, I’d like to defer this since the test as it is written (and rewritten once!) works fine.
Comment on attachment 8822870 [details] Bug 1326534 - Deploy WebDriver conforming capabilities in Marionette; https://reviewboard.mozilla.org/r/101636/#review103500 r+wc ::: testing/marionette/harness/marionette_harness/tests/unit/test_capabilities.py:242 (Diff revisions 10 - 11) > - "Session ID has {{}} in it: {}".format( > - self.marionette.session_id)) > + self.assertIn("proxy", self.marionette.session_capabilities) > + self.assertEqual(self.marionette.session_capabilities["proxy"]["proxyType"], "manual") > + self.assertEqual(self.marionette.get_pref("network.proxy.type"), 1) > > def test_timeouts(self): > self.marionette.delete_session() This line can be removed now because it's in setUp
Attachment #8822870 - Flags: review?(mjzffr) → review+
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8287c2bfbfb8 Correct tests for mandated and supported capabilities; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/ee05e899b923 Exclude array and null from being counted as objects; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/8a63bca6b7ff Preserve stacks when passing error protos to WebDriverError; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/0ea5e8c0beef Add test for assert.array; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/3c65e5b16928 Add assert.in for own properties; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/29d37d4274e7 Lint a few statements in driver.js; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/cc4431a2998a Propagate stacktraces for WebDriver errors; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/602a6650ba46 Correct capability wrapping in tests; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/252175a9c2a6 Rewrite capabilities parsing in Marionette; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/638234f7abd9 Deploy WebDriver conforming capabilities in Marionette; r=automatedtester,maja_zf,whimboo https://hg.mozilla.org/integration/autoland/rev/e28fc7e5b984 Reset session capabilities state on teardown; r=whimboo https://hg.mozilla.org/integration/autoland/rev/d960a2d9e208 Use session.Capabilities representation in listener; r=automatedtester https://hg.mozilla.org/integration/autoland/rev/73f5e38d40a2 Rename sessionCapabilities variable for brevity; r=whimboo
Sheriffs: Please uplift to Aurora as test-only.
Whiteboard: [checkin-needed-aurora]
Comment on attachment 8822872 [details] Bug 1326534 - Use session.Capabilities representation in listener; https://reviewboard.mozilla.org/r/101640/#review104490 Interesting that you were able to get this patch landed even with this missing r+.
Attachment #8822872 - Flags: review?(hskupin) → review+
Comment on attachment 8822872 [details] Bug 1326534 - Use session.Capabilities representation in listener; https://reviewboard.mozilla.org/r/101640/#review104490 It was r+’ed, but the mirroring somehow got messed up between Bugzilla and Review Board.
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: