Last Comment Bug 827403 - Implement 'timeouts' command
: Implement 'timeouts' command
Status: RESOLVED FIXED
:
Product: Testing
Classification: Components
Component: Marionette (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla21
Assigned To: Ann(Yiming)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-07 09:37 PST by Malini Das [:mdas] - Away, not checking bugmail
Modified: 2013-01-20 07:19 PST (History)
1 user (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Adding timeout functionality (11.44 KB, text/plain)
2013-01-09 15:41 PST, Ann(Yiming)
no flags Details
Adding timeout functionality (11.44 KB, patch)
2013-01-09 15:55 PST, Ann(Yiming)
no flags Details | Diff | Splinter Review
timeout functionality update (14.76 KB, patch)
2013-01-10 11:05 PST, Ann(Yiming)
malini: review-
Details | Diff | Splinter Review
timeout functionality error/format fix (12.76 KB, patch)
2013-01-11 11:39 PST, Ann(Yiming)
malini: review+
Details | Diff | Splinter Review

Description Malini Das [:mdas] - Away, not checking bugmail 2013-01-07 09:37:21 PST
As described here: http://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#timeouts-1 and http://code.google.com/p/selenium/wiki/JsonWireProtocol#/session/:sessionId/timeouts, we need a command called 'timeouts' that takes in 'type' and 'ms' arguments so we can set either the script, pageload or implicit wait timeouts in one command.

We already have support for setting the timeout of script and implicit wait timeouts in the server, so now we just need to add support for page load and use the 'timeouts' function to call either of these methods.

I'm setting this bug aside as a ramping up project.
Comment 1 Ann(Yiming) 2013-01-09 15:41:42 PST
Created attachment 700111 [details]
Adding timeout functionality
Comment 2 Ann(Yiming) 2013-01-09 15:55:23 PST
Created attachment 700115 [details] [diff] [review]
Adding timeout functionality
Comment 3 Ann(Yiming) 2013-01-10 11:05:19 PST
Created attachment 700552 [details] [diff] [review]
timeout functionality update
Comment 4 Malini Das [:mdas] - Away, not checking bugmail 2013-01-10 17:18:20 PST
Comment on attachment 700552 [details] [diff] [review]
timeout functionality update

Review of attachment 700552 [details] [diff] [review]:
-----------------------------------------------------------------

Great patch.

I think you just need to undo the change you did to test_findelement.py, and fix up some naming/whitespace issues, and then I'll be ready to give you an r+ :) You just need to upload the new patch and r? me again.

::: testing/marionette/client/marionette/marionette.py
@@ +388,5 @@
>  
>      def navigate(self, url):
>          response = self._send_message('goUrl', 'ok', value=url)
>          return response
> +    

There's some extra whitespace here (4 spaces). Can you remove it?

::: testing/marionette/client/marionette/tests/unit/test_findelement.py
@@ +116,5 @@
>      def test_not_found(self):
>          test_html = self.marionette.absolute_url("test.html")
>          self.marionette.navigate(test_html)
>          self.marionette.set_search_timeout(1000)
> +        self.assertRaises(NoSuchElementException, self.marionette.findeseelement, "id", "I'm not on the page")

there's a typo here, "self.marionette.findeseelement". Are you able to run this test without errors? 

I don't think you want to change this file, either. I'd revert it and resubmit the patch.

::: testing/marionette/client/marionette/tests/unit/test_timeouts.py
@@ +6,5 @@
> +from marionette_test import MarionetteTestCase
> +from marionette import HTMLElement
> +from errors import NoSuchElementException
> +from errors import MarionetteException
> +from errors import JavascriptException, MarionetteException, ScriptTimeoutException

You can merge lines 8-10, so you can import everything in the same line.

@@ +30,5 @@
> +        self.marionette.timeouts("implicit", 1000)
> +        self.assertRaises(NoSuchElementException, self.marionette.find_element, "id", "I'm not on the page")
> +        self.marionette.timeouts("implicit", 0)
> +        self.assertRaises(NoSuchElementException, self.marionette.find_element, "id", "I'm not on the page") 
> +    

Trailing whitespace at the end of line 33 and the beginning of line 34

@@ +37,5 @@
> +        self.marionette.navigate(test_html)
> +        self.marionette.timeouts("implicit", 4000)
> +        self.assertEqual(HTMLElement, type(self.marionette.find_element("id", "newDiv")))
> +
> + 

Trailing whitespace

@@ +42,5 @@
> +    def test_searchtimeout_found(self):
> +        test_html = self.marionette.absolute_url("test.html")
> +        self.marionette.navigate(test_html)
> +        self.assertRaises(NoSuchElementException, self.marionette.find_element, "id", "newDiv")
> +    

Trailing whitespace

::: testing/marionette/marionette-actors.js
@@ +953,5 @@
>      function checkLoad() { 
> +       end = new Date().getTime();
> +       let elapse = end - start;
> +       if (this.pageTimeout == null || elapse <= this.pageTimeout){
> +          if (curWindow.document.readyState == "complete") { 

Trailing whitespace

@@ +957,5 @@
> +          if (curWindow.document.readyState == "complete") { 
> +             sendOk(command_id);
> +             return;
> +          }
> +          else{ 

Trailing whitespace

@@ +1235,5 @@
> +    if (isNaN(timeout)){
> +      this.sendError("Not a Number", 500, null, this.command_id);
> +    }
> +    else{
> +       if (timeout_type == "implicit"){   

There's extra whitespace trailing the {

@@ +1238,5 @@
> +    else{
> +       if (timeout_type == "implicit"){   
> +          aRequest.value = aRequest.ms;
> +          this.setSearchTimeout(aRequest);
> +          

Some more extra whitespace

@@ +1247,5 @@
> +       }
> +       else{
> +          this.pageTimeout = timeout;
> +          this.sendOk(this.command_id);
> +       } 

More whitespace trailing the }

::: testing/marionette/marionette-listener.js
@@ +553,5 @@
> +     }
> +     if (!event.originalTarget.defaultView.frameElement || 
> +          event.originalTarget.defaultView.frameElement == curWindow.frameElement) {
> +        removeEventListener("DOMContentLoaded", onDOMContentLoaded, false);
> +       

More extra whitespace

@@ +565,3 @@
>        }
> +  };
> +  function Timerfunc(){

Timerfunc should be timerFunc. We reserve capitalizing the first letter for "class" type objects.
Comment 5 Ann(Yiming) 2013-01-11 11:39:13 PST
Created attachment 701219 [details] [diff] [review]
timeout functionality error/format fix
Comment 6 Malini Das [:mdas] - Away, not checking bugmail 2013-01-11 11:44:53 PST
Comment on attachment 701219 [details] [diff] [review]
timeout functionality error/format fix

Review of attachment 701219 [details] [diff] [review]:
-----------------------------------------------------------------

Great! I'll land this on m-i for you.
Comment 7 Malini Das [:mdas] - Away, not checking bugmail 2013-01-11 11:46:29 PST
landed on m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef384134776b
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-01-12 12:49:50 PST
https://hg.mozilla.org/mozilla-central/rev/ef384134776b
Comment 9 Malini Das [:mdas] - Away, not checking bugmail 2013-01-20 07:19:45 PST
Comment on attachment 700115 [details] [diff] [review]
Adding timeout functionality

Review of attachment 700115 [details] [diff] [review]:
-----------------------------------------------------------------

Reviewed a following patch.

Note You need to log in before you can comment on or make changes to this bug.