Closed Bug 689316 Opened 8 years ago Closed 8 years ago

DeviceManagerADB needs a getCurrentTime function

Categories

(Testing :: General, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla10

People

(Reporter: wlach, Assigned: wlach)

Details

(Whiteboard: [inbound])

Attachments

(2 files, 5 obsolete files)

This is needed for remote talos to work correctly when running the ts:gfx (and possibly other things as well)
This is the same as the m-c, except it should work for talos too
Attachment #562548 - Flags: review?(jmaher)
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.
Attachment #562545 - Flags: review?(jmaher) → review-
So if we changed "return float(timestr)*1000" to "return int(timestr)*1000", we'd be good?
Ok, the patch had some problems anyway. Fixing...
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
Attachment #562545 - Attachment is obsolete: true
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
Attachment #562548 - Attachment is obsolete: true
Attachment #562548 - Flags: review?(jmaher)
Attachment #562870 - Flags: review?(jmaher)
Attachment #562869 - Flags: review?(jmaher)
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.
Attachment #562869 - Flags: review?(jmaher) → review-
Assignee: nobody → wlachance
Attachment #562869 - Attachment is obsolete: true
Attachment #563508 - Flags: review?(jmaher)
Attachment #562870 - Attachment is obsolete: true
Attachment #562870 - Flags: review?(jmaher)
Attachment #563510 - Flags: review?(jmaher)
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!
Attachment #563508 - Flags: review?(jmaher) → review+
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()
float->int
Attachment #563510 - Attachment is obsolete: true
Attachment #563510 - Flags: review?(jmaher)
Attachment #563521 - Flags: review?(jmaher)
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.
Attachment #563521 - Flags: review?(jmaher) → review+
Cool, noted. Can you land these for me?
https://hg.mozilla.org/mozilla-central/rev/e87bbd84ba87
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.