Closed Bug 931781 Opened 11 years ago Closed 11 years ago

Use timeout arg on the command line to change timeout for desktop/device

Categories

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(firefox28 fixed, b2g-v1.2 fixed)

RESOLVED FIXED
Tracking Status
firefox28 --- fixed
b2g-v1.2 --- fixed

People

(Reporter: zcampbell, Assigned: bitgeeky)

Details

(Whiteboard: [mentor=davehunt][lang=py])

Attachments

(1 file, 10 obsolete files)

376 bytes, text/html
davehunt
: review+
Details
We'd like to be able to use a timeout function to set the `_default_timeout` from the command line.

Tasks:
* Update gaia_test.py where required
* Retain default/current timeout for device tests
* Update Travis command line to use 10 second timeout and see how the test run goes
Whiteboard: [mentor=davehunt][lang=py]
To get started you will need to clone the gaia repository at https://github.com/mozilla-b2g/gaia and install the Python package. I recommend using virtual environments for this, which you can install by running 'pip install virtualenv'.

Then change to the gaia-ui-tests directory, create and activate a virtual environment, and install gaiatest:
cd tests/python/gaia-ui-tests/
virtualenv venv
source venv/bin/activate
python setup.py develop

The gaiatest harness currently sets a search and script timeout here: https://github.com/mozilla-b2g/gaia/blob/c8e1eb8b06bc2e3e02fcf57e206f07b1be02ecc4/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L440 but we want to be able to control these via the command line --timeout argument.

The --timeout argument is already functioning, so we just need to remove anywhere we override the timeouts, and allow for suitable defaults. Here are a couple of examples of the changes we'd need:

In https://github.com/mozilla-b2g/gaia/blob/c8e1eb8b06bc2e3e02fcf57e206f07b1be02ecc4/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L117 we currently specify a default timeout of 30 seconds. If we set the default value to None, we can then use something like `timeout = timeout or self.marionette.timeout or 30` we can use the passed value, falling back on the --timeout command line value if present, and finally falling back to a default value of 30 seconds.

There are a number of places where we currently set timeouts, and all of these will need reviewing for this patch. In most cases we will want to use the value from self.marionette.timeout in favour of a hard-coded value.

If you need any further help with this please ask here, or in IRC.
Assignee: nobody → mozpankaj1994
Thanks Dave , after working on it , I modified it as https://gist.github.com/bitgeeky/7255081 
I have put #changed on every line that I have changed 
Hope this was what you wanted , please let me know if its correct.
Attached patch moz.py (obsolete) — Splinter Review
Attachment #825635 - Flags: feedback?(dave.hunt)
Attached patch 931781.patch (obsolete) — Splinter Review
Attachment #825635 - Attachment is obsolete: true
Attachment #825635 - Flags: feedback?(dave.hunt)
Attachment #825660 - Flags: feedback?(dave.hunt)
Comment on attachment 825660 [details] [diff] [review]
931781.patch

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

Wow, thanks for coming up with a patch so quickly, this is a great start! I've added some comments, and I think you missed the timeouts from apps/base.py. Looking forward to seeing the next version!

::: tests/python/gaia-ui-tests/gaiatest/gaia_test.py
@@ +115,5 @@
>      def runningApps(self):
>          return self.marionette.execute_script("return GaiaApps.getRunningApps()")
>  
> +    def switch_to_frame(self, app_frame, url=None, timeout=None): 
> +	timeout = timeout or self.marionette.timeout or 30       

Thinking about this, marionette.timeout is in milliseconds, so we will want to divide it by 1000 if it's set. Also, please take care not to add white space to the end of lines.

@@ -137,4 @@
>          self.testvars = testvars or {}
>          js = os.path.abspath(os.path.join(__file__, os.path.pardir, 'atoms', "gaia_data_layer.js"))
>          self.marionette.import_script(js)
> -        self.marionette.set_search_timeout(10000)

In this case we should be safe to remove this line. We'll be relying on Marionette's default of 10 seconds or use the value set by the --timeout argument.

@@ +162,5 @@
>          result = self.marionette.execute_async_script('return GaiaDataLayer.insertContact(%s);' % json.dumps(contact), special_powers=True)
>          assert result, 'Unable to insert contact %s' % contact
>  
> +    def remove_all_contacts(self, default_script_timeout=None): 
> +	default_script_timeout=default_script_timeout or self.marionette.default_script_timeout or 60000 

I would remove this argument, and define timeout directly as:
    timeout = max(self.marionette.timeout or 60000, 1000 * len(self.all_contacts))

