Use seconds for defining time in Marionette Python client

RESOLVED WORKSFORME

Status

P4
normal
RESOLVED WORKSFORME
5 years ago
a year ago

People

(Reporter: ato, Assigned: automatedtester)

Tracking

({pi-marionette-client})

Trunk
x86_64
Linux
pi-marionette-client
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 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.
(Assignee)

Updated

5 years ago
Whiteboard: [good first bug][lang=py][mentor=ato]
(Assignee)

Comment 1

5 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

Comment 2

5 years ago
Hi, I wish to fix this bug. Please guide me through the process.
(Assignee)

Comment 3

5 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!
Blocks: 943897

Comment 4

5 years ago
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)

Comment 6

5 years ago
Hi, Yes i am working on this bug. Sorry for replying so late.
Flags: needinfo?(subho.prp)

Comment 7

5 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.
(Assignee)

Comment 8

5 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.
(Reporter)

Comment 9

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

Comment 10

5 years ago
Thanks David and Andreas, i installed all the prerequisites and now the firefox build is running fine.
(Assignee)

Updated

5 years ago
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.

Comment 13

4 years ago
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)
(Reporter)

Comment 15

4 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.
Flags: needinfo?(ato)
(Assignee)

Comment 16

4 years ago
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?
(Reporter)

Comment 18

4 years ago
If :automatedtester is picking it up, then no.
Mentor: ato
(Assignee)

Updated

4 years ago
Keywords: ateam-marionette-client
(Assignee)

Comment 19

a year ago
closing this as only internal users would use it and the quirk has been around for some time
Status: NEW → RESOLVED
Last Resolved: a year 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.