Skip to content

Conversation

beatngu13
Copy link
Contributor

@beatngu13 beatngu13 commented Aug 1, 2019

EventFiringWebDriver also implements WrapsDriver, which is more general and should be implemented by all wrappers. This recently cause issues at retest/recheck-web#277.

As soon as Selenium is updated, org.openqa.selenium.internal.WrapsDriver should be replaced by org.openqa.selenium.WrapsDriver. (Should be available as of Selenium v3.14.0, see SeleniumHQ/selenium@3665dd7#diff-c68b4f7410690a3e2973585f06af6b92.)

@glibas glibas self-assigned this Aug 1, 2019
@glibas
Copy link
Member

glibas commented Aug 1, 2019

Hi @beatngu13 , Thank you for your contribution. Need to think how we can support both versions of the WrapsDriver. Will take a look tonight

@@ -109,8 +109,8 @@ public BufferedImage takeScreenshot() {
* @return BufferedImage resulting image
*/
public BufferedImage takeScreenshotEntirePage() {
if (driver instanceof EventFiringWebDriver) {
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest adding something like

   private WebDriver unwrapDriver(String name){
        try {
            if(Class.forName(name).isInstance(driver)){
                return ((WrapsDriver) driver).getWrappedDriver();
            }
        } catch (ClassNotFoundException e) {
            ;
        }
        return driver;
    }

So that then in takeScreeshotEntirePage() we can be backward compatible with earlier selenium versions e.g.

 driver = unwrapDriver("org.openqa.selenium.WrapsDriver");
 driver = unwrapDriver("org.openqa.selenium.internal.WrapsDriver");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since you have bumped the Selenium version in the meantime, I would rather rebase my branch and switch to org.openqa.selenium.WrapsDriver, which is implemented by org.openqa.selenium.internal.WrapsDriver, so this would support both. (See subinterfaces: https://seleniumhq.github.io/selenium/docs/api/java/org/openqa/selenium/WrapsDriver.html).

`EventFiringWebDriver` also implements `WrapsDriver`, which is more general and should be implemented by all wrappers.
@glibas
Copy link
Member

glibas commented Aug 1, 2019

Selenium dependency is only in compile scope so it won't be included in the distribution library. We still need to check for both versions of the WrapsDriver as if someone is using shutterbug with selenium version prior to 3.14.0 they'll get runtime error.

@beatngu13
Copy link
Contributor Author

Isn't this the default scope anyway?

However, it's your lib; if you want me to go down the path of reflection, I'll do. 😉

@beatngu13
Copy link
Contributor Author

Added a new commit that unwraps via reflection. Feel free to delete it if you want to stick to the new org.openqa.selenium.WrapsDriver.

@glibas glibas merged commit f99a84e into assertthat:master Aug 2, 2019
beatngu13 added a commit to retest/recheck-web that referenced this pull request Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants