Closed Bug 1470646 Opened 6 years ago Closed 6 years ago

WebDriver session should return recommended strings for platformName

Categories

(Remote Protocol :: Marionette, enhancement, P1)

enhancement

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

WebDriver recommends returning "windows", "mac", and "linux" for
the platformName capability.  Marionette passes through whatever
we get from XPCOM, which differs slightly.

We should update Marionette to return the recommended values when
creating a new WebDriver session so that we will have conformance
with the standard.  It will also correct some test expectations in
WPT that rely on platformName.
Assignee: nobody → ato
Blocks: webdriver
Status: NEW → ASSIGNED
Priority: -- → P1
Note there’s still a couple of test failures as a result of this
change which are harder than a simple s/darwin/mac/g.  I’ll look
at these soon as it’s just about updating tests.  But fundamentally
the patch should be ready for review.
Comment on attachment 8987267 [details]
Bug 1470646 - Silence missing nodeType property warning.

https://reviewboard.mozilla.org/r/252526/#review259136

::: testing/marionette/format.js:41
(Diff revision 2)
>  function pprint(ss, ...values) {
>    function pretty(val) {
>      let proto = Object.prototype.toString.call(val);
>  
> -    if (val && val.nodeType === 1) {
> +    if (typeof val == "object" && val !== null &&
> +        "nodeType" in val && val.nodeType === 1) {

Given that we don't allow `null` nor `undefined` why just not `(val != null && "nodetype" in val)`?
Attachment #8987267 - Flags: review?(hskupin) → review+
Comment on attachment 8987266 [details]
Bug 1470646 - Rename session module to capabilities.

https://reviewboard.mozilla.org/r/252524/#review259138

::: commit-message-368ae:5
(Diff revision 2)
> +Bug 1470646 - Rename session module to capabilities. r?whimboo
> +
> +Renames testing/marionette/session.js
> +to testing/marionette/capabilities.js and modularises the types
> +it exposes.  No functional changes apart from these.

Thanks! It was always something which disturbed me.

::: testing/marionette/capabilities.js:4
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

I assume you did a `hg move` (or git equivalent) for this file name change. Mozreview doesn't show it to me as such.

::: testing/marionette/test/unit/test_capabilities.js:4
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Same regarding `hg mv` for this file.
Attachment #8987266 - Flags: review?(hskupin) → review+
Comment on attachment 8987347 [details]
Bug 1470646 - Update Mn and Fxfn tests to use WebDriver conforming platformName.

https://reviewboard.mozilla.org/r/252588/#review259142

Removing request until the tests have been fixed.
Attachment #8987347 - Flags: review?(hskupin)
Comment on attachment 8987268 [details]
Bug 1470646 - Return platformName as recommended by WebDriver.

https://reviewboard.mozilla.org/r/252528/#review259140

::: testing/marionette/capabilities.js:512
(Diff revision 2)
> +      return "windows";
> +
> +    case "Darwin":
> +      return "mac";
> +
> +    case "Android":

`android` is not used in the WebDriver spec. And I don't know what should be reported. So I will have to take it that this is fine.

::: testing/marionette/capabilities.js:513
(Diff revision 2)
> +
> +    case "Darwin":
> +      return "mac";
> +
> +    case "Android":
> +    case "Linux":

I would only specify those platform names in this switch block which clearly need to be renamed. `Android` and `Linux` can can directly be handled by the `default` case.

::: testing/web-platform/meta/MANIFEST.json:523112
(Diff revision 2)
>    "css/css-scoping/shadow-host-with-before-after.html": [
>     "99af6e29e69b3131b59dbdc2b0eead52931123c2",
>     "reftest"
>    ],
>    "css/css-scoping/shadow-link-rel-stylesheet-no-style-leak.html": [
> -   "76a54dabd8bd09f7155ab0331e3d17d1a0cae243",
> +   "a46be006762a16c2deb3d1d3a760e3c4e348668c",

The update of the Manifest.json doens't belong to this changeset.
Attachment #8987268 - Flags: review?(hskupin) → review+
Comment on attachment 8987348 [details]
Bug 1470646 - Update Fxfn tests to use WebDriver conforming platformName.

https://reviewboard.mozilla.org/r/252590/#review259146

::: testing/firefox-ui/tests/functional/sessionstore/session_store_test_case.py:156
(Diff revision 2)
>  
>          :raises: Exception: if not supported on the current platform
>          :raises: WindowsError: if a Windows API call failed
>          """
>  
> -        if self.marionette.session_capabilities['platformName'] != 'windows_nt':
> +        if self.marionette.session_capabilities['platformName'] != 'window':

This doesn't seem to work or you fixed it after your try push?
Attachment #8987348 - Flags: review?(hskupin)
Comment on attachment 8987349 [details]
Bug 1470646 - Update Wd tests to use WebDriver conforming platformName.

https://reviewboard.mozilla.org/r/252592/#review259148
Attachment #8987349 - Flags: review?(hskupin) → review+
Comment on attachment 8987349 [details]
Bug 1470646 - Update Wd tests to use WebDriver conforming platformName.

https://reviewboard.mozilla.org/r/252592/#review259158

You didn't update the new session tests, specifically `merge.py`, which would need an update to the expected status in the ini file too. Why didn't those actually fail?
Attachment #8987349 - Flags: review+ → review-
Comment on attachment 8987267 [details]
Bug 1470646 - Silence missing nodeType property warning.

https://reviewboard.mozilla.org/r/252526/#review259136

> Given that we don't allow `null` nor `undefined` why just not `(val != null && "nodetype" in val)`?

The "nodeType" in val check will fail with TypeErrors for non-object
values such as boolean and number, so the typeof val == "object"
check is still necessary.

typeof(undefined) returns "undefined", which means the second val
!== null check is explicit for null because undefined is not a
possibility.
Comment on attachment 8987266 [details]
Bug 1470646 - Rename session module to capabilities.

https://reviewboard.mozilla.org/r/252524/#review259138

> I assume you did a `hg move` (or git equivalent) for this file name change. Mozreview doesn't show it to me as such.

We can’t make this rename atomic unless we also modify the file to
update the symbols.  Otherwise tests would fail when bisecting.
Comment on attachment 8987268 [details]
Bug 1470646 - Return platformName as recommended by WebDriver.

https://reviewboard.mozilla.org/r/252528/#review259140

> `android` is not used in the WebDriver spec. And I don't know what should be reported. So I will have to take it that this is fine.

I talk about this in the commit message.  The WebDriver standard
only caters for desktop browsers, but is very careful in its wording
around which platform names are recommended so that Linux and Android
(which is based on Linux) can be differentiated in the future.
Comment on attachment 8987347 [details]
Bug 1470646 - Update Mn and Fxfn tests to use WebDriver conforming platformName.

https://reviewboard.mozilla.org/r/252588/#review259142

Sorry, why is the review blocked on some tests failing?  I mentioned
this in https://bugzilla.mozilla.org/show_bug.cgi?id=1470646#c13.
Comment on attachment 8987266 [details]
Bug 1470646 - Rename session module to capabilities.

https://reviewboard.mozilla.org/r/252524/#review259138

> We can’t make this rename atomic unless we also modify the file to
> update the symbols.  Otherwise tests would fail when bisecting.

Which kind of symbols? I would expect that when having this commit applied, all will work fine even with `hg mv`, which would be preferred for better history tracking.
Comment on attachment 8987266 [details]
Bug 1470646 - Rename session module to capabilities.

https://reviewboard.mozilla.org/r/252524/#review259138

> Which kind of symbols? I would expect that when having this commit applied, all will work fine even with `hg mv`, which would be preferred for better history tracking.

So the fundamental problem here is that hg doesn’t support “move
and also change code in the file” in a single operation, which is
why you’re not seeing these are moved files.  In my local branch
they appear as moved but when they are exported to mozreview they
get converted to hg.

The patch also removes the "session." type prefix which I suppose
on second thought it is not necessary to do in the same patch, but
do you feel this is so important?
Comment on attachment 8987266 [details]
Bug 1470646 - Rename session module to capabilities.

https://reviewboard.mozilla.org/r/252524/#review259138

> So the fundamental problem here is that hg doesn’t support “move
> and also change code in the file” in a single operation, which is
> why you’re not seeing these are moved files.  In my local branch
> they appear as moved but when they are exported to mozreview they
> get converted to hg.
> 
> The patch also removes the "session." type prefix which I suppose
> on second thought it is not necessary to do in the same patch, but
> do you feel this is so important?

Ah ok. So I think we had this problem already in the past with mozreview. When you landed such a patch everything was actually fine. It might be the same situation here. In that case we can drop both issues.
Comment on attachment 8987347 [details]
Bug 1470646 - Update Mn and Fxfn tests to use WebDriver conforming platformName.

https://reviewboard.mozilla.org/r/252588/#review259398

::: commit-message-dfa30:1
(Diff revision 3)
> +Bug 1470646 - Update Mn tests to use WebDriver conforming platformName. r?whimboo

Mind adding Firefox Puppeteer here too?
Attachment #8987347 - Flags: review?(hskupin) → review+
Comment on attachment 8987348 [details]
Bug 1470646 - Update Fxfn tests to use WebDriver conforming platformName.

https://reviewboard.mozilla.org/r/252590/#review259402
Attachment #8987348 - Flags: review?(hskupin) → review+
Comment on attachment 8987349 [details]
Bug 1470646 - Update Wd tests to use WebDriver conforming platformName.

https://reviewboard.mozilla.org/r/252592/#review259406

::: testing/web-platform/meta/MANIFEST.json:523112
(Diff revision 3)
>    "css/css-scoping/shadow-host-with-before-after.html": [
>     "99af6e29e69b3131b59dbdc2b0eead52931123c2",
>     "reftest"
>    ],
>    "css/css-scoping/shadow-link-rel-stylesheet-no-style-leak.html": [
> -   "76a54dabd8bd09f7155ab0331e3d17d1a0cae243",
> +   "a46be006762a16c2deb3d1d3a760e3c4e348668c",

Not sure what our strategy is for those updates which are not related to the files you are changing. I usually don't include them in my patch, because if other bugs are getting backed out, this could cause conflicts.

::: testing/web-platform/tests/webdriver/tests/actions/support/keys.py:743
(Diff revision 3)
>          "shift": False,
>          "value": u"\ue040",
>      }
>  }
>  
> -if sys.platform == 'darwin':
> +if sys.platform == "mac":

Interesting that no-one from the other browser vendors complained about all those invalid platform names yet!
Attachment #8987349 - Flags: review?(hskupin) → review+
I’m holding off landing this until
https://bugzilla.mozilla.org/show_bug.cgi?id=1470533 lands on
central.
Depends on: 1470533
Comment on attachment 8987858 [details]
Bug 1470646 - Increase wdspec long timeout to three minutes.

https://reviewboard.mozilla.org/r/253134/#review259934

::: testing/web-platform/tests/tools/wptrunner/wptrunner/wpttest.py:410
(Diff revision 1)
>      result_cls = WdspecResult
>      subtest_result_cls = WdspecSubtestResult
>      test_type = "wdspec"
>  
>      default_timeout = 25
> -    long_timeout = 120
> +    long_timeout = 180  # 3 minutes

We should remember to revert that once our startup times got better, or we have a better new session handling.

I still wish we have timeouts for individual tests and not the full test file. Maybe some time in the future...

Btw. this should fix a lot of our timeout failures and maybe even those for Quantum Render. So we might also be able to re-enable most of them. Can you please file a follow-up bug for that? Thanks.
Attachment #8987858 - Flags: review?(hskupin) → review+
Comment on attachment 8987871 [details]
Bug 1470646 - Modularise capabilities module.

https://reviewboard.mozilla.org/r/253148/#review259936

Hurray for shorter lines!
Attachment #8987871 - Flags: review?(hskupin) → review+
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/42964d651f02
Silence missing nodeType property warning. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/13e47fa99a31
Increase wdspec long timeout to three minutes. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/e5415b1e22f5
Rename session module to capabilities. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/22469044267d
Modularise capabilities module. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/e543f592454a
Return platformName as recommended by WebDriver. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/2b19429d778f
Update Mn and Fxfn tests to use WebDriver conforming platformName. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/58e2010d2842
Update Fxfn tests to use WebDriver conforming platformName. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/9b8d9f803b25
Update Wd tests to use WebDriver conforming platformName. r=whimboo
Backed out 8 changesets (bug 1470646) for linting failure 

Backout: https://hg.mozilla.org/integration/autoland/rev/a33b4bcf0a74497279cae6a3dbc61fa271051b28

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9b8d9f803b2596ec3a2021c2614f64a5e0e2df55

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185967162&repo=autoland&lineNumber=378

task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imply_option_impl:8: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imply_option_impl:11: WARNING: Definition list ends without a blank line; unexpected unindent.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imply_option_impl:15: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imply_option_impl:17: WARNING: Block quote ends without a blank line; unexpected unindent.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imply_option_impl:22: WARNING: Definition list ends without a blank line; unexpected unindent.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imply_option_impl:26: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imply_option_impl:28: WARNING: Block quote ends without a blank line; unexpected unindent.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imply_option_impl:31: WARNING: Definition list ends without a blank line; unexpected unindent.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozbuild/configure/__init__.py:docstring of mozbuild.configure.ConfigureSandbox.imports_impl:5: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozpack/chrome/flags.py:docstring of mozpack.chrome.flags.Flags.match:4: WARNING: Definition list ends without a blank line; unexpected unindent.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozpack/chrome/flags.py:docstring of mozpack.chrome.flags.StringFlag.matches:4: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.431Z] /builds/worker/checkouts/gecko/python/mozbuild/mozpack/chrome/flags.py:docstring of mozpack.chrome.flags.VersionFlag.matches:4: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.432Z] /builds/worker/checkouts/gecko/python/mozbuild/mozpack/chrome/manifest.py:docstring of mozpack.chrome.manifest.ManifestEntry:3: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.432Z] WARNING: missing attribute processOutputLine in object mozprocess.ProcessHandlerMixin
[task 2018-07-02T14:40:13.432Z] WARNING: missing attribute onTimeout in object mozprocess.ProcessHandlerMixin
[task 2018-07-02T14:40:13.432Z] WARNING: missing attribute onFinish in object mozprocess.ProcessHandlerMixin
[task 2018-07-02T14:40:13.432Z] /builds/worker/checkouts/gecko/docs-out/html/_staging/intl/l10n/docs/l10n/fluent_migrations.rst:565: WARNING: Title underline too short.
[task 2018-07-02T14:40:13.432Z] 
[task 2018-07-02T14:40:13.432Z] 3. Add new FTL strings to the local en-US repository
[task 2018-07-02T14:40:13.432Z] ------------------------------------------------
[task 2018-07-02T14:40:13.432Z] /builds/worker/checkouts/gecko/docs-out/html/_staging/intl/l10n/docs/l10n/fluent_migrations.rst:565: WARNING: Title underline too short.
[task 2018-07-02T14:40:13.432Z] 
[task 2018-07-02T14:40:13.432Z] 3. Add new FTL strings to the local en-US repository
[task 2018-07-02T14:40:13.432Z] ------------------------------------------------
[task 2018-07-02T14:40:13.432Z] WARNING: autodoc: failed to import class u'NetworkError' from module u'moznetwork'; the following exception was raised:
[task 2018-07-02T14:40:13.432Z] Traceback (most recent call last):
[task 2018-07-02T14:40:13.432Z]   File "/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/docs-y-qLfVwz/lib/python2.7/site-packages/sphinx/ext/autodoc/importer.py", line 176, in import_object
[task 2018-07-02T14:40:13.432Z]     obj = attrgetter(obj, attrname)
[task 2018-07-02T14:40:13.432Z]   File "/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/docs-y-qLfVwz/lib/python2.7/site-packages/sphinx/ext/autodoc/__init__.py", line 274, in get_attr
[task 2018-07-02T14:40:13.432Z]     return autodoc_attrgetter(self.env.app, obj, name, *defargs)
[task 2018-07-02T14:40:13.432Z]   File "/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/docs-y-qLfVwz/lib/python2.7/site-packages/sphinx/ext/autodoc/__init__.py", line 1517, in autodoc_attrgetter
[task 2018-07-02T14:40:13.432Z]     return safe_getattr(obj, name, *defargs)
[task 2018-07-02T14:40:13.432Z]   File "/builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/docs-y-qLfVwz/lib/python2.7/site-packages/sphinx/util/inspect.py", line 220, in safe_getattr
[task 2018-07-02T14:40:13.433Z]     raise AttributeError(name)
[task 2018-07-02T14:40:13.433Z] AttributeError: NetworkError
[task 2018-07-02T14:40:13.433Z] 
[task 2018-07-02T14:40:13.433Z] /builds/worker/checkouts/gecko/python/mozbuild/mozpack/errors.py:docstring of mozpack.errors.ErrorCollector:6: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.433Z] /builds/worker/checkouts/gecko/python/mozbuild/mozpack/errors.py:docstring of mozpack.errors.ErrorCollector:23: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.433Z] /builds/worker/checkouts/gecko/python/mozbuild/mozpack/errors.py:docstring of mozpack.errors.ErrorCollector:30: WARNING: Unexpected indentation.
[task 2018-07-02T14:40:13.433Z] /builds/worker/checkouts/gecko/python/mozbuild/mozpack/errors.py:docstring of mozpack.errors.ErrorCollector:41: WARNING: Unexpected indentation.
Flags: needinfo?(ato)
The problem is that it can’t find the right session.* types in
testing/marionette/doc/internals/session.rst.
Flags: needinfo?(ato)
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/608e35dbe664
Silence missing nodeType property warning. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/da9689d7801b
Increase wdspec long timeout to three minutes. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/4556c3569ccd
Rename session module to capabilities. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/ba1bd1e1c47f
Modularise capabilities module. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/59e8fd258b33
Return platformName as recommended by WebDriver. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/6ca4afd1f862
Update Mn and Fxfn tests to use WebDriver conforming platformName. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/bcf9a8c4d677
Update Fxfn tests to use WebDriver conforming platformName. r=whimboo
https://hg.mozilla.org/integration/autoland/rev/16ba5f9ae84e
Update Wd tests to use WebDriver conforming platformName. r=whimboo
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12005 for changes under testing/web-platform/tests
Can't merge web-platform-tests PR due to failing upstream checks:
Github PR https://github.com/web-platform-tests/wpt/pull/12005
* continuous-integration/appveyor/pr
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: