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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: jmaher, Assigned: jmaher)
References
Details
Attachments
(3 files, 1 obsolete file)
12.23 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
6.58 KB,
patch
|
mcote
:
review+
|
Details | Diff | Splinter Review |
6.63 KB,
patch
|
mcote
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•13 years ago
|
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 2•13 years ago
|
||
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+
Assignee | ||
Comment 3•13 years ago
|
||
updated patch with feedback addressed!
Assignee: nobody → jmaher
Attachment #512871 -
Attachment is obsolete: true
Attachment #513011 -
Flags: review+
Assignee | ||
Comment 4•13 years ago
|
||
pushed to m-c: http://hg.mozilla.org/automation/remote-testing/pushloghtml?changeset=7cc5c8d8fd6f
Assignee | ||
Comment 5•13 years ago
|
||
just a quick sanity check
Attachment #513014 -
Flags: review?(mcote)
Comment 6•13 years ago
|
||
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+
Assignee | ||
Comment 8•13 years ago
|
||
talos patch landed: http://hg.mozilla.org/build/talos/pushloghtml?changeset=86f925f5a500
Assignee | ||
Comment 9•13 years ago
|
||
get all these changes on m-c.
Attachment #514025 -
Flags: review?(mcote)
Updated•13 years ago
|
Attachment #514025 -
Flags: review?(mcote) → review+
Assignee | ||
Comment 10•13 years ago
|
||
talos part landed: http://hg.mozilla.org/build/talos/pushloghtml?changeset=519c00152d54
Assignee | ||
Comment 12•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=c84daebe92d2
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: New Frameworks → General
You need to log in
before you can comment on or make changes to this bug.
Description
•