Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

As https://github.com/mozilla/geckodriver/issues/800 shows, the "timeouts" Marionette command was removed in https://bugzilla.mozilla.org/show_bug.cgi?id=1375425, but we forgot to release a new geckodriver version that uses "setTimeouts" instead.

	James Graham [:jgraham]
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8884822 [details]
Bug 1379482 - Add hgignore rules for geckodriver for parity;

https://reviewboard.mozilla.org/r/155704/#review160722

	James Graham [:jgraham]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8884823 [details]
Bug 1379482 - Update geckodriver README with -profile and -headless;

https://reviewboard.mozilla.org/r/155706/#review160726

::: testing/geckodriver/README.md:342
(Diff revision 1)
> -and run it with a specific command-line flag,
> -set a preference,
> -and enable verbose logging:
> +to run with a prepared profile from the filesystem
> +in headless mode (available on certain systems and recent Firefoxen).
> +It also increases the number of IPC processes through a preference
> +and enables more verbose logging.
> +
> +You will note that the following example

I think `You will note` is not ideal since someone might not note. Also this entire paragraph seems to be hinting at something being possible without properly explaining it. I'm not sure it's that helpful and would be inclined to remove it. If you really want to rewrite it, you need an example of legacy style capabilities.

::: testing/geckodriver/README.md:553
(Diff revision 1)
>  and acts as a proxy between [WebDriver] and [Marionette].
>  
> -In order to build this program, you will need the [Rust compiler toolchain].
> -
> -To build the project for release,
> -ensure you compile with optimisations
> +geckodriver is built in the Firefox CI by default
> +but _not_ if you build Firefox locally.
> +To enable building of geckodriver locally,
> +ensure you put this in your mozconfig:

Link to instructions for using `mozconfig`?

::: testing/geckodriver/README.md:557
(Diff revision 1)
> -To build the project for release,
> -ensure you compile with optimisations
> -to get the best performance:
> +To enable building of geckodriver locally,
> +ensure you put this in your mozconfig:
> +
> +	ac_add_options --enable-geckodriver
> +
> +The next time you build, you should see a geckodriver binary appear in your objdir:

All this seems like too much information. For a gecko developer knowing how to set up the mozconfig, and where the target will appear should be enough; adding the log output and output of `file` is superflous.

	James Graham [:jgraham]
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8884825 [details]
Bug 1379482 - Update geckodriver repository address;

https://reviewboard.mozilla.org/r/155710/#review160730

::: testing/geckodriver/Cargo.toml:10
(Diff revision 1)
>    "James Graham <james@hoppipolla.co.uk>",
>    "Andreas Tolfsen <ato@mozilla.com>",
>  ]
>  description = "Proxy for using WebDriver clients to interact with Gecko-based browsers."
>  keywords = ["webdriver", "w3c", "httpd", "mozilla", "firefox"]
> -repository = "https://github.com/mozilla/geckodriver"
> +repository = "https://searchfox.org/mozilla-central/source/testing/geckodriver"

Searhfox is an unofficial tool and may vanish. It seems preferable to link to `hg.mozilla.org` even though it's less good.

	James Graham [:jgraham]
	Comment 10 • 7 years ago
mozreview-review

Comment on attachment 8884826 [details]
Bug 1379482 - Upgrade mozrunner to 0.4.1;

https://reviewboard.mozilla.org/r/155712/#review160732

