James Graham [:jgraham] Reporter
	Description • 7 years ago

The fundamental reason for bug 1368262 is that we aren't passing harness-set profile data into wdspec tests and so we end up using the default prefs that vary between beta, dev edition, and nightly. Therefore the results are inconsistent.

	Andreas Tolfsen ❲:ato❳
	Comment 5 • 7 years ago
mozreview-review

Comment on attachment 8876182 [details]
Bug 1371709 - Fix typo in module name,

https://reviewboard.mozilla.org/r/147608/#review151912

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8876183 [details]
Bug 1371709 - Reenable wpt navigation test,

https://reviewboard.mozilla.org/r/147610/#review151914

::: commit-message-14c6d:1
(Diff revision 1)
> +Bug 1371709 - Reenable wpt navigation test, r=ato"

s/wpt/WebDriver/

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8876183 [details]
Bug 1371709 - Reenable wpt navigation test,

https://reviewboard.mozilla.org/r/147610/#review151918

::: commit-message-14c6d:1
(Diff revision 1)
> +Bug 1371709 - Reenable wpt navigation test, r=ato"

s/"//

	Andreas Tolfsen ❲:ato❳
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8876180 [details]
Bug 1371709 - Disallow capabilities with duplicate profiles,

https://reviewboard.mozilla.org/r/147604/#review152366

After thinking more carefully about this, I think it would be good if
geckodriver would handle the presence of a -profile argument to the
moz:firefoxOptions’ args field.

If a user passes

	{"moz:firefoxOptions": {"args": ["-profile", "/path/to/profile"]}}

we append the args to the list of required arguments needed to bootstrap
Marionette, which by default includes a new scratch profile.  Firefox
parses arguments in sequence and will use the first profile that is
provided.

In the above example, the the full command line will be:

	firefox-bin -marionette -profile /tmp/tmp.mTFFXz1ygE -profile /path/to/profile

I think users will find this behaviour surprising because they would
expect the specified profile to be used; not for Firefox to silently
pick another, seemingly random, profile.

Similarly, introducing a profileDir capability will make this even
harder to understand: it’s not clear what would take presedence over
the profile or profileDir capabilities, and it doesn’t make the
situation better if the user passes a -profile flag in args.

This is a chance to fix both issues, and to document how to make
geckodriver pick up a local profile from the system. chromedriver uses a
similar strategy, which is highlighted in their documentation:

	https://sites.google.com/a/chromium.org/chromedriver/capabilities

If we need to choose between the profile capability and -profile
from args, it creates an interdependency between the two.  I would
suggest adding a new test that’s not part of the capabilities
parsing, but which occurs after the capabilities have been successfully
inspected.  This would parse args as an argument list and check if a
-profile/--profile argument with a value is present and use that if no
profile capability has been provided.  If both have been provided, we
should maybe error?

	Andreas Tolfsen ❲:ato❳
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8876181 [details]
Bug 1371709 - Support passing profile to Firefox wdspec tests,

https://reviewboard.mozilla.org/r/147606/#review152368

See comments on last patch.

	Andreas Tolfsen ❲:ato❳
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8876180 [details]
Bug 1371709 - Disallow capabilities with duplicate profiles,

https://reviewboard.mozilla.org/r/147604/#review153938

In the future, I would prefer if the dependency update was done in a separate commit.  When it is included with the consumer changes in geckodriver in the same commit, it is harder for me as a reviewer to know what I need to look at.

::: testing/geckodriver/src/capabilities.rs:186
(Diff revision 2)
>                                  .iter()
>                                  .all(|value| value.is_string()) {
>                                  return Err(WebDriverError::new(
>                                      ErrorStatus::InvalidArgument,
>                                           "args entry is not a string"));
>                                  }

This closing brace is in the wrong indentation block.

	Andreas Tolfsen ❲:ato❳
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8876181 [details]
Bug 1371709 - Support passing profile to Firefox wdspec tests,

https://reviewboard.mozilla.org/r/147606/#review153942

	Andreas Tolfsen ❲:ato❳
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8876180 [details]
Bug 1371709 - Disallow capabilities with duplicate profiles,

https://reviewboard.mozilla.org/r/147604/#review153944

::: testing/geckodriver/src/capabilities.rs:187
(Diff revision 2)
> +                            // Check for both a -profile argument and a profile capability
> +                            if data.contains_key("profile") {
> +                                for arg_json in args {
> +                                    if let Some(arg) = arg_json.as_string() {
> +                                        if is_profile_arg(arg) {
> +                                            return Err(WebDriverError::new(ErrorStatus::InvalidArgument,
> +                                                                           "Got a profile argument in args and a profile capability"));
> +                                        }
> +                                    }
> +                                }
> +                            }

Please document this change in both testing/geckodriver/README.md and testing/geckodriver/CHANGES.md.

	Andreas Tolfsen ❲:ato❳
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8877994 [details]
Bug 1371709 - Update wpt metadata,

https://reviewboard.mozilla.org/r/149402/#review153946