Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed the issue with new selenium v2.35 session id retrieval handshake #266

Merged
merged 4 commits into from

5 participants

@zerkms
Collaborator

A fix for #265

@webdigi

Great the fix works! verified with selenium v2.35 on mac

@julianseeger

Looks like Giorgio is deadlocked in distributed transactions, but we really need a merge here.
Since Firefox 23 does not work well with selenium < 2.35 and phpunit-selenium is meant to work with firefox, there is no argument in supporting only selenium <= 2.33.

PHPUnit/Extensions/Selenium2TestCase/Response.php
@@ -83,6 +83,17 @@ public function getValue()
*/
public function getURL()
{
- return new PHPUnit_Extensions_Selenium2TestCase_URL($this->info['url']);
+ $url = $this->info['url'];
+ $sessionId = $this->jsonResponse['sessionId'];
+
+ // if url doesn't have sessionId included - append it manually
+ // this change was performed in selenium v2.35

was performed in selenium 2.34

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
PHPUnit/Extensions/Selenium2TestCase/Response.php
@@ -83,6 +83,17 @@ public function getValue()
*/
public function getURL()
{
- return new PHPUnit_Extensions_Selenium2TestCase_URL($this->info['url']);
+ $url = $this->info['url'];
+ $sessionId = $this->jsonResponse['sessionId'];
+
+ // if url doesn't have sessionId included - append it manually
+ // this change was performed in selenium v2.35
+ // @see https://code.google.com/p/selenium/issues/detail?id=6089
+ // @see https://github.com/sebastianbergmann/phpunit-selenium/issues/265
+ if (strpos($url, $sessionId) === false) {
+ $url = trim($url, '/') . '/' . $sessionId;

why trim the url? by default doesn't have starting or ending "/" no?

@zerkms Collaborator
zerkms added a note

@felixcarmona is it guaranteed to always be without trailing slash?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
PHPUnit/Extensions/Selenium2TestCase/Response.php
@@ -83,6 +83,17 @@ public function getValue()
*/
public function getURL()
{
- return new PHPUnit_Extensions_Selenium2TestCase_URL($this->info['url']);
+ $url = $this->info['url'];
+ $sessionId = $this->jsonResponse['sessionId'];
+
+ // if url doesn't have sessionId included - append it manually
+ // this change was performed in selenium v2.34
+ // @see https://code.google.com/p/selenium/issues/detail?id=6089
+ // @see https://github.com/sebastianbergmann/phpunit-selenium/issues/265
+ if (strpos($url, $sessionId) === false) {
+ $url = trim($url, '/') . '/' . $sessionId;

i have been checked this and i have seen that the $this->info['url'] is not including the ending /, you can check also

@zerkms Collaborator
zerkms added a note

@felixcarmona yep, it doesn't at the moment, but I don't see any guarantee that JsonWireProtocol provides about it

@zerkms, so, should sanitize it always no?, not only in this if statement no?
i think this trim (or rtrim) should be done when the $url is setted at the first time:
$url = rtrim($this->info['url'], '/');

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
PHPUnit/Extensions/Selenium2TestCase/Response.php
@@ -83,6 +83,17 @@ public function getValue()
*/
public function getURL()
{
- return new PHPUnit_Extensions_Selenium2TestCase_URL($this->info['url']);
+ $url = $this->info['url'];
+ $sessionId = $this->jsonResponse['sessionId'];
+
+ // if url doesn't have sessionId included - append it manually
+ // this change was performed in selenium v2.34
+ // @see https://code.google.com/p/selenium/issues/detail?id=6089
+ // @see https://github.com/sebastianbergmann/phpunit-selenium/issues/265
+ if (strpos($url, $sessionId) === false) {
+ $url = rtrim($url, '/') . '/' . $sessionId;

why not rtrim the $url when the $url contains the $sessionId?
why not do the rtrim when you set the $url at the first time?
$url = rtrim($this->info['url'], '/');

@zerkms Collaborator
zerkms added a note

@felixcarmona the aim of this patch is to bring phpunit-selenium back to work, so I implemented the fix in the most clear and obvious way I see it :-)

so, the url sanitize in the if statement (with rtrim) is also out of the aim of the patch proposal, the url should be sanitized at the two ways or at nothing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@zerkms
Collaborator

@felixcarmona finally I could satisfy your demands :-)

@giorgiosironi giorgiosironi merged commit a573a76 into giorgiosironi:master
@giorgiosironi

Do we need a new release right now or master is fine?

@felixcarmona

master seem fine, because this change respects the retrocompatibility

@zerkms zerkms deleted the zerkms:selenium-2.35-handshake-change branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 12 additions and 1 deletion.
  1. +12 −1 PHPUnit/Extensions/Selenium2TestCase/Response.php
View
13 PHPUnit/Extensions/Selenium2TestCase/Response.php
@@ -83,6 +83,17 @@ public function getValue()
*/
public function getURL()
{
- return new PHPUnit_Extensions_Selenium2TestCase_URL($this->info['url']);
+ $url = $this->info['url'];
+ $sessionId = $this->jsonResponse['sessionId'];
+
+ // if url doesn't have sessionId included - append it manually
+ // this change was performed in selenium v2.34
+ // @see https://code.google.com/p/selenium/issues/detail?id=6089
+ // @see https://github.com/sebastianbergmann/phpunit-selenium/issues/265
+ if (strpos($url, $sessionId) === false) {
+ $url .= '/' . $sessionId;
+ }
+
+ return new PHPUnit_Extensions_Selenium2TestCase_URL($url);
}
}
Something went wrong with that request. Please try again.