Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 8 commits into from
Oct 29, 2016
Merged
5 changes: 5 additions & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ matrix:
env: WEBDRIVER=phantomjs
- php: 5.5
env: WEBDRIVER=phantomjs PHANTOM_VERSION=2
- php: 7.0
env: WEBDRIVER=selenium-remote
sudo: required
services:
- docker

before_script:
- sh bin/run-"$WEBDRIVER".sh
Expand Down
7 changes: 7 additions & 0 deletions bin/run-selenium-remote.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#!/usr/bin/env sh
set -e

echo ' Downloading selenium'
docker pull selenium/standalone-firefox:2.53.1
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

echo ' Running selenium'
docker run -d -p 4444:4444 --network=host selenium/standalone-firefox:2.53.1
62 changes: 61 additions & 1 deletion src/Selenium2Driver.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use Behat\Mink\Exception\DriverException;
use Behat\Mink\Selector\Xpath\Escaper;
use WebDriver\Element;
use WebDriver\Exception\InvalidRequest;
Copy link
Member

Choose a reason for hiding this comment

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

use WebDriver\Exception\NoSuchElement;
use WebDriver\Exception\UnknownError;
use WebDriver\Exception;
Expand Down Expand Up @@ -794,7 +795,18 @@ public function attachFile($xpath, $path)
$element = $this->findElement($xpath);
$this->ensureInputType($element, $xpath, 'file', 'attach a file on');

$element->postValue(array('value' => array($path)));
// Upload the file to Selenium and use the remote path. This will
// ensure that Selenium always has access to the file, even if it runs
// as a remote instance.
try {
$remote_path = $this->uploadFile($path);
Copy link
Member

Choose a reason for hiding this comment

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

please use camelCase variables names

}
catch (InvalidRequest $e) {
// File could not be uploaded to remote instance. Use the local path.
$remote_path = $path;
}

$element->postValue(array('value' => array($remote_path)));
}

/**
Expand Down Expand Up @@ -1120,4 +1132,52 @@ private function trigger($xpath, $event, $options = '{}')
$script = 'Syn.trigger("' . $event . '", ' . $options . ', {{ELEMENT}})';
$this->withSyn()->executeJsOnXpath($xpath, $script);
}

/**
* Uploads a file to the Selenium instance.
*
* 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
Copy link
Member

Choose a reason for hiding this comment

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

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

*
* @param string $path The path to the file to upload.
*
* @return string The remote path.
*
* @throws DriverException When the file is not found.
* @throws InvalidRequest When the driver does not support file uploads.
*/
public function uploadFile($path) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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

if (!is_file($path)) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not strictly needed :)

throw new DriverException('Could not upload file, the file: ' . $path . '. was not found.');
}

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

$archive = new \ZipArchive();
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

$archive->open($temp_filename, \ZipArchive::CREATE);
Copy link
Member

Choose a reason for hiding this comment

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

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

$archive->addFile($path, basename($path));
$archive->close();

try {
$remote_path = $this->getWebDriverSession()->file(array('file' => base64_encode(file_get_contents($temp_filename))));
Copy link
Member

Choose a reason for hiding this comment

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

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

}
catch (InvalidRequest $e) {
Copy link
Member

Choose a reason for hiding this comment

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

Is InvalidRequest the only possible error from file upload operation?

Maybe we should catch and rethrow all exceptions?

// Catch the error so we can still clean up the temporary archive.
}

unlink($temp_filename);

// If the file upload failed, then probably Selenium was not used but
// another web driver such as PhantomJS.
// @todo Support other drivers when (if) they get remote file transfer
// capability.
if (empty($remote_path)) {
Copy link
Member

Choose a reason for hiding this comment

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

Better to replace with this:

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

The above version:

  • doesn't recreate exception based
  • just reuses exception caught before

throw new InvalidRequest();
}

return $remote_path;
}

}