Closed Bug 949543 Opened 6 years ago Closed 2 years ago

Use seconds for defining time in Marionette Python client

Categories

(Testing :: Marionette, defect, P4)

x86_64
Linux
defect

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: ato, Assigned: automatedtester)

References

Details

(Keywords: pi-marionette-client)

Attachments

(1 file)

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.
Whiteboard: [good first bug][lang=py][mentor=ato]
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
Hi, I wish to fix this bug. Please guide me through the process.
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!
Hi, is anybody working on this task?
(In reply to akruglov from comment #4)
> Hi, is anybody working on this task?

Heya Sasha!  Subhendu might be, from comment 2?
Flags: needinfo?(subho.prp)
Hi, Yes i am working on this bug. Sorry for replying so late.
Flags: needinfo?(subho.prp)
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.
: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.
Specifically you can do `apt-get install autoconf 2.13` on any Debian based distro.
Thanks David and Andreas, i installed all the prerequisites and now the firefox build is running fine.
Assignee: nobody → subho.prp
Mentor: kazus
Whiteboard: [good first bug][lang=py][mentor=ato] → [good first bug][lang=py]
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.
Is this bug still open? or patch has been accepted?
Resetting mentor to ato, needinfo'ing ato to take a look at the patch.
Mentor: kazus → ato
Flags: needinfo?(ato)
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.
Flags: needinfo?(ato)
Removing as good first bug and will start work on this.
Assignee: subho.prp → dburns
Whiteboard: [good first bug][lang=py]
should we keep :ato as the mentor here?
If :automatedtester is picking it up, then no.
Mentor: ato
closing this as only internal users would use it and the quirk has been around for some time
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → WONTFIX
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.
Resolution: WONTFIX → WORKSFORME
You need to log in before you can comment on or make changes to this bug.