689316 - DeviceManagerADB needs a getCurrentTime function

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[William Lachance (:wlach)]

This is needed for remote talos to work correctly when running the ts:gfx (and possibly other things as well)

Comment 1

[William Lachance (:wlach)]

Attachment: Patch to add getCurrentTime to devicemanagerADB in mozilla-central

Comment 2

[William Lachance (:wlach)]

Attachment: Patch to add getCurrentTime to devicemanagerADB in talos

This is the same as the m-c, except it should work for talos too

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 562545 [details] [diff] [review]
Patch to add getCurrentTime to devicemanagerADB in mozilla-central

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

::: build/mobile/devicemanagerADB.py
@@ +457,5 @@
> +  def getCurrentTime(self):
> +    timestr = self.runCmd(["shell", "date", "+%s"]).stdout.read()
> +    if (not timestr or not timestr.isdigit()):
> +        raise DMError("Error getting time")
> +    return float(timestr)*1000

in devicemanagerSUT, we return a int, not a float.  This is what we return from devicemanagerSUT:
1317119656911

and I see 'shell date + %s' returning:
1317119669

So the '*1000' is needed, but not adding a .0 at the end.

Comment 4

[William Lachance (:wlach)]

So if we changed "return float(timestr)*1000" to "return int(timestr)*1000", we'd be good?

Comment 5

[William Lachance (:wlach)]

Ok, the patch had some problems anyway. Fixing...

Comment 6

[William Lachance (:wlach)]

Attachment: Take #2 of patch to add currentTime method to deviceManagerADB in m-c

This corrects 3 problems:
* Shouldn't convert to float. Time should be an integer
* Should return as string (as that's what the sut method does and some things depend on that)
* Need to strip out whitespace from what the shell returns

Comment 7

[William Lachance (:wlach)]

Attachment: Take #2 of patch to add currentTime method to deviceManagerADB in talos

This corrects 3 problems:
* Shouldn't convert to float. Time should be an integer
* Should return as string (as that's what the sut method does and some things depend on that)
* Need to strip out whitespace from what the shell returns

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 562869 [details] [diff] [review]
Take #2 of patch to add currentTime method to deviceManagerADB in m-c

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

r- for the raising of an exception.  If this was an internal method I would be fine with the exception.

::: build/mobile/devicemanagerADB.py
@@ +452,5 @@
> +
> +  # external function
> +  # returns:
> +  #  success: time in ms
> +  #  failure: None

This return value doesn't match the failure case below

@@ +456,5 @@
> +  #  failure: None
> +  def getCurrentTime(self):
> +    timestr = self.runCmd(["shell", "date", "+%s"]).stdout.read().strip()
> +    if (not timestr or not timestr.isdigit()):
> +        raise DMError("Error getting time")

I would prefer we return -1 or None here.  It is not guaranteed that all code will catch exceptions and this has bitten us pretty badly on the automation system.

Comment 9

[William Lachance (:wlach)]

Attachment: Take #3 of patch to add currentTime method to deviceManagerADB in m-c

Comment 10

[William Lachance (:wlach)]

Attachment: Take #3 of patch to add currentTime method to deviceManagerADB in talos

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 563508 [details] [diff] [review]
Take #3 of patch to add currentTime method to deviceManagerADB in m-c

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

thanks!

Comment 12

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 563510 [details] [diff] [review]
Take #3 of patch to add currentTime method to deviceManagerADB in talos

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

r=me with the s/float/int/

::: devicemanagerADB.py
@@ +457,5 @@
> +  def getCurrentTime(self):
> +    timestr = self.runCmd(["shell", "date", "+%s"]).stdout.read().strip()
> +    if (not timestr or not timestr.isdigit()):
> +        return None
> +    return str(float(timestr)*1000)

this has a float, not an int()

Comment 13

[William Lachance (:wlach)]

Attachment: Take #4 of patch to add currentTime method to deviceManagerADB in talos

float->int

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 563521 [details] [diff] [review]
Take #4 of patch to add currentTime method to deviceManagerADB in talos

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

in the future if I r+, you can carry over the r+ as long as you have addressed any nits or changes I asked for.

Comment 15

[William Lachance (:wlach)]

Cool, noted. Can you land these for me?

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e87bbd84ba87

Comment 17

[Joel Maher ( :jmaher ) (UTC -8)]

talos:
http://hg.mozilla.org/build/talos/rev/87f57c5b5d7e

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/e87bbd84ba87