::: testing/geckodriver/Cargo.lock:5
(Diff revision 1)
>  [root]
>  name = "geckodriver"
> -version = "0.17.0"
> +version = "0.18.0"
>  dependencies = [
>   "chrono 0.2.25 (registry+https://github.com/rust-lang/crates.io-index)",

Are there no dependency updates we should do for this release?

	James Graham [:jgraham]
	Comment 11 • 7 years ago
mozreview-review

Comment on attachment 8884821 [details]
Bug 1379482 - Sync geckodriver README with GitHub;

https://reviewboard.mozilla.org/r/155702/#review160720

::: testing/geckodriver/README.md:59
(Diff revision 1)
>  Support is best in Firefox 53 and greater,
>  although generally the more recent the Firefox version,
>  the better the experience as they have more bug fixes and features.
>  Some features will only be available in the most recent Firefox versions,
>  and we strongly advise using the [latest Firefox Nightly](https://nightly.mozilla.org/) with geckodriver.
> -Since Windows XP support was dropped with Firefox 53,
> +Since Windows XP support in Firefox will be dropped with Firefox 53,

This change seems like a regression.

	James Graham [:jgraham]
	Comment 12 • 7 years ago
mozreview-review

Comment on attachment 8884824 [details]
Bug 1379482 - Update geckodriver changelog for 0.18.0;

https://reviewboard.mozilla.org/r/155708/#review160728

::: testing/geckodriver/CHANGES.md:12
(Diff revision 1)
>  ### Changed
> -- Log geckodriver version on startup
> +- [`RectResponse`](https://docs.rs/webdriver/0.27.0/webdriver/response/struct.RectResponse.html) permits returning floats for `width` and `height` fields
> +- New type [`CookieResponse`](https://docs.rs/webdriver/0.27.0/webdriver/response/struct.CookieResponse.html) for the [`GetNamedCookie` command](https://docs.rs/webdriver/0.27.0/webdriver/command/enum.WebDriverCommand.html#variant.GetNamedCookie) returns a single cookie, as opposed to an array of a single cookie
> +- To pick up a prepared profile from the filesystem, it is now possible to pass `["-profile", "/path/to/profile"]` in the `args` array on `moz:firefoxOptions`
> +- geckodriver now recommends Firefox 53 and greater
> +- Version information (`--version`) contains the hash from whence geckodriver was built

`whence` is obscure. `from the commit used to build geckodriver` seems better.

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

Comment on attachment 8884821 [details]
Bug 1379482 - Sync geckodriver README with GitHub;

https://reviewboard.mozilla.org/r/155702/#review160720

> This change seems like a regression.

We have made changes to the README on GitHub that have not made it into m-c.  This commit takes what’s on GitHub and puts it in-tree.  I’m addressing this in https://reviewboard.mozilla.org/r/155706/diff/1#index_header later in the series.

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

Comment on attachment 8884826 [details]
Bug 1379482 - Upgrade mozrunner to 0.4.1;

https://reviewboard.mozilla.org/r/155712/#review160732

> Are there no dependency updates we should do for this release?

Thanks for reminding me.

I thought, apparently naïvely, that `./mach vendor rust' would do
this for me, but I should’ve known better.  Until cargo supports
using crates.io as a fallback to check if there are newer versions
of crates I guess we need to pin the crates exactly to force it to
download them.

I have introduced a new commit in this patch series that pins
mozrunner 0.4.1 exactly in testing/geckodriver/Cargo.toml.  This
forces `./mach vendor rust' to fetch and vendor the latest version.

	James Graham [:jgraham]
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8884826 [details]
Bug 1379482 - Upgrade mozrunner to 0.4.1;

https://reviewboard.mozilla.org/r/155712/#review160802

::: testing/geckodriver/Cargo.toml:20
(Diff revision 2)
>  chrono = "^0.2"
>  clap = {version = "^2.19", default-features = false, features = ["suggestions", "wrap_help"]}
>  hyper = "0.10"
>  lazy_static = "0.1"
>  log = "0.3"
> -mozprofile = "0.3"
> +mozprofile = "0.3.0"

So I prefer specifying the patchless version unless we actually depend on features in a specific patch revision, but it doesn't matter much.

	James Graham [:jgraham]
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8884868 [details]
Bug 1379482 - Release geckodriver 0.18.0;

https://reviewboard.mozilla.org/r/155738/#review160804

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 22 • 7 years ago
mozreview-review

Comment on attachment 8884823 [details]
Bug 1379482 - Update geckodriver README with -profile and -headless;

https://reviewboard.mozilla.org/r/155706/#review160810

::: testing/geckodriver/README.md:551
(Diff revision 2)
> -ensure you compile with optimisations
> +ensure you put this in your [mozconfig]:
> -to get the best performance:
>  
> -	% cargo build --release
> +	ac_add_options --enable-geckodriver
>  
> -Or if you want a non-optimised binary for debugging:
> +The _geckodriver_ binary will appear in `${objdir}/dist/bin/geckodriver`

Please note that you have to run the following commands:

./mach configure
./mach build testing/geckodriver

Also you might want to add that artifact builds should not be enabled.

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

Comment on attachment 8884826 [details]
Bug 1379482 - Upgrade mozrunner to 0.4.1;

https://reviewboard.mozilla.org/r/155712/#review160802

> So I prefer specifying the patchless version unless we actually depend on features in a specific patch revision, but it doesn't matter much.

I do too, but unless the system works I’m not sure we can do
anything.  When nfroyd or ted is back, I will ask them how we intend
to solve crate upgrades.

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

Comment on attachment 8884823 [details]
Bug 1379482 - Update geckodriver README with -profile and -headless;

https://reviewboard.mozilla.org/r/155706/#review160810

> Please note that you have to run the following commands:
> 
> ./mach configure
> ./mach build testing/geckodriver
> 
> Also you might want to add that artifact builds should not be enabled.

Like jgraham said, telling Mozilla developers how to build Firefox
is a little out of the scope so I removed the instructions on how to
invoke a build.

In any case, I don’t feel this should be blocking our release of
0.18.0.

	Pulsebot
	Comment 32 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/67f6029e979c
Sync geckodriver README with GitHub; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/25710188cc52
Add hgignore rules for geckodriver for parity; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/b9d438ef444f
Update geckodriver README with -profile and -headless; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/24577b96157a
Upgrade mozrunner to 0.4.1; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/e204b3cfaa07
Update geckodriver changelog for 0.18.0; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/59602c27b8e5
Update geckodriver repository address; r=jgraham
https://hg.mozilla.org/integration/autoland/rev/0e2cd0ea8392
Release geckodriver 0.18.0; r=jgraham

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 33 • 7 years ago
mozreview-review-reply

Comment on attachment 8884823 [details]
Bug 1379482 - Update geckodriver README with -profile and -headless;

https://reviewboard.mozilla.org/r/155706/#review160810

> Like jgraham said, telling Mozilla developers how to build Firefox
> is a little out of the scope so I removed the instructions on how to
> invoke a build.
> 
> In any case, I don’t feel this should be blocking our release of
> 0.18.0.

Please keep in mind that also people from the Selenium community might want to compile geckodriver for testing latest changes. Not supplying the information is a blocker for them. Talk to Simon Stuart who tried this on Friday, and which has taken him a while. :/

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 34 • 7 years ago

(In reply to Henrik Skupin (:whimboo) [partly available 07/10 -07/14] from comment #33)
> Comment on attachment 8884823 [details]
> Bug 1379482 - Update geckodriver README with -profile and -headless;
> 
> https://reviewboard.mozilla.org/r/155706/#review160810
> 
> > Like jgraham said, telling Mozilla developers how to build Firefox
> > is a little out of the scope so I removed the instructions on how to
> > invoke a build.
> > 
> > In any case, I don’t feel this should be blocking our release of
> > 0.18.0.
> 
> Please keep in mind that also people from the Selenium community might want
> to compile geckodriver for testing latest changes. Not supplying the
> information is a blocker for them. Talk to Simon Stuart who tried this on
> Friday, and which has taken him a while. :/

Submit a patch.

In the two years since we’ve worked on geckodriver, we have received in total
a number of patches I can count on one hand.  Going by metrics, this is not
a thing that is a blocker.

	Carsten Book [:Tomcat]
	Comment 35 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/67f6029e979c
https://hg.mozilla.org/mozilla-central/rev/25710188cc52
https://hg.mozilla.org/mozilla-central/rev/b9d438ef444f
https://hg.mozilla.org/mozilla-central/rev/24577b96157a
https://hg.mozilla.org/mozilla-central/rev/e204b3cfaa07
https://hg.mozilla.org/mozilla-central/rev/59602c27b8e5
https://hg.mozilla.org/mozilla-central/rev/0e2cd0ea8392