Introduce driver registration #133

matthewmcgarvey · 2022-01-13T04:28:00Z

Fixes #132

This adds in the concept of registering drivers and also allows different drivers to be used in a test suite. The end goal is to allow for an in-memory driver alternative to be used as the default and any that need it can be set to use headless chrome.

This is a breaking change:

Several settings were removed that were counter to this new process
LuckyFlow::Server was deleted as it didn't serve well in this model
LuckyFlow::Spec.setup must be used to correctly configure LuckyFlow for use in specs
LuckyFlow::Drivers::HeadlessChrome was deleted as it is now just a chrome driver with slightly different config

There are also bugs in the underlying Selenium library that I found when trying to run specs with headless and non-headless chrome. This are not blocking from the default path of using this library (single driver for all specs).

With the goal of allowing for an in-memory driver alternative, the next step is to remove all direct access to the underlying selenium library since the new driver won't be provided through selenium.

Could delete screenshots taken from different driver in same suite

matthewmcgarvey · 2022-01-13T04:29:06Z

spec/spec_helper.cr

@@ -5,6 +5,8 @@ require "./support/**"

 include LuckyFlow::Expectations

+LuckyFlow::Spec.setup


This sets up the lucky flow driver if one is requested through tags on the spec, and then resets back to the default driver after the spec is run.

It also has a callback for shutting down the drivers once all specs finish.

I wonder if there would be a way to force people to call this... Or like, you try to run some specs, but you forgot to add this so you get a compile-time error telling you to add it 🤔

I don't know about "force", but another option would be to have luckyflow/spec unrequired by the library but requires luckyflow and does the Spec setup right in the file. Then we document that in order to use the library in your specs, you must require "lucky_flow/spec" after you require "spec" and it will just work. It could potentially be a pattern that we could support spectator in the future if we want to (just require "lucky_flow/spectator"). Thoughts on that?

matthewmcgarvey · 2022-01-13T04:29:58Z

src/ext/spec/item.cr

@@ -0,0 +1,12 @@
+module Spec
+  module Item
+    def all_tags : Set(String)


Tags on a spec don't include parent contexts (context, describe) so this monkey-patched method is to facilitate that.

matthewmcgarvey · 2022-01-13T04:30:47Z

src/lucky_flow.cr

@@ -52,9 +74,9 @@ class LuckyFlow

  def take_screenshot(filename : String = generate_screenshot_filename, fullsize : Bool = true)
    if fullsize
-      with_fullsized_page { session.screenshot(filename) }
+      with_fullsized_page { driver.screenshot(filename) }


Switched to the driver instead of the session so that the driver could make the screenshot directory if it's missing

matthewmcgarvey · 2022-01-13T04:31:32Z

src/lucky_flow/driver.cr

+  abstract def stop
+
+  def reset : Nil
+    @session.try &.cookie_manager.delete_all_cookies


we use @session instead of the getter so that it doesn't start a session if there wasn't one already

What happens if this is called before session is set?

It will be nil, so the .try will avoid an error

matthewmcgarvey · 2022-01-13T04:32:35Z

src/lucky_flow/drivers/chrome.cr


-  private def browser_binary : String?
-    LuckyFlow.settings.browser_binary
+LuckyFlow::Registry.register :headless_chrome do


Don't like this config? Just call LuckyFlow::Registry.register :headless_chrome do ... end with your own setup and it will overwrite this one 👍

matthewmcgarvey · 2022-01-13T04:33:19Z

src/lucky_flow/registry.cr

@@ -0,0 +1,21 @@
+class LuckyFlow::Registry
+  @@registry = Hash(String, Proc(LuckyFlow::Driver)).new
+  @@running_registry = Hash(String, LuckyFlow::Driver).new


@@registry is for drivers that can be used but aren't made yet,
@@running_registry are drivers that have been created and should be used before making a new one

matthewmcgarvey · 2022-01-13T04:35:29Z

src/lucky_flow/spec.cr

+module LuckyFlow::Spec
+  macro setup
+    Spec.around_each do |spec|
+      if driver_name = (spec.example.all_tags & LuckyFlow::Registry.available).first?


