Issue #187: Add support for uploading files to remote Selenium instance #252

pfrenssen · 2016-10-27T13:50:02Z

Here's a proposed fix for #187. This works both on local and remote instances. I tested using a local installation of Selenium and a Docker instance.

Fixes minkphp#187.

pfrenssen · 2016-10-27T15:36:48Z

This still needs some work. There's a PHP error, but I also see that it now fails on a local instance of PhantomJS when running in webdriver mode.

aik099 · 2016-10-28T15:32:17Z

bin/run-selenium-remote.sh

+set -e
+
+echo '    Downloading selenium'
+docker pull selenium/standalone-firefox:2.53.1


This uses Docker on Travis CI? I wonder how slow that might me.

It's quite comparable it seems, the build takes about 30 seconds longer. It looks like this time is needed to download the docker image.

I think this is the most cost effective way of testing a remote instance of Selenium. If we want to try a "real" remote then we probably have to go with a hosted service like Browserstack.

pfrenssen · 2016-10-28T16:07:48Z

This is ready for a review. The tests are failing but this is due to the known problem #245

aik099 · 2016-10-28T19:02:33Z

src/Selenium2Driver.php

+     * @throws InvalidRequest  When the driver does not support file uploads.
+     */
+    public function uploadFile($path) {
+        if (!is_file($path)) {


There wasn't such check before. Is it even needed?

Not strictly needed :)

aik099 · 2016-10-28T19:03:25Z

src/Selenium2Driver.php

+     * @throws DriverException When the file is not found.
+     * @throws InvalidRequest  When the driver does not support file uploads.
+     */
+    public function uploadFile($path) {


The opening { of function declaration should be placed on next line.

P.S.
I wonder why Scrutinizer CI haven't picked that up.

aik099 · 2016-10-28T19:04:46Z

src/Selenium2Driver.php

+        // Selenium only accepts uploads that are compressed as a Zip archive.
+        $temp_filename = tempnam('', 'WebDriverZip');
+
+        $archive = new \ZipArchive();


Is ZipArchive class present in any PHP installation. If it's possible for some OS to have limited PHP installation, then we'd better check that such class exists and throw useful exception instead of getting Fatal Error on missing ZipArchive class.

Good point!

Also it seems, that you always are using ZipArchive class now.

To preserve BC I think, that:

in case if class is absent and we know it's remote file (not exists locally) then throw DriverException

in case case if class is absent and it's local file just upload file the old way without even creating ZipArchive

This method is not replacing the uploading that was done before. That part is still handled by Selenium2Driver::attachFile() which ultimately does the sendKeys() webdriver command to attach the file to the file field. What this does is attempt to transfer the file from the local environment to the remote instance.

Do you think it would be maybe a good idea to rename this method to transferFile() to make this clearer?

When doing this transfer the file always needs to be zipped, it never works otherwise. If you try to transfer an unzipped file to Selenium you get this error:

Expected there to be only 1 file. There were: 0 Build info: version: '3.0.0', revision: '350cf60', time: '2016-10-13 10:48:16 -0700' System info: host: 'N/A', ip: 'N/A', os.name: 'Linux', os.arch: 'amd64', os.version: '4.8.4-1-ARCH', java.version: '1.8.0_91' Driver info: driver.version: unknown

Note that it is not possible to figure out if Selenium runs locally or remotely. A remote instance might be forwarded to a local port, appearing exactly the same to us as a local instance. So the only thing we can do is to attempt a transfer. When this succeeds we get a remote path back which we can use. When anything goes wrong we catch this and continue as normal with the original file path.

I've restructured it a bit now so that it works as you suggest, any error throws an exception which will be caught and the file will be attached in the old way.

Then when ZipArchive class isn't available we should execute old code immediately without throwing an exception and let driver fail as it fails currently.

Yes that is what is going to happen now. If ZipArchive is not there a DriverException is thrown, and this is caught by attachFile() which will then continue with the original (local) path. It will then fail exactly as it does now later on when the sendKeys command is executed.

aik099 · 2016-10-28T19:06:15Z

src/Selenium2Driver.php

+        $archive->close();
+
+        try {
+          $remote_path = $this->getWebDriverSession()->file(array('file' => base64_encode(file_get_contents($temp_filename))));


Use $this->wdSession directly (as done in other methods of the driver) instead of calling $this->getWebDriverSession() method.

aik099 · 2016-10-28T19:08:30Z

src/Selenium2Driver.php

+        // another web driver such as PhantomJS.
+        // @todo Support other drivers when (if) they get remote file transfer
+        // capability.
+        if (empty($remote_path)) {


Better to replace with this:

if (isset($e)) { throw $e; }

The above version:

doesn't recreate exception based

just reuses exception caught before

aik099 · 2016-10-28T19:09:20Z

src/Selenium2Driver.php

+        try {
+          $remote_path = $this->getWebDriverSession()->file(array('file' => base64_encode(file_get_contents($temp_filename))));
+        }
+        catch (InvalidRequest $e) {


Is InvalidRequest the only possible error from file upload operation?

Maybe we should catch and rethrow all exceptions?

pfrenssen · 2016-10-28T22:23:50Z

Addressed the remarks. This is ready for another look :)

aik099 · 2016-10-29T09:30:32Z

src/Selenium2Driver.php

+     *
+     * Note that uploading files is not part of the official WebDriver
+     * specification, but it is supported by Selenium.
+     * @see https://github.com/SeleniumHQ/selenium/blob/master/py/selenium/webdriver/remote/webelement.py#L533


Please move @see tag after @throws. Or in any place, where other methods in this project has @see tag placed.

aik099 · 2016-10-29T09:32:22Z

Except moving of @see tag the code looks good to me.

aik099 · 2016-10-29T09:33:17Z

src/Selenium2Driver.php

@@ -13,6 +13,7 @@
 use Behat\Mink\Exception\DriverException;
 use Behat\Mink\Selector\Xpath\Escaper;
 use WebDriver\Element;
+use WebDriver\Exception\InvalidRequest;


The Scrutinizer CI says that this class import isn't used anymore: https://scrutinizer-ci.com/g/minkphp/MinkSelenium2Driver/inspections/89c102e8-c656-4124-b660-6129c7ec2f16/patches?status=new

pfrenssen · 2016-10-29T10:19:26Z

Thanks for the quick reviews! I have addressed the remarks.

aik099 · 2016-10-29T17:04:09Z

have you actually checked, that new tests will fail without new library code changes?
when test suite is running locally on developer machine all will work properly as before, but of course no remote upload tests?

pfrenssen · 2016-10-29T20:15:24Z

I don't fully understand the question.

I need this fix because we are running our tests on continuousphp which recently switched to remote Selenium instances and our uploads were failing.

I have tested on my dev machine with local and remote instances of Selenium. It's now also working on continuousphp which it wasn't before.

pfrenssen · 2016-10-29T20:48:50Z

Oh you probably mean to run the tests on Travis using a remote Selenium instance, but without the fix.

I made a branch containing only the test, but without the fix. Here is the relevant test result: https://travis-ci.org/pfrenssen/MinkSelenium2Driver/jobs/171671291

You can see that in the response the text 1 uploaded file is missing after some_file.txt. This proves that without the fix the file is not uploaded to the remote instance.

Here is the same test result with the fix: https://travis-ci.org/pfrenssen/MinkSelenium2Driver/jobs/171596712

In this case you can see that the text 1 uploaded file is present in the response, so the file is correctly uploaded to the remote instance.

aik099 · 2016-10-29T21:30:38Z

That's exactly what I've meant. Good.

aik099 · 2016-10-29T21:31:10Z

Merging, thanks @pfrenssen .

frankcarey · 2016-12-14T17:00:53Z

This was such a pain for so long, THANK YOU!

stof · 2016-12-14T17:58:25Z

src/Selenium2Driver.php

+        // ensure that Selenium always has access to the file, even if it runs
+        // as a remote instance.
+        try {
+          $remote_path = $this->uploadFile($path);


please use camelCase variables names

stof · 2016-12-14T17:58:34Z

src/Selenium2Driver.php

+        try {
+          $remote_path = $this->uploadFile($path);
+        }
+        catch (\Exception $e) {


should be on the previous line

stof · 2016-12-14T17:58:55Z

src/Selenium2Driver.php

+     *
+     * @see https://github.com/SeleniumHQ/selenium/blob/master/py/selenium/webdriver/remote/webelement.py#L533
+     */
+    public function uploadFile($path)


should be private

stof · 2016-12-14T18:02:24Z

src/Selenium2Driver.php

+        $temp_filename = tempnam('', 'WebDriverZip');
+
+        $archive = new \ZipArchive();
+        $archive->open($temp_filename, \ZipArchive::CREATE);


you need to check the return value here, as ZipArchive uses the return value to handle errors instead of throwing exceptions

aik099 · 2016-12-14T19:16:10Z

@stof , thanks for reviewing, but PR is already merged. All the fixes should go as separate PR I guess.

stof · 2016-12-14T20:15:00Z

yeah, I received a notification about this PR because of the comment, and I reviewed it while noticing it was merged only at the end.
Doing a new PR for the fixes is indeed the way to go (if it is not done at the time I work on releasing, I will do it myself, as I don't want to release with the method exposed publicly and so covered by BC)

pfrenssen · 2016-12-15T16:26:03Z

Thanks for the review. I will open a followup to address the remarks. Will link it here when I make the PR.

volkyeth · 2016-12-28T10:07:37Z

Is there any roadmap for the next release yet? This feature by itself would deserve a release.

asprega · 2017-01-01T10:46:26Z

+1. Is there a planned release for this feature?

Fix review remarks. Followup for #252.

kvdnberg · 2017-05-04T09:50:04Z

+1 for release! We spend days in agony trying to fix this problem while testing CircleCI 2.0 with docker images (which can't share volumes). When I finally found this yesterday I was thrilled. One note: I had to debug it to get it to work because my docker php didn't have zip support and because the exception is caught and the 'local' file is then used, it seemed like this didn't fix the problem. So if anyone else is checking this fix and it doesn't seem to work, check your zip extension ;)

pfrenssen · 2017-05-24T12:57:14Z

I made a separate issue that suggests to make a new release, the comments here are probably not going to be seen since this is already closed: #269

Add support for uploading files to remote Selenium instances.

8ff8c99

Fixes minkphp#187.

pfrenssen added 3 commits October 28, 2016 16:38

Add support for local PhantomJS instances.

de3acde

Test a remote Selenium instance.

c6fded0

Use Firefox instead of Chrome for the remote Selenium instance.

f519ea6

aik099 reviewed Oct 28, 2016

View reviewed changes

pfrenssen mentioned this pull request Oct 28, 2016

uses chrome as web browser ec-europa/joinup-dev#233

Closed

aik099 requested changes Oct 28, 2016

View reviewed changes

pfrenssen added 2 commits October 29, 2016 00:53

Fix review remarks.

3bff87e

Add documentation.

0a18331

Throw a helpful exception when the file to upload doesn't exist locally.

2064e99

aik099 reviewed Oct 29, 2016

View reviewed changes

Fix review remarks.

506a2a0

aik099 approved these changes Oct 29, 2016

View reviewed changes

aik099 merged commit 721cbba into minkphp:master Oct 29, 2016

aik099 mentioned this pull request Oct 29, 2016

Problems with file uploads on remote hosts #187

Closed

pfrenssen deleted the remote-file-upload branch October 30, 2016 13:51

pfrenssen mentioned this pull request Nov 4, 2016

switch from PhantomJS to Selenium2 ec-europa/platform-dev#943

Closed

stof reviewed Dec 14, 2016

View reviewed changes

pfrenssen added a commit to pfrenssen/MinkSelenium2Driver that referenced this pull request Dec 15, 2016

Fix review remarks. Followup for minkphp#252.

4d626ae

pfrenssen mentioned this pull request Dec 15, 2016

Fix review remarks. Followup for #252. #253

Merged

stof added a commit that referenced this pull request Jan 3, 2017

Merge pull request #253 from pfrenssen/252-followup

28b4a56

Fix review remarks. Followup for #252.

pfrenssen mentioned this pull request May 24, 2017

Create a new release #269

Open

stof mentioned this pull request Jan 7, 2018

Implementation of setFileDetector #224

Closed

ryanaslett mentioned this pull request Sep 3, 2019

Change of master branch alias to 1.4.x-dev breaks packages depending on 1.3.x-dev #313

Closed

Issue #187: Add support for uploading files to remote Selenium instance #252

Issue #187: Add support for uploading files to remote Selenium instance #252

Conversation

pfrenssen commented Oct 27, 2016

pfrenssen commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfrenssen commented Oct 28, 2016

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

pfrenssen commented Oct 28, 2016

Choose a reason for hiding this comment

aik099 commented Oct 29, 2016

Choose a reason for hiding this comment

pfrenssen commented Oct 29, 2016

aik099 commented Oct 29, 2016

pfrenssen commented Oct 29, 2016

pfrenssen commented Oct 29, 2016

aik099 commented Oct 29, 2016

aik099 commented Oct 29, 2016

frankcarey commented Dec 14, 2016

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

aik099 commented Dec 14, 2016

stof commented Dec 14, 2016

pfrenssen commented Dec 15, 2016

volkyeth commented Dec 28, 2016

asprega commented Jan 1, 2017

kvdnberg commented May 4, 2017

pfrenssen commented May 24, 2017