@@ +167,2 @@
>          self.marionette.switch_to_frame()
>          self.marionette.set_script_timeout(max(default_script_timeout, 1000 * len(self.all_contacts)))

Let's remove this set_script_timeout as it means we have to then set it back. Instead we can set the timeout for just this call, by adding timeout=timeout as an argument for the execute_async_script call below.

@@ +416,4 @@
>          self.marionette.wait_for_port()
>          self.marionette.start_session()
>          if self.is_android_build:
> +	    script_timeout= self.marionette.script_timeout or 60000 

In this case, let's keep the hard-coded timeout. We're waiting for B2G to start, and that may take a while.

@@ +445,5 @@
>          self.restart = kwargs.pop('restart', False)
>          kwargs.pop('iterations', None)
>          kwargs.pop('checkpoint_interval', None)
>          MarionetteTestCase.__init__(self, *args, **kwargs)
> +	_default_timeout=marionette._default_timeout

We shouldn't need this now.

@@ +467,5 @@
>          # the emulator can be really slow!
> +	_script_timeout=self.marionette._script_timeout or 60000
> +        self.marionette.set_script_timeout(_script_timeout)
> +	_search_timeout=self.marionette._search_timeout or 10000
> +        self.marionette.set_search_timeout(_search_timeout)

We can remove these lines now and rely on the --timeout arg setting them.

@@ +616,5 @@
>      def screen_orientation(self):
>          return self.marionette.execute_script('return window.screen.mozOrientation')
>  
> +    def wait_for_element_present(self, by, locator, timeout=None): 
> +	timeout= timeout or self.marionette._default_timeout or 30 

You need a space either side of the = character. Also, use self.marionette.timeout (divided by 1000 to get the seconds) if it's set rather than _default_timeout.

@@ +621,1 @@
>          timeout = float(timeout) + time.time()

Rather than redefine timeout, we could rename this variable to end_time like it is in wait_for_condition.
Attachment #825660 - Flags: feedback?(dave.hunt) → feedback+
Attached patch 931781v2.patch (obsolete) — Splinter Review
Changed both gaia_test.py and apps/base.py
Attachment #825660 - Attachment is obsolete: true
Attachment #825931 - Flags: feedback?(dave.hunt)
Comment on attachment 825931 [details] [diff] [review]
931781v2.patch

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

Mostly looks good. Please be careful not to introduce unwanted whitespace. We should be ready for a review soon, at which point you should submit a pull request so we get some results in Travis-CI.

::: tests/python/gaia-ui-tests/gaiatest/apps/base.py
@@ +26,5 @@
>  
> +    def wait_for_element_present(self, by, locator, timeout=None):
> +	if self.marionette.timeout:
> +		marionette_timeout = self.marionette.timeout/1000
> +	timeout = timeout or marionette_timeout or 30

I don't like that we mix seconds with milliseconds, especially without documenting these methods. That said, we shouldn't change this as our tests rely on it. What I would suggest is to simplify this to one line:

    timeout = timeout or (self.marionette.timeout and self.marionette.timeout / 1000) or 60

@@ +102,1 @@
>          end_time = time.time() + timeout

Elsewhere we cast timeout to a float and have the components in the opposite order. Let's stay consistent.

::: tests/python/gaia-ui-tests/gaiatest/gaia_test.py
@@ +68,4 @@
>                                                      (app_name, permission_name, value))
>  
>      def launch(self, name, switch_to_frame=True, url=None, launch_timeout=None):
> +	launch_timeout = launch_timeout or  self.marionette.launch_timeout 

Marionette does not have a 'launch_timeout' attribute. In this case I would just use the passed timeout, so you can remove this change.

@@ +162,4 @@
>          result = self.marionette.execute_async_script('return GaiaDataLayer.insertContact(%s);' % json.dumps(contact), special_powers=True)
>          assert result, 'Unable to insert contact %s' % contact
>  
> +    def remove_all_contacts(self): 

Remove the trailing whitespace.

@@ +166,2 @@
>          self.marionette.switch_to_frame()
> +        self.marionette.set_script_timeout(max(self.marionette.timeout or 60000 , 1000 * len(self.all_contacts)))

We shouldn't use set_script_timeout anymore, we should define the value for the timeout, and then pass it was an argument for the execute_async_script call on the next line. For example:

    timeout = max(self.marionette.timeout or 60000, 1000 * len(self.all_contacts))
    result = self.marionette.execute_async_script('return GaiaDataLayer.removeAllContacts();', special_powers=True, script_timeout=timeout)

