search

solidusio / solidus_dev_support

A collection of tools for developing Solidus extensions.
MIT License
21 stars 27 forks source link

Switch the JS driver to WebDriver/Selenium #136

Closed elia closed 4 years ago

elia commented 4 years ago

Summary

Since apparition has an open issue with Chrome 85 we want to open up to other drivers and let individual extensions pick the one they prefer we'll switch to "webdrivers" which is Rails' default.

~~Cuprite, like Apparition, is based on CDP, for fast runs and more control over the browser. Webdrivers relies on the WebDriver W3C standard and Selenium with the largest cross-browser support and the default for Rails.~~

Checklist

aldesantis commented 4 years ago

@elia I'm not too fond of this to be honest. The point of solidus_dev_support is to save the time of extension maintainers and let them focus on the code that is specific to their extension, while solidus_dev_support handles all other aspects. Giving them multiple drivers to choose from may be cool in theory, but it doesn't really create any value, and it may create confusion in the ecosystem due to different extensions using different drivers.

My suggestion is to simply switch to a stable, battle-tested driver instead such as Selenium.

elia commented 4 years ago

@aldesantis I agree with you in principle, yet the CI is still green for all extensions because the CircleCI ORB is still using an image with an older Chrome version. Forcefully switching to WebDriver as the default will surely break most extensions using the JS driver. So the reasoning behind this is exactly to provide easy to use defaults, such as driver definitions that already provide the correct resolution (avoiding problems with the hidden admin menu etc.). And allowing maintainers to simply switch driver and fix the failing specs to overcome the open issue on apparition.

This is not about being cool, but rather about how to create the smoothest experience for maintainers. Also the fact that Selenium is much more limited and is noticeably slower might be a problem in some scenarios, requiring impossible workarounds to deal with async requests, for a mild example see the changes required to make it work in solidus_content.

aldesantis commented 4 years ago

@elia I still don't see the point of shipping with multiple drivers out of the box: extensions will use the default driver we ship with unless the driver is overridden. If we need to override it because the tests are failing with the new default for a specific extension, then we may as well fix the tests to work with the new default driver (preferable) or override the driver in the extension itself.

It's a philosophical matter I guess: we should ship with one set of sane defaults, and then it should be clear that if you want to deviate from those defaults, you'll need to take on the additional burden of maintaining the customization.

Have you taken a look at Selenium to see how many tests break across all extensions and how much slower they are compared to Apparition? The reason I don't like Cuprite is that, while it may be better, it's still pre-1.0 and not widely known, and we may run into issues very similar to the ones we've had with Apparition.

If this becomes too problematic because there are too many broken tests to fix, we may also decide to simply sit it out and keep Apparition, waiting for the problem to be fixed upstream. We don't have that many JS tests in extensions as far as I remember, and it shouldn't be too disruptive to the ecosystem - especially considering that the CI is green.

elia commented 4 years ago

I can remove cuprite if that's the problem, I included it just in case an extension needs that extra level of control over the browser, and since not having the right resolution tripped me up, I thought it was good to give a default, and also choice.

If you feel it's better to just change the default that's also fine by me, having red CIs just means we need to fix 'em. 👍

Just let me know what you prefer

aldesantis commented 4 years ago

I think I'd feel much more comfortable shipping with only Selenium, and then if extension maintainers need to override it for some reason they can do it themselves. 👍

Thanks! 🙏

elia commented 4 years ago

@aldesantis done, at this point we should consider shipping 2.0

aldesantis commented 4 years ago

@elia looks like CI is failing, can you take a look?

aldesantis commented 4 years ago

@elia looks good now. A couple of things before we merge:

elia commented 4 years ago
  • Should we change the title of the PR? The new driver is Selenium, webdrivers is just a gem that keeps Selenium WebDrivers up to date.

I changed it to mention both WebDriver (the standard) and Selenium, I hope it's fine

  • I'm not sure this is a breaking change. I get why you marked it as breaking, but since the API hasn't changed, does that really make sense? 🤔

Since it will break the CI of some extensions, and given the domain of this gem is aiding development (vs. production use) I think it's correct, also technically the API changes, both in behavior (the same method on a different driver can behave differently) and in the available features (anything starting with page.driver.).

aldesantis commented 4 years ago

@elia thanks for fixing, feel free to release 2.0 whenever you want.