533857 - [report] Add functionality to create screenshots when a test fails

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, enhancement)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It would be helpful to have a screenshot of the current window when a test fails so you can easily see why the problem occurred. We can do that by using the canvas element. I will attach a patch which shows this functionality.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Mozmill test

This test makes use of creating a screenshot of the window which is specified by the controller. Further it will draw a rect over the element which could have been failed for a test.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: screenshot

This is an example screenshot of the above given test.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: screenshot

One more try. This is the real screenshot for sure.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

I've got some in-progress work in my git-hub repo that enables doing this kind of thing:
http://github.com/asutherland/mozmill/commit/1b2b19f711f4b78d311086a0a8d3b45efdd3a6bd

Specifically it works with something like the below.  I definitely am assuming a pure CouchDB target.


function setupModule(module) {
  module.__failure_handlers__.push(screenshot_all_windows_on_error);
}

function screenshot_all_windows_on_error(aModule, aTest, aErrorObj,
                                         aReportingConfig) {
  // if there is no one to listen to our screenshots, don't bother.
  if (aReportingConfig.reporting)
    return;

  // filename => {content_type, data}
  let attachments;
  if (!("_attachments" in aErrorObj))
    attachments = aErrorObj._attachments = {};
  else
    attachments = aErrorObj._attachments;

  let basename = aTest.name + "_";

  //let windowsData = aErrorObj.windows = [];

  let windowMediator = Cc["@mozilla.org/appshell/window-mediator;1"]
                         .getService(Ci.nsIWindowMediator);
  let enumerator = windowMediator.getEnumerator(null);
  let iWin=0;
  while (enumerator.hasMoreElements()) {
    let win = enumerator.getNext();
    let filename = basename + (iWin++) + ".png";

    attachments[filename] = {
      content_type: 'image/png',
      data: screenshotter.screenshotToBase64(win)
    };
  }
}

Comment 6

[Jeff Hammel]

Actually, I'm not sure if bug 634333 is a duplicate.  This bug is about making a screenshot on test failure.  bug 634333 is about moving the functionality to mozmill core.  That bug should probably be reopened and this bug made dependent on it

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It should be made in one step. There is really no point of just copying the module over. Also in the current state we don't really have that expertise how stable and reliable it is.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

As a follow-up just so you know where Thunderbird is, we implemented screenshot-on-failure (plus some other info) by adding an events.fail listener and having it invoke this code here:

http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-window-helpers.js#1416

My previous code using chrome-privileged canvas rendering required recursive processing for the iframes, but it turns out that is not required anymore, at least if you pass a few very useful flags.  It is not clever enough to render live popups, although we do at least emit a list of the active popups in our meta-data.

Comment 9

[Andrew Halberstadt [:ahal]]

Another important use case is localization testing, so the
user should have the ability to arbitrarily take a screenshot, not just on
failure.

(In reply to comment #7)
> It should be made in one step. There is really no point of just copying the
> module over. Also in the current state we don't really have that expertise how
> stable and reliable it is.

Henrik, are you saying that we shouldn't use the implementation in
http://hg.mozilla.org/qa/mozmill-tests/file/1ccaf9638b3b/lib/screenshot.js as
it isn't reliable?

It seems very similar to the Thunderbird implementation in comment 8

Comment 10

[Andrew Halberstadt [:ahal]]

The screengrab! extension also does it in a similar way: http://code.google.com/p/screengrab/source/browse (albeit with more bells and whistles)

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I have only said that our module is not tested very well at this point. So I cannot guarantee for mostly bug free code. But if it looks similar to what you have we can combine all the good parts and strip out the bad ones. :)

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: WIP Patch v1

This is a quick patch that lets you use 'controller.screenShot(element, destFile)'. (thanks :asuth)

At the moment all it does is save the screenshot to the specified file path.

You can either pass in a document to take a screen shot of the whole window, or pass in any arbitrary element to take a screen shot of just its client rect.

We still need to figure out how/where we want to save the screenshots and how to integrate them with reporting or if we should integrate them at all.

I also still want to implement the highlighting functionality found in the qa-automation's screenshot.js.

p.s Andrew, I'm not seeing any issues by omitting those flags you mentioned in the drawWindow method. Then again, I've only done extremely basic testing with this. Was there any case in specific that failed without those flags?

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

