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)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
mozilla10

People

(Reporter: jhford, Assigned: jhford)

References

Details

Attachments

(4 files, 3 obsolete files)

Attached patch screencapture v1 (obsolete) — 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)
~ $ 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()
Also, do all test frameworks use automation.py?
All Mochitest variants, Reftest and Crashtest use automation.py. xpcshell tests do not, but they do use automationutils.py.
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-
Assignee: nobody → jhford
Attached patch screencapture v2 (obsolete) — Splinter Review
Attachment #567688 - Attachment is obsolete: true
Attachment #567815 - Flags: review?(ted.mielczarek)
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)
Attached patch screencapture v2.1 (obsolete) — Splinter Review
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)
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
Blocks: 693918
There are lots of oranges on Lion and this should be very helpful in getting them ready to green.
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+
(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
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 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+
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
Attachment #568754 - Flags: checkin?
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
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 → ---
Attachment #571049 - Flags: review?(ted.mielczarek)
Comment on attachment 571049 [details] [diff] [review]
bustage fix changeset

from IRC, <khuey> jhford-work: r=me
Attachment #571049 - Flags: review?(ted.mielczarek)
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)
Attachment #571219 - Flags: review?(ted.mielczarek) → review+
https://hg.mozilla.org/mozilla-central/rev/6f455fb4ad71
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Attachment #571219 - Attachment is patch: true
You need to log in before you can comment on or make changes to this bug.