Merge pull request #220 from gempesaw/coerce-timeouts

lib/Selenium/Remote/Driver.pm

@@ -968,9 +968,11 @@ sub get_capabilities {
 
 sub set_timeout {
     my ( $self, $type, $ms ) = @_;
-    if ( not defined $type or not defined $ms ) {
-        croak "Expecting type & timeout in ms";
+    if ( not defined $type  ) {
+        croak "Expecting type";
     }
+    $ms = _coerce_timeout_ms( $ms );
+
     my $res = { 'command' => 'setTimeout' };
     my $params = { 'type' => $type, 'ms' => $ms };
     return $self->_execute_command( $res, $params );
@@ -994,9 +996,8 @@ sub set_timeout {
 
 sub set_async_script_timeout {
     my ( $self, $ms ) = @_;
-    if ( not defined $ms ) {
-        croak "Expecting timeout in ms";
-    }
+    $ms = _coerce_timeout_ms( $ms );
+
     my $res    = { 'command' => 'setAsyncScriptTimeout' };
     my $params = { 'ms'      => $ms };
     return $self->_execute_command( $res, $params );
@@ -1026,6 +1027,8 @@ sub set_async_script_timeout {
 
 sub set_implicit_wait_timeout {
     my ( $self, $ms ) = @_;
+    $ms = _coerce_timeout_ms( $ms );
+
     my $res    = { 'command' => 'setImplicitWaitTimeout' };
     my $params = { 'ms'      => $ms };
     return $self->_execute_command( $res, $params );
@@ -2748,6 +2751,28 @@ sub delete_local_storage_item {
     return $self->_execute_command($res, $params);
 }
 
+sub _coerce_timeout_ms {
+    my ($ms) = @_;
+
+    if ( defined $ms ) {
+        return _coerce_number( $ms );
+    }
+    else {
+        croak 'Expecting a timeout in ms';
+    }
+}
+
+sub _coerce_number {
+    my ($maybe_number) = @_;
+
+    if ( Scalar::Util::looks_like_number( $maybe_number )) {
+        return $maybe_number + 0;
+    }
+    else {
+        croak "Expecting a number, not: $maybe_number";
+    }
+}
+
 
 1;
 

t/01-driver.t

@@ -195,10 +195,22 @@ WINDOW: {
 
     $ret = $driver->get_page_source();
     ok($ret =~ m/^<html/i, 'Received page source');
-    eval {$driver->set_implicit_wait_timeout(20001);};
-    ok(!$@,"Set implicit wait timeout");
+
+    # Using a string instead of an int exercises the
+    # _coerce_timeout_ms functionality
+    eval {$driver->set_implicit_wait_timeout("20001");};
+    ok(!$@,"Set implicit wait timeout with string");
     eval {$driver->set_implicit_wait_timeout(0);};
-    ok(!$@,"Reset implicit wait timeout");
+    ok(!$@,"Reset implicit wait timeout with integer");
+
+    my $invalid_ms = 'invalid timeout ms';
+    ok( exception{ $driver->set_timeout('script', $invalid_ms) },
+        'Coerce ms arguments for set_timeout' );
+    ok( exception{ $driver->set_async_script_timeout( $invalid_ms ) },
+        'Coerce ms arguments for set_async_script_timeout' );
+    ok( exception{ $driver->set_implicit_wait_timeout( $invalid_ms ) },
+        'Coerce ms arguments for set_implicit_wait_timeout' );
+
     $ret = $driver->get("$website/frameset.html");
     $ret = $driver->switch_to_frame('second');
 
@@ -554,4 +566,22 @@ QUIT: {
     ok((not defined $driver->{'session_id'}), 'Killed the remote session');
 }
 
+COERCION: {
+    my $string = 'string';
+    like( exception { Selenium::Remote::Driver::_coerce_number( $string ) },
+          qr/Expecting a number/,
+          'Can coerce numbers'
+      );
+
+    like( exception { Selenium::Remote::Driver::_coerce_timeout_ms() },
+          qr/Expecting a timeout/,
+          'Can coerce missing timeouts'
+      );
+
+    like( exception { Selenium::Remote::Driver::_coerce_timeout_ms( $string ) },
+          qr/Expecting a number/,
+          'Can coerce non-numeric timeouts'
+      );
+}
+
 done_testing;