get_ok always fails against 2.42

[jamadam]

get_ok always fails in real world test case and the use case in t directory also fails.

$ perl -Ilib t/Test-Selenium-Remote-Driver-google.t
not ok 1 - get'http://www.google.com'
#   Failed test 'get'http://www.google.com''
#   at t/Test-Selenium-Remote-Driver-google.t line 15.
ok 2
ok 3
1..3
# Looks like you failed 1 test of 3.

The browser successfully shows the site and no error found in selenium server log.

Is this my environmental problem?

[jamadam]

This is because the server returns content for get command with null value since selenium server version 2.42.
The server use to returns no content for get command so the final result of get method returns error message which happen to evaluated as true and get_ok passed.

I'm trying to fix some how but no idea for now :)

[gempesaw]

Yeah, looks like they changed the API return value for GETing /session/:sessionId/url in 2.42. Hm.

[jamadam]

Yes that's what I wanted to say :)
Here is a workaround for it but it's too evil to PR.

jamadam@8c48a8e

Wonder if *_ok methods can check $resp->{cmd_status} eq 'OK' some how.

[jamadam]

hmm. It doesn't work. cmd_status always set to OK.

[ghost]

Could we combine this with my proposal in issue #130 ?

[jamadam]

Hi. olivierBlanc.

I'm not pretty sure for now your proposal has to do something with this but please take over it :)

By the way I've got another workaround for getting success out of _execute_command just in case wantarray solution is not acceptable.

jamadam@05dc1da

[gempesaw]

@peroumal1 Hey, do you have any input here? The ::DoesTesting role, now that the return values have changed for some methods as of 2.42.x, is apparently giving different behavior from before. I was hoping we could contain the changes to DoesTesting.pm, since the testing methods like get_ok probably shouldn't have any overlap to the underlying Driver.pm...

[peroumal1]

I will try to have a look as soon as I have some time ;)

On Wed, Jun 25, 2014 at 5:08 PM, Daniel Gempesaw notifications@github.com
wrote:

@peroumal Hey, do you have any input here? The ::DoesTesting role, now
that the return values have changed for some methods as of 2.42.x, is
apparently giving different behavior from before. I was hoping we could
contain the changes to DoesTesting.pm, since the testing methods like
get_ok probably shouldn't have any overlap to the underlying Driver.pm...

—
Reply to this email directly or view it on GitHub
#135 (comment)
.

Peroumalnaik Emmanuel

[peroumal1]

So...
2.42.2 response :

 0  HTTP::Response=HASH(0x4189670)
   '_content' => '{"status":0,"sessionId":"66884be3-8aae-4a1d-a8db-96e6e9daace1","value":null,"state":"success","class":"org.openqa.selenium.re
mote.Response","hCode":813457600}'
   '_headers' => HTTP::Headers=HASH(0x4189658)
      'cache-control' => ARRAY(0x41897d8)
         0  'no-cache'
         1  'no-cache'
      'client-date' => 'Wed, 25 Jun 2014 15:31:59 GMT'
      'client-peer' => '127.0.0.1:4444'
      'client-response-num' => 1
      'connection' => 'close'
      'content-length' => 158
      'content-type' => 'application/json; charset=utf-8'
      'date' => 'Wed, 25 Jun 2014 15:31:58 GMT'
      'expires' => ARRAY(0x4189778)
         0  'Thu, 01 Jan 1970 00:00:00 GMT'
         1  'Thu, 01 Jan 1970 00:00:00 GMT'
      'server' => 'Jetty/5.1.x (Linux/3.8.0-42-generic amd64 java/1.6.0_31'
   '_msg' => 'OK'
   '_protocol' => 'HTTP/1.1'
   '_rc' => 200
   '_request' => HTTP::Request=HASH(0x4184fc8)
      '_content' => '{"url":"http://.a.b"}'
      '_headers' => HTTP::Headers=HASH(0x4184ce0)
         'accept' => 'application/json'
         'content-type' => 'application/json; charset=utf-8'
         'user-agent' => 'libwww-perl/6.04'
      '_method' => 'POST'
      '_uri' => URI::http=SCALAR(0x413e908)
         -> 'http://localhost:4444/wd/hub/session/66884be3-8aae-4a1d-a8db-96e6e9daace1/url'
      '_uri_canonical' => URI::http=SCALAR(0x413e908)
         -> REUSED_ADDRESS

2.41 response :

0  HTTP::Response=HASH(0x29b8e80)
   '_content' => ''
   '_headers' => HTTP::Headers=HASH(0x29b8da8)
      'cache-control' => 'no-cache'
      'client-date' => 'Wed, 25 Jun 2014 15:39:47 GMT'
      'client-peer' => '127.0.0.1:4444'
      'client-response-num' => 1
      'connection' => 'close'
      'date' => 'Wed, 25 Jun 2014 15:39:47 GMT'
      'expires' => 'Thu, 01 Jan 1970 00:00:00 GMT'
      'server' => 'Jetty/5.1.x (Linux/3.8.0-42-generic amd64 java/1.6.0_31'
   '_msg' => 'No Content'
   '_protocol' => 'HTTP/1.1'
   '_rc' => 204
   '_request' => HTTP::Request=HASH(0x29b4848)
      '_content' => '{"url":"http://.a.b"}'
      '_headers' => HTTP::Headers=HASH(0x29b43b0)
         'accept' => 'application/json'
         'content-type' => 'application/json; charset=utf-8'
         'user-agent' => 'libwww-perl/6.04'
      '_method' => 'POST'
      '_uri' => URI::http=SCALAR(0x296ded8)
         -> 'http://localhost:4444/wd/hub/session/2ac50f3d-125e-40de-b450-405e61474096/url'
      '_uri_canonical' => URI::http=SCALAR(0x296ded8)
         -> REUSED_ADDRESS

So the main change is that they used to return '204 No Content' on older versions, which was caught correctly by our code (_process_response in Selenium::Remote::Connection).
Now they are returning '200' with a json encoded hash on 2.42.X. We try to make sense of that hash by getting 'value' from that hash, which is null. Then stuff happens.
One possible fix would be that, on line 135 of Selenium::Remote::RemoteConnection, we could replace the line with :

  $data->{'cmd_return'} = $decoded_json->{'value'} // 'No Content';

That's for the quick and dirty solution, but it should work.
We should be careful though if there are people relying on cmd_return for anything, it could break their stuff, hence generating other bugs.

[peroumal1]

@gempesaw I just noticed that I actually did not answer precisely enough.
In our case here, there is no workaround possible in DoesTesting, since the problem is more on how we process the responses from Selenium Server : from 2.41 to 2.42, on the same request, we use two different paths in _process_response sub, so my correction to patch the new path to make it behave as before seems wiser. What do you think ?

[gempesaw]

This seems plenty reasonable, and it should keep the behavior consistent for <2.42 versions with that //, I believe. Thanks a bunch.

[gempesaw]

@peroumal1 Thanks so much for the investigation and the insightful PR, _process_response does seem to be exactly the place we want to be handling this issue. But, I was thinking that we could stick to the current method of handling successful 'no content' responses with that string and the response code as the return value:

53c971f

            if (defined $decoded_json && $decoded_json->{'value'}) {
                $data->{'cmd_return'} = $decoded_json->{'value'};
            }

This also would fit nicely #136 since the return value for those subs is set based off of that response code string.