In Rspec, you can pass key-value pairs in it's but in Crystal you only have tags which are a list of strings. So instead of requiring passing something like driver: :chrome, you have to do tags: "chrome" instead and then we take the overlap of the tags you passed with the available browsers you can use. If for some reason you do tags: ["headless_chrome", "chrome"], there'd be an overlap of 2 but we take the first

jwoertink · 2022-01-13T16:11:17Z

src/lucky_flow/driver.cr

+
+  def screenshot(path : String)
+    FileUtils.mkdir_p(File.dirname(path))
+    session.screenshot(path)


should this also use the instance variable?

No, the only places I used the instance variable was places where if it was never started, there's nothing to stop/reset

jwoertink · 2022-01-13T16:24:22Z

src/ext/spec/item.cr

+  module Item
+    def all_tags : Set(String)
+      all_tags = tags || Set(String).new
+      temp = parent


Does this include all parents, or just the direct parent?

describe Thing do context "when thing", tags: ["chrome"] do describe "is a thing" do it "does stuff" do end end end end

Yes, in the if below I call all_tags on the parent which means it will fetch all tags of ever parent until it reaches the Spec::RootContext which is not a Spec::Item

ah, ok. I see what it's doing now. Can we rename the internal variable so it doesn't shadow the method name?

I ended up using the same naming convention we were recommended to use over here https://github.com/luckyframework/avram/blob/ac281f490a85283eaf8b74fd23357dd70a6d7f22/src/ext/db/connection.cr#L7

with an underscore and the name of the shard so in this case _lucky_flow_all_tags. I also added # :nodoc:. It's not a very clear name but it doesn't need to be

Cool, I like that.

jwoertink

I think this looks good!

matthewmcgarvey added 7 commits January 12, 2022 18:10

Driver registration

265bd41

Begin refactoring drivers to prepare for registration

f75b1ab

Switch to driver registration

3f4fe61

Add ability to chose driver through spec tags

3d8592d

more minor cleanup

8a32eee

Don't delete screenshot dir when creating driver

5a947ff

Could delete screenshots taken from different driver in same suite

Add macro for setting up LuckyFlow spec config

35c8854

matthewmcgarvey commented Jan 13, 2022

View reviewed changes

matthewmcgarvey mentioned this pull request Jan 13, 2022

Error when using more than one type of chrome driver #134

Open

jwoertink added the BREAKING CHANGE label Jan 13, 2022

jwoertink reviewed Jan 13, 2022

View reviewed changes

matthewmcgarvey added 3 commits January 13, 2022 19:24

Use naming convention for monkey-patched methods

10f6672

Make sure it's also not documented

968d1fc

Reset driver after spec if present, set driver as expected

2b36a00

jwoertink approved these changes Jan 14, 2022

View reviewed changes

matthewmcgarvey merged commit da9c947 into master Jan 14, 2022

matthewmcgarvey deleted the driver-registration branch January 14, 2022 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce driver registration #133

Introduce driver registration #133

matthewmcgarvey commented Jan 13, 2022 •

edited

Loading

matthewmcgarvey Jan 13, 2022

jwoertink Jan 13, 2022

matthewmcgarvey Jan 14, 2022

matthewmcgarvey Jan 13, 2022

matthewmcgarvey Jan 13, 2022

matthewmcgarvey Jan 13, 2022

jwoertink Jan 13, 2022

matthewmcgarvey Jan 13, 2022

matthewmcgarvey Jan 13, 2022

matthewmcgarvey Jan 13, 2022

matthewmcgarvey Jan 13, 2022

jwoertink Jan 13, 2022

matthewmcgarvey Jan 13, 2022

jwoertink Jan 13, 2022

matthewmcgarvey Jan 13, 2022

jwoertink Jan 13, 2022

matthewmcgarvey Jan 14, 2022

jwoertink Jan 14, 2022

jwoertink left a comment

		@@ -5,6 +5,8 @@ require "./support/**"

		include LuckyFlow::Expectations

		LuckyFlow::Spec.setup

Introduce driver registration #133

Introduce driver registration #133

Conversation

matthewmcgarvey commented Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwoertink left a comment

Choose a reason for hiding this comment

matthewmcgarvey commented Jan 13, 2022 •

edited

Loading