Automatically detect Chrome browser binary if it exists

[DanielRyanSmith]

The current implementation of the Chrome class does not check the specified virtualenv path for an existing Chrome browser binary. Passing in the venv_path when trying to find the binary will ensure the binary is found properly if it exists.

Also, currently a generic error "Unable to locate Chrome binary" will be thrown when running tests against Chrome via wpt run chrome command with no browser binary in the virtualenv directory. Because a browser binary must exist in the virtualenv directory and not just the local machine, a more verbose error message has been added to explain how to fix the issue.
EDIT: This part is more of a separate issue, and can be fixed at a later time.

[jgraham]

So the upshot of this is that if we try to find a nightly binary and fail, we'll fall back to using just google-chrome on the $PATH on linux and similar default names on other channels? It's unclear to me if that's expected.

[DanielRyanSmith]

That is the intent, the current implementation will just throw a generic error if a nightly binary doesn't exist, no falling back. But maybe this change is not a good idea from some usability perspective and falling back could be an issue?

[DanielRyanSmith]

It looks like Firefox has a similar implementation where the virtual environment is checked, and then falls back if a binary is not located there:

wpt/tools/wpt/browser.py

Lines 268 to 285 in ab2592d

	binary = self.find_binary_path(path, channel)
	if not binary and self.platform == "win":
	winpaths = [os.path.expandvars("$SYSTEMDRIVE\\Program Files\\Mozilla Firefox"),
	os.path.expandvars("$SYSTEMDRIVE\\Program Files (x86)\\Mozilla Firefox")]
	for winpath in winpaths:
	binary = self.find_binary_path(winpath, channel)
	if binary is not None:
	break
	if not binary and self.platform == "macos":
	macpaths = ["/Applications/Firefox Nightly.app/Contents/MacOS",
	os.path.expanduser("~/Applications/Firefox Nightly.app/Contents/MacOS"),
	"/Applications/Firefox Developer Edition.app/Contents/MacOS",
	os.path.expanduser("~/Applications/Firefox Developer Edition.app/Contents/MacOS"),
	"/Applications/Firefox.app/Contents/MacOS",
	os.path.expanduser("~/Applications/Firefox.app/Contents/MacOS")]
	return find_executable("firefox", os.pathsep.join(macpaths))

[jgraham]

The difference is that Firefox doesn't have different binary names for nightly vs stable. Since Chrome does, I'm not sure if it makes sense to use something that's known to not be a nightly build when the channel was specified as nightly.

[DanielRyanSmith]

I see, thank you for the clarification. I am bringing this up because this was a pain point when I first tried to run the tests in this repository. Do you think it would be better to simply add a more verbose error message for this section? Something like "Binary not found in virtual environment. Run ./wpt install to ensure you have the latest binary."

"nightly" is also the default choice when you do not specify a channel and run the tests, so this issue comes up whenever a user specifies nothing (which is why I ran into this issue as someone new to wpt).

wpt/tools/wpt/install.py

Lines 27 to 37 in ae28863

	channel_args = argparse.ArgumentParser(add_help=False)
	channel_args.add_argument('--channel', choices=channel_by_name.keys(),
	default='nightly', action='store',
	help='''
	Name of browser release channel (default: nightly). "stable" and "release" are
	synonyms for the latest browser stable release; "beta" is the beta release;
	"dev" is only meaningful for Chrome (i.e. Chrome Dev); "nightly",
	"experimental", and "preview" are all synonyms for the latest available
	development or trunk release. (For WebDriver installs, we attempt to select an
	appropriate, compatible version for the latest browser release on the selected
	channel.) This flag overrides --browser-channel.''')

[DanielRyanSmith]

I updated the code to no longer fall back to looking on the $PATH or other channels if a browser binary is not found on the virtualenv path. This should behave exactly as the current implementation except a more verbose error message will display on how to fix the issue.

[DanielRyanSmith]

@jgraham The more I thought about this, the more it feels like a separate issue from the main change. The most important part of this PR is the venv_path change, which fixes an issue where the Chrome browser binary would go completely undetected even if it existed in virtualenv. I removed the above change, and if it's still important, I will return to it when I am certain of the correct approach.

[jgraham]

Thanks!

Re the Chrome issue, I think we need to fix the channel handling overall. Currently I think we pick a default channel too early in the process when none is supplied, when really we should defer picking a channel until after we found a binary we're going to use, because often we can infer the channel from the binary. So for Chrome we'd handle the channel=None binary=None case by looking for all possible binaries and picking one to use, and then setting the channel from the selected binary.

[DanielRyanSmith]

we should defer picking a channel until after we found a binary we're going to use, because often we can infer the channel from the binary. So for Chrome we'd handle the channel=None binary=None case by looking for all possible binaries and picking one to use, and then setting the channel from the selected binary.

@jgraham Do you think this is worth raising as a separate issue?

[jgraham]

I think there's already several issues that cover bits and pieces of this. I'll pick #16024 as the place to discuss/fix this.