Closed Bug 634691 Opened 13 years ago Closed 13 years ago

update devicemanager to adjust screen resolution and fix unit tests

Categories

(Testing :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jmaher, Assigned: jmaher)

References

Details

Attachments

(3 files, 1 obsolete file)

this adds in adjustResolution() to devicemanager as well as the accompanying unittests.  In addition the test_reboot.py and test_inst.py unit tests have been updated and now all tests are green on my system.

Also this fixes the fire/launch proc issue with "" environment variables.
Attachment #512871 - Flags: review?(ctalbert)
Attachment #512871 - Flags: feedback?(mcote)
Comment on attachment 512871 [details] [diff] [review]
update devicemanager to adjust screen resolution and fix unit tests (1.0)

>+  def adjustResolution(self, width=1680, height=1050, type='hdmi'):
>+    if self.getInfo('os')['os'][0].split()[0] != 'harmony-eng':
>+      if (self.debug >= 2): print "WARNING: unable to adjust screen resolution on non Tegra device"
>+      return False
>+
>+    results = self.getInfo('screen')
>+    parts = results['screen'][0].split(':')
>+    if (self.debug >= 3): print "INFO: we have a current resolution of %s, %s" % (parts[1].split()[0], parts[2].split()[0])
>+
>+    #verify screen type is valid
>+    screentype = -1
>+    if (type == 'hdmi'):
>+      screentype = 5
>+    elif (type == 'vga' or type == 'crt'):
>+      screentype = 3
>+    else:
>+      return False
Let's add a comment about where these magic numbers come from.
>+
>+    #verify we have numbers
>+    try:
>+      w = float(width)
>+      h = float(height)
>+    except:
>+      return False
Python doesn't have some kind of type introspection that doesn't throw?  This seems like a bit of overkill just to determine that we were indeed passed floats, but if that's what you have to do, it's what you have to do.  I looked around a bit and found this:
import operator 
def isFloat(f): 
  if not operator.isNumberType(f): return 0 
  if f % 1: return 1
  else: return 0
But that's about as kludgey as what we've got here.  Maybe Mark has an idea?

I love the unittest for this, very nice.

r+ with the comment, and I'm fine with the isfloat() try/except unless Mark knows a better way.
Attachment #512871 - Flags: review?(ctalbert) → review+
Comment on attachment 512871 [details] [diff] [review]
update devicemanager to adjust screen resolution and fix unit tests (1.0)

+    if not cmd or len(cmd) == 0:

This is redundant in Python... not '' == True.  So 'if not cmd' suffices.

+    #verify we have numbers
+    try:
+      w = float(width)
+      h = float(height)
+    except:
+      return False

First point: why are we verifying that the parameters are specifically floats?  Wouldn't it make more sense to verify that they are integers?

Second: The above is acceptable, though I wouldn't bother assigning to a value (since w & h are throwaways), and I would change the bare except to "except ValueError", which is more indicative of what you're doing.

The other, probably more canonical way of doing it (assuming you really want ints here) is

if not (isinstance(width, int) and isinstance(height, int)):
  return False

+    try:
+      data = self.verifySendCMD(["exec setprop persist.tegra.dpy%s.mode.width %s" % (screentype, width)])
+      data = self.verifySendCMD(["exec setprop persist.tegra.dpy%s.mode.height %s" % (screentype, height)])
+    except(DMError):
+      return False

No point in assigning to 'data' here.

+        if (float(width) == float(w) and float(height) == float(h)):
+            return True
+        return False

Should probably be int() calls.  Actually I would have getResolution() do the type coercion itself and omit the coercion in verifyResolution(), but whatever.

+        """ reset to original state """
+        print "resetting the resolution back to the original %s, %s" % (width, height)
+        self.assertTrue(self.dm.adjustResolution(int(width), int(height), screentype), "unable to set original value for width and height");
+        self.assertTrue(self.verifyResolution(int(width), int(height)), "unable to verify original value for width and height");

If this is actually intended to restore the device to its settings, then this should probably be in a tearDown() method so that it is run even if the test fails at some point in the middle.

Anyway seems generally good!
Attachment #512871 - Flags: feedback?(mcote) → feedback+
updated patch with feedback addressed!
Assignee: nobody → jmaher
Attachment #512871 - Attachment is obsolete: true
Attachment #513011 - Flags: review+
just a quick sanity check
Attachment #513014 - Flags: review?(mcote)
Comment on attachment 513014 [details] [diff] [review]
talos version of the devicemanager changes (1.0)

Looks good.  However did you forget to check in test_resolution.py in your last commit?
Attachment #513014 - Flags: review?(mcote) → review+
get all these changes on m-c.
Attachment #514025 - Flags: review?(mcote)
Attachment #514025 - Flags: review?(mcote) → review+
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=c84daebe92d2
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Component: New Frameworks → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: