Allow to use different drivers in the same test suite

[randoum]

In my test suite I use different drivers, including appium_capybara.

I get this message for tests that use other drivers (selenium for example)

NoMethodError: undefined method `browser_initialized?' for #<Capybara::Selenium::Driver:0x007fdcf1773028>
appium_capybara/lib/appium_capybara/ext/session_ext.rb:5:in `reset!'

This pull request fix it

[bootstraponline]

Is this still a problem in the v1.0.0 release of appium_capybara?

[randoum]

Yes same problem with 1.0.0

[bootstraponline]

use && instead of and. and has weird precedence rules.

a = true and false; a
=> true

[randoum]

That's because = takes precedence over and, add parenthesis to prove it

a = (true and false); a
=> false

On the other hand, and takes precedence over if

 if true and false; 'yes'; else; 'no'; end
=> "no"

Therefore in ruby if a and b is fully equivalent to if a && b
But I'll edit it to follow your suggestion

[bootstraponline]

The and and or keywords are banned. It's just not worth it. Always use && and || instead.
See the github style guide.

[bootstraponline]

squash to one commit:

git reset --soft head~2
git commit -am "Allow to use different drivers in the same test suite"
git push -f

I think the code looks fine once the issues are addressed.

/cc @sbonebrake what do you think?

[bootstraponline]

this check should be inside the if statement

[randoum]

It could, but it is easier to read like this won't you think?

[bootstraponline]

sorry, you're right. I missed this was used in the next line. 👍

[bootstraponline]

once this is squashed then the code looks good to me.

[sbonebrake]

I agree this should be fixed, but your solution doesn't allow this to work with other drivers that may have implemented .browser_initialized? or patched to have it. Instead of checking for "driver.instance_of? Appium::Capybara::Driver", why not "browser_initialized = driver.respond_to?("is_initialized") ? driver.is_initialized? : true". In fact, I should see if Capybara would accept that as a fix for the bug.

[randoum]

@bootstraponline rebase done. On my side I can't see how it affected the pull request, can you?

@sbonebrake Well your thought is correct but it does not apply in this case, because a correct implementation of a Capybara driver is to inherit from its class and that's what all driver does.
In appium_capybara you guys are patching Selenium classes, so this problem would occur only between Selenium and AppiumCapybara, and as of now browser_initialized? is specific to AppiumCapybara.

[bootstraponline]

@bootstraponline rebase done. On my side I can't see how it affected the pull request, can you?

you should force push git push -f

[randoum]

Ok now?

[bootstraponline]

I'm still seeing 3 commits. You ran all 3 commands?

git reset --soft head~3
git commit -am "Allow to use different drivers in the same test suite"
git push -f

[bootstraponline]

looks correct now. I agree with @sbonebrake about using respond_to?

[sbonebrake]

The browser being reinitialized during session reset is an issue that affects all drivers, not just appium capybara. Using respond_to? is the best way I know to allow users to make their own patches to implement browser_initialized? or use it in any custom drivers they have.

[randoum]

The browser being reinitialized during session reset is an issue that affects all drivers, not just appium capybara.

That's not what I discuss about.

AppiumCapybara is a very useful project, and I believe it is purposed for a long life when the community will start to understand its advantages. But it is, as of now, made of a very bad design. Let me explain.

This are some highlights on the current design:

• Appium::Capybara::Driver inherit from Selenium
• Appium::Capybara::Node inherit from Capybara
• there are a lot of monkey patching of Capybara classes (see the file driver/base_ext.rb and all files under ext directory)

This mix-up of libraries and the abuse of monkey patching will lead to several problems. 1) if Capybara update its code you will have issues 2) it is impossible to maintain compatibility with different version of Capybara on the long run 3) your patches may already not be compatible with existing drivers.

Your project is still young and problems dues to this design already start to rise: AppiumCapybara is breaking Selenium (which is turn lead me to open this issue). One of those monkey patch is forcing the base implementation of Capybara to call browser_initialized?, which is a method that is specific to AppiumCapybara.

Each and every other existing driver inherit from Capybara classes or define their own classes from scratch. This is the correct way to go and you should do the same.

• I assume all this patching is a temporary solution and you would soon implement a correct design of your driver by creating its own classes that inherit from Capybara, am I correct?
• In the emergency, and before you fix the design, I propose this quick fix which is as stable as was the patch of Capybara::Session. It is not intended to be a permanent solution
• No actual Capybara driver are using a method called browser_initialized?, therefore there is no need for respond_to?. If users are patching Capybara on their own they already have work to do to implement AppiumCapybara that also patches Capybara, and you cannot handle all user cases.

As a conclusion, even is my 3 points above demonstrate that it is useless, I will still update to respond_to? because you ask me to, but I do hope that you are not considering the actual design as a permanent solution and you will be thoughtful to my explanation and update the whole design very soon.

[bootstraponline]

I agree it'd be better without the monkey patches. The Appium::Capybara::Driver inherits from Capybara::Selenium::Driver. As for the rest, I'm not sure how to fix that.

I do hope that you are not considering the actual design as a permanent solution

I'm just volunteering for appium_capybara because I want the project to be successful. I don't use it at work so the current design being permanent or not depends on contributors. If you want to improve the design and @sbonebrake agrees with the changes then I'm happy to see them merged into the project.

[randoum]

@sbonebrake what do you say?

[sbonebrake]

Addressing a the issues one by one:

• Over use of the patches: This was never supposed to be permanent. appium_capybara was just basically a series of patches I was using at work to get appium to function in capybara. Ideally we don't want to have any patches and push them upstream into capybara, but really depends on how open they are to issues such as using selectors other than css and xpath.
• We probably should have the appium node inherit from the selenium node assuming those selenium nodes have use for mobile web browser automation. @bootstraponline would know more about this than me. If we choose not to do this, then we should go back to having them both inherit from the base.
• browser_initialized issue - I proposed a ways to fix this to capybara in this thread Session's reset! is causing a new browser to launch if the browser has already been quit. teamcapybara/capybara#1237 if they don't respond within the week I think we should take the solution with responds_to?

[bootstraponline]

We probably should have the appium node inherit from the selenium node assuming those selenium nodes have use for mobile web browser automation.

Yes, I'd expect the selenium nodes to be useful for mobile web.

[randoum]

Over use of the patches: This was never supposed to be permanent. appium_capybara was just basically a series of patches I was using at work to get appium to function in capybara. Ideally we don't want to have any patches and push them upstream into capybara

Nice. And I pretty sure we could find a clean solution to the reset! problem while getting rid of the patches.
So I don't know about the time you can dedicate to appium_capybara, but I believe we would all agree to say that the sooner we do it, the better. Would you guys create a 2.0 branch in github to start working on it?
Also I can help a bit if you need me to

[sbonebrake]

What do you guys think? Merge this is now or cross our fingers and hope my PR for issue teamcapybara/capybara#1237 is pulled in soonish? I'm leaning towards merging this now even if we have to pull it back out for the next version.

[bootstraponline]

I added a +1 to teamcapybara/capybara#1415

I guess it's going to take a bit for them to merge and even longer to make a release. I think it makes sense to merge now and revert later.