@@ +170,3 @@
>  
>      def get_setting(self, name):
> +        return self.marionette.execute_async_script('return GaiaDataLayer.getSetting("%s")' % name, special_powers=True ,timeout = timeout)

This change is not needed as the default timeout will be used.

@@ +420,4 @@
>      window.removeEventListener('mozbrowserloadend', loaded);
>      marionetteScriptFinished();
>    }
> +});""", 60000) 

There should be no change here.

@@ +461,3 @@
>              self.device.start_b2g()
>  
>          # the emulator can be really slow!

We can remove this comment too, as it was related to the timeout lines.

@@ +608,4 @@
>      def screen_orientation(self):
>          return self.marionette.execute_script('return window.screen.mozOrientation')
>  
> +    def wait_for_element_present(self, by, locator, timeout=None): 

Remove trailing whitespace.
Attachment #825931 - Flags: feedback?(dave.hunt) → feedback+
Attached patch 931781.patch (obsolete) — Splinter Review
Removed the trailing white spaces , fixed the problem with spaces and tabs and also modified the files as mentioned in comment.
Attachment #825931 - Attachment is obsolete: true
Attachment #828636 - Flags: feedback?(dave.hunt)
Pointer to Github pull-request
Attachment #828771 - Flags: review?(dave.hunt)
Comment on attachment 828774 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478#attch-to-bugzilla

Pointer to Pull Request : https://github.com/mozilla-b2g/gaia/pull/13478
Attachment #828774 - Flags: review?(dave.hunt)
Attachment #828774 - Attachment is obsolete: true
Attachment #828774 - Flags: review?(dave.hunt)
Attachment #828636 - Attachment is obsolete: true
Attachment #828636 - Flags: feedback?(dave.hunt)
Comment on attachment 828771 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478

This is really close now! Just a few comments added to the pull request. I also think it's worth getting Zac's feedback on your next patch version.
Attachment #828771 - Flags: review?(dave.hunt) → review-
Status: NEW → ASSIGNED
Pointer to Github pull-request
Comment on attachment 830155 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478

:zac i made the changes as mentioned on the pull request , squashed and rebased the commits , please have a look at the pull request.
Attachment #830155 - Flags: feedback?(zcampbell)
Comment on attachment 830155 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478

Issue with default timeout/implicit wait, see comments in pull.

I'd also like to see some basic documentation in the readme.md. Just declare that the command line exists and recommend to use seconds or microseconds.

Re-mark the f? when it's done, I am a fan of this pull :)
Attachment #830155 - Flags: feedback?(zcampbell) → feedback-
I agree with Zac that we should mention the timeout in the README, but it's not good that we should expect failures if no timeout is specified. Zac: Would you object to setting the search timeout to 10 seconds if none has been set on the command line? We would do this just once per session in setUp.

Other that that, we should add a --timeout=10000 to the Travis script here: https://github.com/mozilla-b2g/gaia/blob/a04340f5d52ef95008ae0199b770ebd773bdcc96/tests/travis_ci/gaia_ui_tests/script#L14 and add a higher timeout for device testing in Jenkins, but that would be separate from this patch.
Flags: needinfo?(zcampbell)
+1, +1 and +1!

10 second default I think should be reasonably even for device.

I am comfortable because I think in most cases where we need a larger timeout we have overridden the default in the test/page object itself (eg waiting for a SMS).

Agree let's do the script updates separately.
Flags: needinfo?(zcampbell)
Attached patch 931781v4.patch (obsolete) — Splinter Review
Please have a look at this patch and suggest changes if any
Attachment #831560 - Flags: feedback?(dave.hunt)
Flags: needinfo?(zcampbell)
Attachment #828771 - Attachment is obsolete: true
Attachment #830155 - Attachment is obsolete: true
Comment on attachment 831560 [details] [diff] [review]
931781v4.patch

Please don't attach a new patch, but add this to the open pull request.

+    --timeout < time in milliseconds >  to set the default timeout 

We should clarify that this sets the page load timeout, the search timeout, and the script timeout. We should also mention the the defaults if this is not specified is 30s for page load, 10s for search, and 10s for script. 

+    def setUp(self, timeout=None):
+        _search_timeout = timeout or self.marionette.timeout or 10000
+        self.marionette.set_search_timeout(_search_timeout)

Have you tested this? I think this is before we have a Marionette session so suspect this would fail. Let's restore the line at https://github.com/mozilla-b2g/gaia/blob/1b5247170863865327ea31480bec5469a747d435/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L491

Also, we shouldn't add a timeout argument to setUp. In fact, we only need to set this if nothing has been specified on the command line. So it should look something like:

if not self.marionette.timeout:
    self.marionette.set_search_timeout(10000)

You also pointed out on IRC that we're forcing the search timeout in is_element_present here: https://github.com/mozilla-b2g/gaia/blob/1b5247170863865327ea31480bec5469a747d435/tests/python/gaia-ui-tests/gaiatest/apps/base.py#L108

We should change this so we set it to self.marionette.timeout if it's set, falling back to 10000. It's not ideal to have this value hard-coded in two places, but I think we'll have to live with it for now.
Attachment #831560 - Flags: feedback?(dave.hunt) → feedback-
Pointer to Github pull-request
Comment on attachment 831859 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478

:davehunt please have a look at the last commit that i pushed , if the pull looks fine to you i may squash and rebase my commits.
Attachment #831859 - Flags: review?(dave.hunt)
Attachment #831560 - Attachment is obsolete: true
Comment on attachment 831859 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478

This looks great! I've triggered an ad-hoc device testrun here: http://qa-selenium.mv.mozilla.com:8080/view/B2G%20Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/42/ (unfortunately you won't be able to see this without VPN access) if the results are comparable to without the patch applied then we should merge. In the meantime, please rebase and squash your commits.
Attachment #831859 - Flags: review?(dave.hunt) → review+
Flags: needinfo?(zcampbell)
Flags: needinfo?(dave.hunt)
Okay, the results are in and it looks good. Most of the failures are known intermittents, but there are a couple of timeouts that we can probably address in this patch.

It appears that we're failing to connect to WiFi within the default timeout, which isn't too surprising at 10 seconds. Let's increase the timeout to 60 seconds or marionette.timeout (whichever is greater).

This would be added here: https://github.com/mozilla-b2g/gaia/blob/61ac5d3bc764e66dd35eb1d43b74282b1b0232c2/tests/python/gaia-ui-tests/gaiatest/gaia_test.py#L269 as a script_timeout argument. I would use max(self.marionette.timeout, 60000) as the timeout value so it can still be overridden on the command line but would be no less than 60 seconds.

Similarly, we should do this for the WiFi UI test, here: https://github.com/mozilla-b2g/gaia/blob/61ac5d3bc764e66dd35eb1d43b74282b1b0232c2/tests/python/gaia-ui-tests/gaiatest/apps/settings/regions/wifi.py#L45 adding a timeout argument (note that this is in seconds, not milliseconds).
Flags: needinfo?(dave.hunt)
Pointer to Github pull-request
Comment on attachment 832309 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478

:davehunt , made the changes please have a look at pull , and suggest changes if any
Attachment #832309 - Flags: review?(dave.hunt)
Hey bitgeeky I just noticed that your pull has merge conflicts.

If you rebase it before Dave reviews it next he'll have an easier time.
Comment on attachment 832309 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478

See comments in pull request. When resubmitting for review you can just change the previous r- to a r? if the location of the patch has not changed. If you create a new attachment please be sure to mark any obsolete ones appropriately.
Attachment #832309 - Flags: review?(dave.hunt) → review-
Attachment #831859 - Attachment is obsolete: true
Attachment #832309 - Attachment is obsolete: true
Comment on attachment 8335251 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478/files

made the change , according to the comment on the pull
Attachment #8335251 - Flags: review?(dave.hunt)
Comment on attachment 8335251 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478/files

See comment in pull request. Please avoid squashing your commits in your next push so I can see your fix without checking the full diff.
Attachment #8335251 - Flags: review?(dave.hunt) → review-
Comment on attachment 8335251 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478/files

Apologies, I couldn't see what was right there! :) Let's run an adhoc job and see how it goes.
Attachment #8335251 - Flags: review- → review+
Flags: needinfo?(dave.hunt)
I noticed this failure:

Traceback (most recent call last):
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.adhoc/.env/local/lib/python2.7/site-packages/marionette_client-0.6.2-py2.7.egg/marionette/marionette_test.py", line 143, in run
testMethod()
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/tests/functional/settings/test_settings_wifi.py", line 26, in test_connect_to_wifi_via_settings_app
wifi_settings.connect_to_network(self.testvars['wifi'])
File "/var/jenkins/workspace/b2g.hamachi.mozilla-central.ui.adhoc/tests/python/gaia-ui-tests/gaiatest/apps/settings/regions/wifi.py", line 46, in connect_to_network
timeout = max(self.marionette.timeout / 1000, 60))
TypeError: unsupported operand type(s) for /: 'NoneType' and 'int'

Here we're trying to divide a None object by 1000. You could fix this by first checking if the timeout is set before converting it to seconds:

timeout = max(self.marionette.timeout and self.marionette.timeout / 1000, 60))
Flags: needinfo?(dave.hunt)
Comment on attachment 8335251 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478/files

Flipping review state due to failure.
Attachment #8335251 - Flags: review+ → review-
Comment on attachment 8335251 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/13478/files

Updated the Pull
Attachment #8335251 - Flags: review- → review?(dave.hunt)
Running another adhoc job: http://qa-selenium.mv.mozilla.com:8080/view/B2G Hamachi/job/b2g.hamachi.mozilla-central.ui.adhoc/51

Zac: when it's done could you look over any failures and comment if any are suspicious? We may need to add a --timeout to Jenkins if 10 seconds is not long enough for our waits.
Flags: needinfo?(zcampbell)
Dave, I've run it again because last week's build was poor. Keeping the ni?
Okay, just keep in mind that the branch we're running against will be out of date with master.
(In reply to Dave Hunt (:davehunt) from comment #36)
> Zac: when it's done could you look over any failures and comment if any are
> suspicious? We may need to add a --timeout to Jenkins if 10 seconds is not
> long enough for our waits.

For all of the failures I don't recognise there appears to be a pattern around wifi failing to connect. I feel like we should push them up to 30 seconds but for UI interactions 10 would be better.
Thus I'm happy to take on this patch with --timeout=30 for Jenkins as you suggest and then tune the wifi timin in the future and reduce the Jenkins back to --timeout=10


One issue I have though, why can't you use a canonical/global variable for what the marionette timeout is in seconds so that we can refer to it from within page objects?
It is calculated every time in gaia_test.py and we'd have to do the same in the page object. Every time it just introduces a chance for it to be messed up or done wrong.
Flags: needinfo?(zcampbell)
(In reply to Zac C (:zac) from comment #39)
> One issue I have though, why can't you use a canonical/global variable for
> what the marionette timeout is in seconds so that we can refer to it from
> within page objects?
> It is calculated every time in gaia_test.py and we'd have to do the same in
> the page object. Every time it just introduces a chance for it to be messed
> up or done wrong.

It's only necessary to convert it from milliseconds because the wait_for methods in gaiatest expect seconds. Ideally we'd just use milliseconds, but changing this now would be rather disruptive. When we have a Wait support class we could perhaps switch to milliseconds and deprecate the wait_for methods.

We could provide a method such as timeout_in_seconds that takes a marionette instance and returns the timeout converted to seconds, however there's still a chance for this not to be called and the marionette.timeout is used directly. Also, such a method would need to be duplicated in the test base and page base classes.
(In reply to Dave Hunt (:davehunt) from comment #40)
> We could provide a method such as timeout_in_seconds that takes a marionette
> instance and returns the timeout converted to seconds, however there's still
> a chance for this not to be called and the marionette.timeout is used
> directly. Also, such a method would need to be duplicated in the test base
> and page base classes.

Couldn't you just do it once in the setUp?

I probably didn't describe the use case very well. Here is an example:
https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/gaiatest/apps/browser/app.py#L50

How would we change that to use the default timeout from the command line in the python method declaration yet be overridden from the test itself when required?
(In reply to Zac C (:zac) from comment #41)
> (In reply to Dave Hunt (:davehunt) from comment #40)
> > We could provide a method such as timeout_in_seconds that takes a marionette
> > instance and returns the timeout converted to seconds, however there's still
> > a chance for this not to be called and the marionette.timeout is used
> > directly. Also, such a method would need to be duplicated in the test base
> > and page base classes.
> 
> Couldn't you just do it once in the setUp?

Where would we store it that would be available to test cases and page objects alike? It's always available from the marionette object, and that's where we're currently getting it from.

> I probably didn't describe the use case very well. Here is an example:
> https://github.com/mozilla-b2g/gaia/blob/master/tests/python/gaia-ui-tests/
> gaiatest/apps/browser/app.py#L50
> 
> How would we change that to use the default timeout from the command line in
> the python method declaration yet be overridden from the test itself when
> required?

That's ultimately calling wait_for_condition, which with this patch will now wait for the duration specified on the command line, or a default of 30 seconds, or the value passed to it - in this case 30 seconds. Doesn't this achieve what you need?
> 
> That's ultimately calling wait_for_condition, which with this patch will now
> wait for the duration specified on the command line, or a default of 30
> seconds, or the value passed to it - in this case 30 seconds. Doesn't this
> achieve what you need?

Ok so you would just pass in None and get the default timeout, gotcha!
Attachment #8335251 - Flags: review?(dave.hunt) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: