Switch to fb webdriver #263

uuf6429 · 2017-02-05T10:00:09Z

No description provided.

aik099

In general I like the change, but too many unneeded formatting changes to clearly see it.

aik099 · 2017-02-05T10:04:10Z

.travis.yml

@@ -6,7 +6,7 @@ cache:
  directories:
    - $HOME/.composer/cache/files

-php: [5.3, 5.4, 5.5, 5.6, 7.0, 7.1, hhvm]


Revert. See #262 (comment).

aik099 · 2017-02-05T10:04:29Z

.travis.yml

@@ -35,7 +35,7 @@ before_script:
  # Start a webserver for web fixtures. Force using PHP 5.6 to be able to run it on PHP 5.3 and HHVM jobs too
  - MINK_PHP_BIN=~/.phpenv/versions/5.6/bin/php vendor/bin/mink-test-server > /dev/null 2>&1 &

-script: phpunit -v --coverage-clover=coverage.clover


Revert. See #262 (comment).

aik099 · 2017-02-05T10:04:59Z

composer.json

@@ -20,13 +20,14 @@
    ],

    "require": {
-        "php":                       ">=5.3.1",


Revert. See #262 (comment).

aik099 · 2017-02-05T10:05:24Z

composer.json

    },

    "require-dev": {
-        "mink/driver-testsuite": "dev-master"


Revert. See #262 (comment).

aik099 · 2017-02-05T10:06:25Z

phpunit.xml.dist

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>

-<phpunit colors="true" bootstrap="vendor/autoload.php">


Revert. You can specify it for now in .travis.yml and command line option to PHPUnit. No need to alter default phpunit config file.

aik099 · 2017-02-05T10:10:53Z

src/Selenium2Driver.php

@@ -186,17 +151,17 @@ public function getWebDriverSession()
     */
    public static function getDefaultCapabilities()
    {
-        return array(
-            'browserName'       => 'firefox',


No need to remove formatting of multi-line array calls.

aik099 · 2017-02-05T10:12:01Z

src/Selenium2Driver.php

        }

        return json_encode($options);
    }

    /**
-     * Executes JS on a given element - pass in a js script string and {{ELEMENT}} will


Why the {{ELEMENT}} was replaced with __ELEMENT__? If I recall correctly it's driver itself who parses {{ELEMENT}} and not WebDriver/Facebook.

It shows up as JS errors in my IDE

It seems to be replaced by webdriver: https://github.com/uuf6429/MinkSelenium2Driver/blob/05872053bd733959b25fa4df3f3aa41bef845d64/src/Selenium2Driver.php#L251-L251

Of course it will show as JavaScript error in IDE because {{ELEMENT}} is invalid syntax. Since it works please replace __ELEMENT__ back into {{ELEMENT}}.

Why do you want to show errors when they're in fact just replaced tokens? O.o

Because changes in PR should be on topic. Making unrelated change doesn't help anybody.

You can send another PR with just that change and we can merge/not merge it separately regardless of faith of this PR.

Plus, it is a BC break as it is a change affecting a protected method

aik099 · 2017-02-05T10:12:21Z

src/Selenium2Driver.php

            $this->applyTimeouts();
        } catch (\Exception $e) {
-            throw new DriverException('Could not open connection: '.$e->getMessage(), 0, $e);


Revert. No need to change formatting.

aik099 · 2017-02-05T10:13:34Z

src/Selenium2Driver.php

    private function clickOnElement(Element $element)
    {
-        $this->wdSession->moveto(array('element' => $element->getID()));
+        //$element->click(); // TODO why was this needed?


What this comment is for? The click is called because method is called clickOnElement 😄

Ah, never mind... seems to be an artefact from my changes (I thought old code called $element->click(); twice).

Then you can safely remove it.

aik099 · 2017-02-05T10:14:05Z

src/Selenium2Driver.php

-        }
-
-        $element->postValue(array('value' => array($remotePath)));
+        $element->sendKeys($path);


Are you sure, that sendKeys is clever enough to handle file upload code on local/remote servers?

Yes: https://github.com/facebook/php-webdriver/blob/85aa68020ef2cf4fd261b1f1a690052afe67a9e0/lib/Remote/RemoteWebElement.php#L324

Good. In any case we have functional tests to prove that it really works as expected.

uuf6429 · 2017-02-05T10:15:14Z

phpunit.xml.dist

@@ -1,6 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>

-<phpunit colors="true" bootstrap="vendor/autoload.php">
+<phpunit colors="true" bootstrap="vendor/autoload.php" stopOnFailure="true">


This is temporary until we get all tests working.

Still please move it to .travis.yml. This would allow people checking out PR branch (for reviewing purposes) like me and using alternative PHPUnit runner not have this stopOnFailure behavior.

stof · 2017-02-06T08:21:11Z

Why closing the PR ?

aik099 · 2017-02-06T08:23:26Z

Why closing the PR ?

Because nobody noticed upfront that Facebook's version of WebDriver requires PHP 5.5+ to run. Since Mink/Mink Drivers require PHP 5.3+ to run we have no way to use it.

stof · 2017-02-06T08:37:24Z

Well, as PHP 5.3 and 5.4 are EOLed, dropping support for them could be acceptable if we have some benefit when doing it (switching to a maintained library).
The previous PR dropping it for Mink was dropping support with no benefit for us. Here, it could have one.

Some drivers may drop support for older PHP versions (the third-party phantomJS driver does not support PHP 5.3 btw)

stof · 2017-02-06T08:40:59Z

src/Selenium2DriverSession.php

+use Facebook\WebDriver\WebDriverBy;
+use Facebook\WebDriver\WebDriverElement;
+
+class Selenium2DriverSession


Why creating this class ?

It's a proxy class to avoid changing the original driver too much. In the end, I could merge all changes into one class.

aik099 · 2017-02-06T10:07:19Z

Here, it could have one.

Switching one dependency for another, while both work at the end is no benefit to me. Fact that one dependency is better maintained than another one doesn't really change the fact that both work.

volkyeth · 2017-02-15T18:52:08Z

@aik099 actually there are many limitations on the instaclick version that are not present on the facebook one. Off the top of my head I can remember the iframe manipulation bit, for instance. On instaclick/php-webdriver you can only change iframes based on it's id, whereas in facebook/php-webdriver you can use the id, name or an WebDriverElement

aik099 · 2017-02-15T21:12:12Z

Off the top of my head I can remember the iframe manipulation bit, for instance.

This particular change isn't a problem, because public API of Mink supporting locating frames by name only anyway.

Since Instaclick version covers 100% of functionality needed to implement the driver it doesn't really matter at this point if we switch to FB at all.

uuf6429 · 2017-02-15T21:49:51Z

Some other reasons:

maintained library is possibly faster and in case of performance/bug/security issues, there's a higher chance of a patch
the FB library takes care of things like handling uploads and other stuff that is unnecessarily replicated in MinkSelenium2Driver.

That said, I gave up arguing here. It's a huge shame this project will die like this "because it works".

volkyeth · 2017-02-15T23:31:48Z

@uuf6429 no reason for it to die at all, just put your fork up on Packagist and I'll help you maintain it. No trouble at all if the main one is stagnant and becomes obsolete, we'll just move forward using yours.

aik099 · 2017-02-16T08:02:08Z

I see you're eager to change stuff even though it works as expected for years now. I'm not here to judge how you should spend your time, but in your place I won't be spending time on doing anything that won't result in any kind of gain.

In this case I don't see any actual gains except FB library is more maintained than Instaclick one. There can't be any possible performance gain from upgrade, because both FB and Instaclick are doing remote calls using WebDriver protocol, where speed is result of how fast we get a response and not how fast library can process it.

Einenlum · 2017-04-04T21:05:59Z

Seems instaclick does not officially support Selenium 3.x, am I right?
Therefore, if a library is still active (facebook) and the other is not, plus does not supports officially Selenium 3.x, why shouldn't we change this dependency?

Moreover, if it's a question of BC and minimum PHP version, it's also our duty to make all this community evolve. Even 5.6 is not actively supported anymore. People with previous PHP versions will still have older versions of MinkSelenium2Driver.

uuf6429 added 9 commits February 1, 2017 03:11

Switch to FB webdriver

d5f73f2

Various implementations

502cfdb

Increased minimum PHP version to 5.5

0e6c204

Added phpunit dev dep and stop on failure

dfb3258

Fix attempt

3f1a406

Remove webdriver setter (BC break)

496eec3

Fixed file upload impl. & code style

0587205

Removed config added by mistake

bd9486a

Use composer phpunit in travis

f8efc7a

uuf6429 mentioned this pull request Feb 5, 2017

Relying on old package #262

Closed

Stop on failure

d86efd5

aik099 requested changes Feb 5, 2017

View reviewed changes

uuf6429 commented Feb 5, 2017

View reviewed changes

CR and CS fixes

f100c95

uuf6429 closed this Feb 5, 2017

stof reviewed Feb 6, 2017

View reviewed changes

aik099 mentioned this pull request Oct 4, 2017

Selenium 3 support #254

Closed

aik099 mentioned this pull request Jan 4, 2019

[WIP] Facebook driver #304

Closed

		@@ -1,6 +1,6 @@
		<?xml version="1.0" encoding="UTF-8"?>

		<phpunit colors="true" bootstrap="vendor/autoload.php">

Switch to fb webdriver #263

Switch to fb webdriver #263

Conversation

uuf6429 commented Feb 5, 2017

aik099 left a 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

uuf6429 Feb 5, 2017 • edited

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

stof commented Feb 6, 2017

aik099 commented Feb 6, 2017 • edited

stof commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aik099 commented Feb 6, 2017

volkyeth commented Feb 15, 2017

aik099 commented Feb 15, 2017

uuf6429 commented Feb 15, 2017

volkyeth commented Feb 15, 2017

aik099 commented Feb 16, 2017 • edited

Einenlum commented Apr 4, 2017

uuf6429 Feb 5, 2017 •

edited

aik099 commented Feb 6, 2017 •

edited

aik099 commented Feb 16, 2017 •

edited