Closed
Bug 695274
Opened 13 years ago
Closed 13 years ago
unit test boxes should upload a screen shot after testing, on OSX
Categories
(Testing :: General, enhancement, P5)
Testing
General
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla10
People
(Reporter: jhford, Assigned: jhford)
References
Details
Attachments
(4 files, 3 obsolete files)
2.08 KB,
text/plain
|
Details | |
3.54 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
974 bytes,
patch
|
Details | Diff | Splinter Review | |
3.77 KB,
patch
|
ted
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #589668 +++ +++ This bug was initially created as a clone of Bug #414049 +++ The unit test boxes sometimes have mysterious failures due to focus, which can happen if someone logs in and leaves a console window open or probably other silly things. They should upload a screenshot somewhere (linked in the log file) so we could quickly spot check for that sort of problem. Bug 414049 added data URL screenshots for timeouts with GTK builds. This can be extended to other platforms by adding utility programs to dump the screen to stdout in png format. Bug 414049 references a win32 screenshot utility that could be adapted, as well as some unsuccessful attempts to use screencapture on Mac. Bug 589668 seems to be focused on Win32, so morphing bug to be a Win32 specific bug and filing this bug for the OS X implementation ======= Attachment Notes: I have attached a patch that I think should work. I have tested this function using a small script that substitutes self.log.info with print (and changes the formatters appropriately) as well as using subprocess.Popen directly. I am writing the screenshot to a temporary file, reading that file in, then printing it out to logs after base64 encoding it. I tried to follow the existing structure of the program by conditionally defining dumpScreen for IS_MAC. I didn't include the utiltyPath kwarg in the mac version of dumpScreen because its not needed. I think it might be a good idea to define a single dumpScreen that is a noop for unsupported platforms, with one signature.
Attachment #567688 -
Flags: feedback?(ted.mielczarek)
Assignee | ||
Comment 1•13 years ago
|
||
~ $ python screen.py SCREENSHOT: data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAB4AAAAS<snip>JRU5ErkJggg== ~ $ cat screen.py import tempfile import subprocess import os def dumpScreen(format='png'): haveDumpedScreen = True; utility = '/usr/sbin/screencapture' # Eww! tmpfd, imgfile = tempfile.mkstemp(prefix='mozilla-test-fail_', suffix='.%s'%format) os.close(tmpfd) try: dumper = subprocess.Popen([utility, '-x', '-t', format, imgfile]) except OSError, err: print "Failed to start %s for screenshot: %s" % (screencapture, err.strerror) return status = dumper.wait() if status != 0: print "screenshot utility %s exited with code %d" % (utility, imgfile) return try: imgfile_obj = open(imgfile, 'r') imgdata = imgfile_obj.read() except IOError, err: print "reading %s failed: %s" % (imgfile, err.strerror) import base64 encoded = base64.b64encode(imgdata) print "SCREENSHOT: data:image/png;base64,%s"% encoded if os.path.exists(imgfile): try: os.remove(imgfile) except OSError, err: self.log.info("couldn't remove temporary screenshot file %s: %s", imgfile, err.strerror) dumpScreen()
Assignee | ||
Comment 2•13 years ago
|
||
Also, do all test frameworks use automation.py?
Comment 3•13 years ago
|
||
All Mochitest variants, Reftest and Crashtest use automation.py. xpcshell tests do not, but they do use automationutils.py.
Comment 4•13 years ago
|
||
Comment on attachment 567688 [details] [diff] [review] screencapture v1 Review of attachment 567688 [details] [diff] [review]: ----------------------------------------------------------------- I agree. Can you refactor this so that we have a stub implementation on platforms where we don't support this?
Attachment #567688 -
Flags: feedback?(ted.mielczarek) → feedback-
Updated•13 years ago
|
Assignee: nobody → jhford
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #567688 -
Attachment is obsolete: true
Attachment #567815 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 6•13 years ago
|
||
Comment on attachment 567815 [details] [diff] [review] screencapture v2 silly mouse doing silly things
Attachment #567815 -
Attachment is obsolete: true
Attachment #567815 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 7•13 years ago
|
||
This patch uses a single function for all platforms, using a temp file depending on the platform type. I stayed with a single function because most of the code is common. My concern here is reading the stdout after the wait call. Moving the .wait() to after reading the stdout might be worthwhile. I have a script that I have used to test the linux path, but using "echo image" instead of an actual screen capture. Using IS_MAC I get: SCREENSHOT: data:image/png;base64,iVBORw0KGgoAAAA...AAAAASUVORK5CYII= Using UNIXISH (and 'echo image' instead of screentopng): SCREENSHOT: data:image/png;base64,aW1hZ2UK Using IS_WIN32 I get: If you fixed bug 589668, you'd get a screenshot here I can run this through try before or after review. have preferences? If I had a patch that I knew would trigger a crash and/or timeout, that might make my try run more useful
Attachment #567822 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 8•13 years ago
|
||
This is the logic from v2.1 transplanted into a short script. setting UNIXISH, IS_MAC or IS_WIN32 as desired will emulate each platform self.log.info("msg %s", 'string') -> print "msg %s" % 'string' self.Process -> subprocess.Popen references to self replaced with normal vars
Assignee | ||
Comment 9•13 years ago
|
||
There are lots of oranges on Lion and this should be very helpful in getting them ready to green.
Comment 10•13 years ago
|
||
Comment on attachment 567822 [details] [diff] [review] screencapture v2.1 Review of attachment 567822 [details] [diff] [review]: ----------------------------------------------------------------- r=me with a few nits. ::: build/automation.py.in @@ +755,5 @@ > + if UNIXISH: > + utility = [os.path.join(utilityPath, "screentopng")] > + imgoutput = 'stdout' > + elif IS_MAC: > + utility = ['/usr/sbin/screencapture', '-C', '-x', '-t', 'png'] # Eww! Why the "Eww"? @@ +783,4 @@ > return > > + # Read the captured image in (not sure how well reading a reaped > + # process's stdout will work) Should be fine, stdout is just a pipe here. Realistically, you should probably use dumper.communicate() instead of .wait up above, to avoid filling up the stdout pipe's buffer. @@ +787,5 @@ > + try: > + if imgoutput == 'stdout': > + image = dumper.stdout.read() > + elif imgoutput == 'file': > + imgfile = open(imgfilename) with open(imgfilename) as imgfile: image = imgfile.read() @@ +794,5 @@ > + try: > + os.remove(imgfilename) > + except OSError, err: > + self.log.info("couldn't remove temporary screenshot file %s: %s", > + imgfilename, err.strerror) Is this worth erroring about? Seems low-value.
Attachment #567822 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 11•13 years ago
|
||
(In reply to Ted Mielczarek [:ted, :luser] from comment #10) > Comment on attachment 567822 [details] [diff] [review] [diff] [details] [review] > screencapture v2.1 > > Review of attachment 567822 [details] [diff] [review] [diff] [details] [review]: > ----------------------------------------------------------------- > > r=me with a few nits. > > ::: build/automation.py.in > @@ +755,5 @@ > > + if UNIXISH: > > + utility = [os.path.join(utilityPath, "screentopng")] > > + imgoutput = 'stdout' > > + elif IS_MAC: > > + utility = ['/usr/sbin/screencapture', '-C', '-x', '-t', 'png'] # Eww! > > Why the "Eww"? Just the fact that its a hardcode. Since that's the location in 10.5, 10.6 and 10.7, I don't know if there is value in making that configurable. > @@ +783,4 @@ > > return > > > > + # Read the captured image in (not sure how well reading a reaped > > + # process's stdout will work) > > Should be fine, stdout is just a pipe here. Realistically, you should > probably use dumper.communicate() instead of .wait up above, to avoid > filling up the stdout pipe's buffer. Good call. I have a slightly reworked version of this patch that uses communicate, for both file and stdout. > @@ +787,5 @@ > > + try: > > + if imgoutput == 'stdout': > > + image = dumper.stdout.read() > > + elif imgoutput == 'file': > > + imgfile = open(imgfilename) > > with open(imgfilename) as imgfile: > image = imgfile.read() Excellent! I didn't see the __future__ import of with. > @@ +794,5 @@ > > + try: > > + os.remove(imgfilename) > > + except OSError, err: > > + self.log.info("couldn't remove temporary screenshot file %s: %s", > > + imgfilename, err.strerror) > > Is this worth erroring about? Seems low-value. Hmm, i don't think its that important
Assignee | ||
Comment 12•13 years ago
|
||
I can't seem to ask for checkin-needed, and the changes to the patch are kind of intrusive. I tested using the small script again and both file and stdout worked for me.
Attachment #567822 -
Attachment is obsolete: true
Attachment #568754 -
Flags: review?(ted.mielczarek)
Comment 13•13 years ago
|
||
Comment on attachment 568754 [details] [diff] [review] changeset with nits addressed Review of attachment 568754 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks. It's too bad that screencapture can't write to stdout.
Attachment #568754 -
Flags: review?(ted.mielczarek) → review+
Assignee | ||
Comment 14•13 years ago
|
||
changing product/component so I can get a checkin-needed flag
Assignee: jhford → nobody
Component: General → Build Config
Product: Testing → Core
QA Contact: general → build-config
Assignee | ||
Updated•13 years ago
|
Attachment #568754 -
Flags: checkin?
Comment 15•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b0534b657c
Assignee: nobody → jhford
Component: Build Config → General
Flags: checkin?
Product: Core → Testing
QA Contact: build-config → general
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/a3b0534b657c
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla10
Assignee | ||
Comment 17•13 years ago
|
||
UNIXISH, IS_MAC and IS_WIN32 are class vars for the Automation class. Because they are used in the class scope to optionally define methods, I didn't realize that they weren't globals. INFO | runtests.py | Received unexpected exception while running application 'global name 'UNIXISH' is not defined' diff --git a/build/automation.py.in b/build/automation.py.in --- a/build/automation.py.in +++ b/build/automation.py.in @@ -743,13 +743,13 @@ user_pref("camino.use_system_proxy_setti self.haveDumpedScreen = True; # Need to figure out what tool and whether it write to a file or stdout - if UNIXISH: + if self.UNIXISH: utility = [os.path.join(utilityPath, "screentopng")] imgoutput = 'stdout' - elif IS_MAC: + elif self.IS_MAC: utility = ['/usr/sbin/screencapture', '-C', '-x', '-t', 'png'] imgoutput = 'file' - elif IS_WIN32: + elif self.IS_WIN32: self.log.info("If you fixed bug 589668, you'd get a screenshot here") return
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #571049 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 19•13 years ago
|
||
Comment on attachment 571049 [details] [diff] [review] bustage fix changeset from IRC, <khuey> jhford-work: r=me
Attachment #571049 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 20•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/6f455fb4ad71
Assignee | ||
Comment 21•13 years ago
|
||
dumpScreen should also be defined unconditionally, instead of only on non-IS_WIN32 platforms. Before this patch lands, timeouts and crashes on win32 will have issues caused an error due to a dumpScreen not being defined.
Attachment #571219 -
Flags: review?(ted.mielczarek)
Updated•13 years ago
|
Attachment #571219 -
Flags: review?(ted.mielczarek) → review+
Comment 22•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6f455fb4ad71
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Attachment #571219 -
Attachment is patch: true
Assignee | ||
Comment 23•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/4393eac8ea2d
Assignee | ||
Comment 24•13 years ago
|
||
screenshot on OS X! https://tbpl.mozilla.org/php/getParsedLog.php?id=7165609&tree=Firefox&full=1#error3
Comment 25•13 years ago
|
||
comment 23 merged https://hg.mozilla.org/mozilla-central/rev/4393eac8ea2d
You need to log in
before you can comment on or make changes to this bug.
Description
•