Andreas Tolfsen ❲:ato❳ Reporter
	Description • 11 years ago

Using seconds to define time durations in Python is considered good
practice in the standard library.  Honouring this in the Marionette
Python client will reduce the chance of conversion errors.

As :davehunt points out changing this behaviour in Marionette will
need to be carefully managed, or consumers will be waiting a thousand
times longer than they'd expect!

Bug 850881 also relies heavily on the use of the time library, for
which convering input and output to the class would be a real concern.

	David Burns :automatedtester Assignee
	Comment 1 • 10 years ago

http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#770-797 will need updating as well as http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_implicit_waits.py and http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_timeouts.py

This will need the following try pushes 

try: -b do -p linux,macosx64,win32,linux_gecko,linux64_gecko -u marionette,marionette-webapi,gaia-ui-test -t none
try: -b do -p emulator,panda -u marionette-webapi -t none

	Subhendu Ghosh
	Comment 2 • 10 years ago

Hi, I wish to fix this bug. Please guide me through the process.

	David Burns :automatedtester Assignee
	Comment 3 • 10 years ago

Hi Subhendu,

Follow the steps on https://developer.mozilla.org/en-US/docs/Simple_Firefox_build to get firefox built.

Make the change as mentioned in comment 1. We need to be explicit with our imports and then follow 

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Running_Tests for running the tests.

Hope that helps!

	akruglov
	Comment 4 • 10 years ago

Hi, is anybody working on this task?

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 5 • 10 years ago

(In reply to akruglov from comment #4)
> Hi, is anybody working on this task?

Heya Sasha!  Subhendu might be, from comment 2?

	Subhendu Ghosh
	Comment 6 • 10 years ago

Hi, Yes i am working on this bug. Sorry for replying so late.

	Subhendu Ghosh
	Comment 7 • 10 years ago

hi, i get this error while building simple_firefox_build. How do i resolve it?

[subho@localhost mozilla-central]$ ./mach build
 0:00.33 /usr/bin/gmake -f client.mk -s
 0:00.43 Adding client.mk options from /home/subho/work/mozilla/mozilla-central/mozconfig:
 0:00.43     FOUND_MOZCONFIG := /home/subho/work/mozilla/mozilla-central/mozconfig
 0:00.45 /home/subho/work/mozilla/mozilla-central/client.mk:315: *** Could not find autoconf 2.13.  Stop.
 0:00.45 gmake: *** [build] Error 2
 0:00.49 0 compiler warnings present.

	David Burns :automatedtester Assignee
	Comment 8 • 10 years ago

:Subhendu 

You are missing the prerequisites for building Firefox. https://developer.mozilla.org/en-US/docs/Simple_Firefox_build has what you will need for your OS.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 9 • 10 years ago

Specifically you can do `apt-get install autoconf 2.13` on any Debian based distro.

	Subhendu Ghosh
	Comment 10 • 10 years ago

Thanks David and Andreas, i installed all the prerequisites and now the firefox build is running fine.

	Luv Agarwal(:lagarwal)
	Comment 12 • 10 years ago

In the above patch i have made changes for the unit of function arguments from ms to seconds. Also i have changed all files affected (i think) by the unit changes. Now in marionette.py there is only one function which takes arguments in ms and i.e flick in class Action.

Was this i supposed to do ?. Now if it is going right then i can change units in flick function also and test it with try server with the pushes David gave in comment 1.

	Subhendu Ghosh
	Comment 13 • 10 years ago

Is this bug still open? or patch has been accepted?

	William Lachance (:wlach)
	Comment 14 • 10 years ago

Resetting mentor to ato, needinfo'ing ato to take a look at the patch.

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 15 • 10 years ago

I did a quick review of the current internal users of the public API timeout methods and arguments.  I also looked at internal references to timeouts and conversions, and came up with this non-exhaustive list of things which are not addressed by the attached patch:

- timeout argument to Marionette class constructor
- socket_timeout argument to  Marionette class constructor
- Marionette.execute_js_script's script_timeout and inactivity_timeout arguments
- Marionette.execute_script's script_timeout argument
- Marionette.execute_async_script's script_timeout argument
- Uses in test_simpletest_sanity.py, test_log.py, test_navigation.py, test_execute_isolate.py, test_execute_async_script.py, test_emulator.py, runner/base.py, marionette_test.py, and b2g_update_test.py

Considering the number of things that needs to change, not only for Marionette directly but for the programs depending on it, I have reservations about proceeding with this bug.

The Selenium WebDriver Python client already uses seconds which are converted to milliseconds and using that against Marionette should not be a problem.

	David Burns :automatedtester Assignee
	Comment 16 • 10 years ago

Removing as good first bug and will start work on this.

	Joel Maher ( :jmaher ) (UTC -8)
	Comment 17 • 10 years ago

should we keep :ato as the mentor here?

	Andreas Tolfsen ❲:ato❳ Reporter
	Comment 18 • 10 years ago

If :automatedtester is picking it up, then no.

	David Burns :automatedtester Assignee
	Comment 19 • 7 years ago

closing this as only internal users would use it and the quirk has been around for some time

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 20 • 7 years ago

Well, for quite a while it works great from the Marionette Python client to set timeouts via seconds. The conversion to milliseconds will happen internally in `timeout.py`. So reading the originally filed request this should be a WFM.