Skip to content
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

Use webdrivers.cr for managing drivers #80

Merged
merged 1 commit into from
May 15, 2020
Merged

Use webdrivers.cr for managing drivers #80

merged 1 commit into from
May 15, 2020

Conversation

matthewmcgarvey
Copy link
Member

@matthewmcgarvey matthewmcgarvey commented May 14, 2020

Fixes #74
Fixes #54

Summary

Rather than depending on the user to have chromedriver installed or including a vendored version with the library, use webdrivers.cr to install and manage the drivers. After switching to this library, we will now have all the infrastructure in place to begin offering testing in other browsers like Firefox.

Considerations

This library will make http calls in order to find the latest drivers and install them. There are potential failure points that come with it, especially as this library is not as battle-tested as the ruby version.

By default it installs drivers at ~/.webdrivers but it can be configured.

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One tiny suggestion. Thank you so much for all the work you've done. One of the goals of Lucky is to abstract things like this away so it Just Works. Thanks for helping us with that goal!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/lucky_flow.cr Outdated Show resolved Hide resolved
Copy link
Member

@jwoertink jwoertink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah snap! Loving all these new updates! I think aside from the error message update Paul suggested, it all looks good to me.

@@ -15,7 +16,7 @@ class LuckyFlow
setting base_uri : String
setting retry_delay : Time::Span = 10.milliseconds
setting stop_retrying_after : Time::Span = 1.second
setting chromedriver_path : String = LuckyFlow.default_driver_path
setting chromedriver_path : String?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paulcsmith This might be a bug with Habitat or a known limitation but when I had the installation here and caused a failure, it failed with a message about recursion while setting class variables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm could you open an issue in Habitat? I don’t think I’ve seen that before and am not sure I understand what caused it but it seems like we should support it or at least raise a better error :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the issue luckyframework/habitat#46

Copy link
Member

@paulcsmith paulcsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thank you 🙏

@@ -15,7 +16,7 @@ class LuckyFlow
setting base_uri : String
setting retry_delay : Time::Span = 10.milliseconds
setting stop_retrying_after : Time::Span = 1.second
setting chromedriver_path : String = LuckyFlow.default_driver_path
setting chromedriver_path : String?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm could you open an issue in Habitat? I don’t think I’ve seen that before and am not sure I understand what caused it but it seems like we should support it or at least raise a better error :D

@paulcsmith paulcsmith merged commit 0b67f74 into luckyframework:master May 15, 2020
@matthewmcgarvey matthewmcgarvey deleted the webdrivers branch May 15, 2020 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants