[wptrunner] Fix support for --binary-arg in Fx
#13154
Conversation
In order to automate Firefox, wptrunner requires the browser's internal module "Marionette" to be enabled. This is a requirement regardless of any custom configuration specified by the user, including additional command-line flags via the `--binary-arg` argument. Previously, this project would omit the `-marionette` argument in the presence of any user-specified arguments. Because this interferes with normal operation (and because the source code is formatted in a way that conflicts with the order of operations), this is likely an unintentional behavior. Modify the code which derives the command-line arguments to always include the `-marionette` argument.
wpt-pr-bot
added
infra
wpt
wptrunner
Also include `-marionette` in order to work around a known bug in the WPT CLI [1]. [1] web-platform-tests#13154
Include additional arguments to work around a known bug in the WPT CLI: web-platform-tests#13154
tools/wpt/tests/test_wpt.py
Outdated
| @pytest.mark.remote_network | ||
| @pytest.mark.xfail(sys.platform == "win32", | ||
| reason="Tests currently don't work on Windows for path reasons") | ||
| def test_run_firefox_binary_args(manifest_dir): |
There was a problem hiding this comment.
I think this test is pretty heavyweight for what it does. Can we fold it into another test, or just add an assert to the code?
There was a problem hiding this comment.
Sure. I've extended the previous test and inserted a comment to explain the consideration we're making here.
| @@ -251,9 +251,11 @@ def start(self, group_metadata=None, **kwargs): | |||
| if self.ca_certificate_path is not None: | |||
| self.setup_ssl() | |||
|
|
|||
| args = self.binary_args[:] if self.binary_args else [] | |||
| args += [cmd_arg("marionette"), "about:blank"] | |||
There was a problem hiding this comment.
I think we possibly want to check for duplicate arguments. It doesn't make much difference, but is a little cleaner.
If not any(item.strip("-/") == "marionette" for item in args):
args += "--marionette"
Just appending about:blank is probably fine, although we might worry about args not starting with --, - or /, which would be interpreted as URLs and could in some future hypothetically start opening multiple tabs.
There was a problem hiding this comment.
Since we have a utility that's designated for creation of CLI arguments, I'd rather incorporate that:
marionette_flag = cmd_arg("marionette")
if not any(item == marionette_flag for item in args):
args.append(marionette_flag)
...but I don't think this discussion should delay the bug fix. I'm also not sure how to relate the new behavior to the fix in the commit message. What do you think about deferring this for a follow-up patch?
There was a problem hiding this comment.
That doesn't work because multiple variants are supported. The Firefox command line parser is something quite… special.
I'm OK on this not delaying the fix though.
In order to automate Firefox, wptrunner requires the browser's internal
module "Marionette" to be enabled. This is a requirement regardless of
any custom configuration specified by the user, including additional
command-line flags via the
--binary-argargument.Previously, this project would omit the
-marionetteargument in thepresence of any user-specified arguments. Because this interferes with
normal operation (and because the source code is formatted in a way that
conflicts with the order of operations), this is likely an unintentional
behavior.
Modify the code which derives the command-line arguments to always
include the
-marionetteargument.