-
Notifications
You must be signed in to change notification settings - Fork 189
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: expose public interface for locating Chrome installations #177
feat: expose public interface for locating Chrome installations #177
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
36cca01
to
2be0045
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nits
thanks @knksmith57!
Add Launcher.getInstallations() public static method that returns an array of paths to available Chrome binaries. Also, refactor Launcher#launch() to use it. Launcher.getInstallations() uses chrome-finder under the hood, so all previous resolution logic is preserved: - if CHROME_PATH env variable is set, it will be included - if a Chrome Canary is detected, it will be included - if a Chrome (stable) is detected, it will be included Resolves GoogleChrome#176
fb74c30
to
71e46d3
Compare
OK looks like adding that additional assert worked (I didn't realize we codified the chrome dep in the travis config; I should have looked at that first 😅). I think I've addressed all feedback --thanks again for the super fast review! |
yup the rest of our tests would fail if we didn't have chrome available so it's a safe bet :) |
looks great, thanks again @knksmith57! 🎉 💯 |
great idea :D |
Add
Launcher.getInstallations()
public static method that returns anarray of paths to available Chrome binaries. Also, refactor
Launcher#launch()
to use it.Launcher.getInstallations()
useschrome-finder
under the hood, so allprevious resolution logic is preserved:
CHROME_PATH
env variable is set, it will be includedResolves #176