(In reply to comment #12)
> We still need to figure out how/where we want to save the screenshots and how
> to integrate them with reporting or if we should integrate them at all.

For Thunderbird's reporting, we emit them as data URL's so we can store them in our meta-data JSON blobs.  When I was using couchdb for reporting, I used the data url mechanism as a fast way to get base64 encoding for couchdb's attachments mechanism.
 
> p.s Andrew, I'm not seeing any issues by omitting those flags you mentioned in
> the drawWindow method. Then again, I've only done extremely basic testing with
> this. Was there any case in specific that failed without those flags?

I presume the drawWindow mechanism just has improved, then.  Thunderbird uses iframes in various spots and they weren't getting drawn without us recursing when I was first playing with this stuff a looong time ago.

Comment 14

[Andrew Halberstadt [:ahal]]

Attachment: WIP Patch v2

Usage:
controller.screenShot(node, name, highlights);

-node: either a window or an arbitrary DOM element. In the latter case only the elements clientRect will be captured.

-name: the name to associate with the screenshot. Used for reporting and file saving.

-highlights: a list of elements that will be highlighted (have a red rectangle drawn around them).

I ran into issues passing the data blob of the screenshot from js to python. Instead what happens is that the screenshot will be saved as a .png in the users tmp directory. A filepath will then be given to the user in their reporting. It can be up to them to move, delete, upload, encode the file etc.

Comment 15

[Andrew Halberstadt [:ahal]]

Note, there were a fair amount of edges cases in relation to the highlighting mechanism that I needed to work around. The code in utils.takeScreenshot() is a bit messy, so if you see a way to make it look nicer or if you just have questions about how stuff works, let me know.

Comment 16

[Andrew Halberstadt [:ahal]]

Also there is no concept of "screenshot on error" here. I'm not sure if that's something we should implement, or if it should be left up to the test with try/catch blocks (as I can't imagine we want to make a screenshot for *every* error).

Comment 17

[Jeff Hammel]

(In reply to comment #16)
> Also there is no concept of "screenshot on error" here. I'm not sure if that's
> something we should implement, or if it should be left up to the test with
> try/catch blocks (as I can't imagine we want to make a screenshot for *every*
> error).

IMHO, this should be part of test-land. It *should* (not saying it is) be easy to override/implement error handlers that could do this if desired

Comment 18

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - Add screenshots to mozmill

Allows users the ability to specify whether they want to save the screenshot to their temp dir or retrieve the screenshot via data url.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #18)
> Allows users the ability to specify whether they want to save the screenshot to
> their temp dir or retrieve the screenshot via data url.

Can you please use an optional argument for the path? The temp folder is not always the best case to choose. In our case we are using the screenshot_path from the persisted object, which let tools around Mozmill define the path.

http://hg.mozilla.org/qa/mozmill-tests/file/mozilla2.0/lib/screenshot.js

Comment 20

[cmtalbert]

Comment on attachment 524744 [details] [diff] [review]
Patch 1.0 - Add screenshots to mozmill

This looks good.  I've tested it and the functionality works well.  Nice job.

Henrik, we do not want to open up the ability to write to any data path on the system.  If we do that then we have to worry about security and access rights and whether or not the test could overwrite something important.

TmpD is guaranteed to be available and writable by the OS and it is a safe, good for a place to find the file.  It's a simple work around to write the file into tmpd, get back the path, and then copy the file elsewhere if your tool requires this.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 524744 [details] [diff] [review]
Patch 1.0 - Add screenshots to mozmill

>+/**
>+ * Take a screenshot of specified node
>+ * node: either a window or DOM element
>+ * name: name of the screenshot
>+ * save: if true, saves the screenshot to user's temp directory. Otherwise a data url is returned
>+ * highlights: list of additional elements to highlight with a red rectangle
>+ */
>+MozMillController.prototype.screenShot = function(node, name, save, highlights) {

As given our Mozmill meeting last week a proper JSDoc documentation has to be used. See the assertions module for a good example. Also please name the function. Right now it's a noname function. Further why can't we use takeScreenshot as used in utils.js? screenShot is kinda confusing and doesn't match waitFor, sleep, and others.

>+  if (!node) {
>+    throw new Error("node is undefined");
>+  }

I thought we wanted to start to fire custom errors instead? Probably worth a follow-up bug to update all existing instances too.

>+  // If save is false, a dataURL is used
>+  // Include both in the report anyway to avoid confusion and make the report easier to parse
>+  var filepath, dataURL;
>+  if (save) {
>+    filepath = utils.takeScreenshot(node, name, highlights);
>+  } else {
>+    dataURL = utils.takeScreenshot(node, undefined, highlights);
>+  }

In our case we will probably never use the dataURL because the screenshot should be an attachment in the report which can be handled independently. So not sure how useful the feature is.

>+  // Report object
>+  var obj = { "filepath": filepath,
>+              "dataURL": dataURL,
>+              "name": name,
>+              "timestamp": d.toLocaleString(),

What's the value of an uninitialized variable after transmitted to the Python side? Will it be None? If not, filepath and dataURL have to be initialized correctly.

>+function takeScreenshot(node, name, highlights) {
[..]
>+    top = rect.top;
>+    left = rect.left;

Have you tested that in RTL builds of Firefox?

>+          var addonbar = win.document.getElementById("addon-bar");
>+          if (addonbar) { 
>+            chromeHeight -= addonbar.scrollHeight;
>+          }
>+          var findbar = win.document.getElementById("FindToolbar");
>+          if (findbar) {
>+            chromeHeight -= findbar.scrollHeight;

This will break all applications except Firefox.

>+function saveCanvas(canvas, name) {
>+  file.append("mozmill_screens");
>+  file.append(name + ".png");

You should not force to use a different file name as given by the name.

In it's current implementation it's already hard to see any relation to the test itself, which makes it impossible for us to show it up side-by-side in the dashboard. Could you simply add the current test module name to the screenshot object?

>+  file.createUnique(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0666);

Shouldn't we use 0600 here and only allow the owner of the files to have access?

>+  // create a data url from the canvas and then create URIs of the source and targets  
>+  var io = Components.classes["@mozilla.org/network/io-service;1"]
>+                              .getService(Components.interfaces.nsIIOService);
>+  var source = io.newURI(canvas.toDataURL("image/png", ""), "UTF8", null);
>+  var target = io.newFileURI(file)
>+ 
>+  // prepare to save the canvas data
>+  var persist = Components.classes["@mozilla.org/embedding/browser/nsWebBrowserPersist;1"]
>+                                   .createInstance(Components.interfaces.nsIWebBrowserPersist);
>+
>+  persist.persistFlags = Components.interfaces.nsIWebBrowserPersist.PERSIST_FLAGS_REPLACE_EXISTING_FILES;
>+  persist.persistFlags |= Components.interfaces.nsIWebBrowserPersist.PERSIST_FLAGS_AUTODETECT_APPLY_CONVERSION;
>+ 
>+  // save the canvas data to the file
>+  persist.saveURI(source, null, null, null, null, file);

You should probably enclose all that code in a try/catch. What happens right now if an exception is raised?

Comment 22

[Andrew Halberstadt [:ahal]]

(In reply to comment #21)
> As given our Mozmill meeting last week a proper JSDoc documentation has to be
> used.
Will do.
 
> Also please name the function. Right now it's a noname function.
Maybe I just don't know javascript that well, but why is this a bad thing? Also every function in controller is "anonymous".
. 
> In our case we will probably never use the dataURL
> So not sure how useful the feature is.
I think it's a cleaner was of managing the screenshots rather than having to copy and paste from the temp dir. Also you can send the dataURL to a couch db as an attachment.
 
> Have you tested that in RTL builds of Firefox?
Nope, never thought about it.. good thinking though, I'll give it a test.
 
> >+          var addonbar = win.document.getElementById("addon-bar");
> >+          if (addonbar) { 
> >+            chromeHeight -= addonbar.scrollHeight;
> >+          }
> >+          var findbar = win.document.getElementById("FindToolbar");
> >+          if (findbar) {
> >+            chromeHeight -= findbar.scrollHeight;
> 
> This will break all applications except Firefox.
No, this should work in other applications. I'm checking for existence (if they don't exist then I don't have to do anything). It might throw a warning in the error console in the worst case but I don't think it will even do that.

> Could you simply add the current test module name to the screenshot object?
Yeah, I'll do that.

> Shouldn't we use 0600 here and only allow the owner of the files to have
> access?
I would say no.. the screenshots are getting saved to the temp directory. If the user needs them they should move them somewhere else immediately (or use the dataURL). I don't want to get in a situation where the user can't delete their temporary files just because mozmill was run with higher privileges at some point.

Comment 23

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #22)
> Maybe I just don't know javascript that well, but why is this a bad thing? Also
> every function in controller is "anonymous".

Noname functions are extremely hard to debug with i.e. Venkman. Just use ObjX.prototype.method = function ObjX_method(...)

> I think it's a cleaner was of managing the screenshots rather than having to
> copy and paste from the temp dir. Also you can send the dataURL to a couch db
> as an attachment.

But in the current case it is directly bundled inside the report, so our own scripts would have to remove all those entries before uploading the report to the couchdb or any other database.

> > >+          var addonbar = win.document.getElementById("addon-bar");
> > >+          if (addonbar) { 
> > >+            chromeHeight -= addonbar.scrollHeight;
> > >+          }
> > >+          var findbar = win.document.getElementById("FindToolbar");
> > >+          if (findbar) {
> > >+            chromeHeight -= findbar.scrollHeight;
> > 
> > This will break all applications except Firefox.
> No, this should work in other applications. I'm checking for existence (if they
> don't exist then I don't have to do anything). It might throw a warning in the
> error console in the worst case but I don't think it will even do that.

Sorry, my fault in misreading the patch here. But why we have to do that? I frankly don't fully understand the current method in determining the offset. Is the handling of the bottom toolbars needed to only get the height of the top toolbars? If yes, the code would be a bit fragile and will break when another toolbar gets added or will exist in another application.

> > Could you simply add the current test module name to the screenshot object?
> Yeah, I'll do that.

Thanks! I kinda appreciate that.

> I would say no.. the screenshots are getting saved to the temp directory. If
> the user needs them they should move them somewhere else immediately (or use
> the dataURL). I don't want to get in a situation where the user can't delete
> their temporary files just because mozmill was run with higher privileges at
> some point.

The selection between the file vs. dataURL cannot be done by the user. It's a hard-coded option in the tests itself. If Mozmill would provide a --screenshot-path via the command line that would offer our users the chance to select between those two methods. In the same step we could also specify another folder to store the images, which is kinda important for our Mozmill Crowd extension too.

Comment 24

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.1 - incremental - check for horizontal offset + minor tweaks

I addressed many of Henrik's comments but I also account for potential horizontal offset (if the bookmark or history sidebar is open).

The changes in this patch were big enough that I wasn't comfortable checking in without a second review.

Comment 25

[cmtalbert]

Comment on attachment 528154 [details] [diff] [review]
Patch 1.1 - incremental - check for horizontal offset + minor tweaks

Review of attachment 528154 [details] [diff] [review]:

Quick drive by review.  I might be missing something.  But just a couple of nits so far.

::: mozmill/mozmill/extension/resource/modules/controller.js
@@ +328,5 @@
   }
   
   // If save is false, a dataURL is used
   // Include both in the report anyway to avoid confusion and make the report easier to parse
   var filepath, dataURL;

You need to initialize these so that the one you don't include in the report is '' rather than undefined.

::: mozmill/mozmill/extension/resource/modules/utils.js
@@ +506,1 @@
       if (needsOffset) {

I don't see where needsOffset is being set/determined.

Comment 26

[cmtalbert]

Comment on attachment 528154 [details] [diff] [review]
Patch 1.1 - incremental - check for horizontal offset + minor tweaks

ok, I see what I was missing.  r+ with the one nit that matters (i.e. not the needsoffset one).

Comment 27

[Andrew Halberstadt [:ahal]]

Maybe I don't understand why having filepath/dataURL be '' instead of undefined is a good thing.  Basically the way it is now, if filepath never gets initialized then there will be no 'filepath' attribute on the python reporting side.

To me I think this is expected behaviour. Am I wrong here?  Otherwise this pastebin may clear things up: http://pastebin.com/zSQmRFLU

Comment 28

[cmtalbert]

I thought we'd decided that you were right about this. And I thought this had landed.  Does this still need to land, ahal?

Comment 29

[Andrew Halberstadt [:ahal]]

Nope forgot to close the bug apparently.  I blame my busy last week...

master: https://github.com/mozautomation/mozmill/commit/c3a49fea49f8dfefbcc0690d9d2e5d961a8acc10