unit test boxes should upload a screen shot after testing, on Windows

VERIFIED FIXED in mozilla14

Status

enhancement
P5
normal
VERIFIED FIXED
9 years ago
7 years ago

People

(Reporter: karlt, Unassigned)

Tracking

(Blocks 2 bugs)

Trunk
mozilla14
All
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox11 wontfix, firefox12 wontfix, firefox13 wontfix)

Details

Attachments

(1 attachment, 3 obsolete attachments)

+++ 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.
Blocks: 620248
Summary: unit test boxes should upload a screen shot after testing → unit test boxes should upload a screen shot after testing, on Windows (and OSX) too
Version: unspecified → Trunk
There is a separate bug for OS X now.
Summary: unit test boxes should upload a screen shot after testing, on Windows (and OSX) too → unit test boxes should upload a screen shot after testing, on Windows
I had to undef WIN32_LEAN_AND_MEAN in the program's source because it causes a bunch of errors.  I looked at windows.h and added the headers that aren't included because of lean and mean.  My understanding is that WIN32_LEAN_AND_MEAN's purpose is to avoid including unnecessary headers.  Since this is a support program and takes seconds to build, I don't know how important it is to figure out why this is breaking.

https://tbpl.mozilla.org/?tree=Try&rev=503fb7736731
Assignee: nobody → jhford
Status: NEW → ASSIGNED
this is having issues, specifically screenshot.exe is returning 1 even though it is taking the screenshot.
Attachment #571838 - Attachment is obsolete: true
Shouldn't the last line of wmain be something like

  return stat == Ok ? 0 : 1;

i.e. returning 0 on success?
Posted patch updated patch (obsolete) — Splinter Review
Updated per Cameron's comment 6.

This seems to mostly work (screenshot.exe is invoked and screen shot gets saved to temp directory correctly), but the code that actually reads the PNG and tries to add it to the log seems to fail - only the first few bytes are read, and the output is "SCREENSHOT: data:image/png;base64,iVBORwo=".

This same code works on Mac, so I'm not sure what's up. Some Windows-specific python oddity with the way we're reading the file?
Presumably the open line needs to be changed to open(imgfilename, 'rb') to open it in binary mode. It would be nice to change screenshot.cpp to accept "-" as "write to stdout", but it looks like we'd need to implement something that implements the IStream COM interface, which looks like a gigantic mess, so this is probably simpler.
I made that change and pushed a test patch (which also introduces a test timeout) to try:
https://tbpl.mozilla.org/?tree=Try&rev=074f97c43d39
The try build failed:
['c:\\talos-slave\\test\\build\\bin\\screenshot.exe'] exited with code -1073741515

Google suggests that 0xc0000135 is apparently what you get when there's a missing dependency.
Oh, it worked on Rev3 WINNT 5.1 try debug, but not Rev3 WINNT 6.1 try opt.
Attachment #572031 - Attachment is obsolete: true
Whiteboard: [autoland-try: -p win32 -u mochitests]
Whiteboard: [autoland-try: -p win32 -u mochitests] → [autoland-in-queue]
Autoland Patchset:
	Patches: 595260
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=fbcb125c5b6f
Try run started, revision fbcb125c5b6f. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=fbcb125c5b6f
Try run for fbcb125c5b6f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fbcb125c5b6f
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-fbcb125c5b6f
Whiteboard: [autoland-in-queue]
(In reply to Mozilla RelEng Bot from comment #15)
>     https://tbpl.mozilla.org/?tree=Try&rev=fbcb125c5b6f

It built "-p all -u none" :-(
Doesn't it support a space?
Whiteboard: [autoland-try:-p win32 -u mochitests]
Whiteboard: [autoland-try:-p win32 -u mochitests] → [autoland-in-queue]
Autoland Patchset:
	Patches: 595260
	Branch: mozilla-central => try
	Destination: http://hg.mozilla.org/try/pushloghtml?changeset=ae857c1615a3
Try run started, revision ae857c1615a3. To cancel or monitor the job, see: https://tbpl.mozilla.org/?tree=Try&rev=ae857c1615a3
Try run for ae857c1615a3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ae857c1615a3
Results (out of 26 total builds):
    exception: 1
    success: 24
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/autolanduser@mozilla.com-ae857c1615a3
Whiteboard: [autoland-in-queue]
(In reply to Mozilla RelEng Bot from comment #18)
>     https://tbpl.mozilla.org/?tree=Try&rev=ae857c1615a3

Hum, it passed, but the attached patch does not include the fake timeout trigger :-|
Posted patch working patchSplinter Review
This patch works! Just needed a USE_STATIC_LIBS=1.

https://tbpl.mozilla.org/?tree=Try&rev=11bcdbd99f43
Attachment #595260 - Attachment is obsolete: true
Not sure who should review this (if anyone). Can we just land it?
Attachment #604605 - Flags: review?(rcampbell)
Assignee: jhford → gavin.sharp
Flags: in-testsuite-
OS: All → Windows XP
I don't think it's particularly worth reviewing the screenshot.cpp code, honestly. I can review the rest (or you can get khuey or someone to review it if that would make you feel better).
Comment on attachment 604605 [details] [diff] [review]
working patch

+
+  for(UINT j = 0; j < num; ++j)
+    {
+      if( wcscmp(pImageCodecInfo[j].MimeType, format) == 0 )
+        {
+          *pClsid = pImageCodecInfo[j].Clsid;
+          free(pImageCodecInfo);
+          return j;  // Success
+        }    
+    }
+

geez, ted. What's up with that indent? ;)

This is landable, but I'd like this to land AFTER the merge (tomorrow, March 13, 2012).

Ping for re-review then.
also, I think you can compile your screenshotter as straight-up C and save yourself some runtime size, if you care.
(In reply to Rob Campbell [:rc] (robcee) from comment #24)
> also, I think you can compile your screenshotter as straight-up C and save
> yourself some runtime size, if you care.

sorry, disregard. Looked more closely and see the gdiplus.h #include and using namespace calls.

carry on...
(In reply to Rob Campbell [:rc] (robcee) from comment #23)
> This is landable, but I'd like this to land AFTER the merge (tomorrow, March
> 13, 2012).
> 
> Ping for re-review then.

Any particular reason? This doesn't seem very risky to me. The automation-touching bits are really minor, and it's gone through a successful try run.
Serge: could you please stop micro-managing bugs? I'm quite capable of requesting review and assigning bugs to myself, so if I don't do it there's usually a reason.
(In reply to Ted Mielczarek [:ted] from comment #26)
> (In reply to Rob Campbell [:rc] (robcee) from comment #23)
> > This is landable, but I'd like this to land AFTER the merge (tomorrow, March
> > 13, 2012).
> > 
> > Ping for re-review then.
> 
> Any particular reason? This doesn't seem very risky to me. The
> automation-touching bits are really minor, and it's gone through a
> successful try run.

I've been burned in the past by changes to our automation that caused the tree to light up. I don't want to do it to others. I don't see any urgency to take this before the merge.
Assignee: gavin.sharp → nobody
Attachment #604605 - Flags: review?(rcampbell) → review?(khuey)
Comment on attachment 604605 [details] [diff] [review]
working patch

when you ran this through try did you happen to cause any tests to fail?
(In reply to Rob Campbell [:rc] (robcee) from comment #29)
> when you ran this through try did you happen to cause any tests to fail?

Yes, check comment 20.
Comment on attachment 604605 [details] [diff] [review]
working patch

someone just r+ this already! :P
Attachment #604605 - Flags: review?(ted.mielczarek)
Attachment #604605 - Flags: review?(rcampbell)
Attachment #604605 - Flags: review?(khuey)
Attachment #604605 - Flags: review?(ted.mielczarek)
Attachment #604605 - Flags: review?(rcampbell)
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #33)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/84f9eb64e005

This already went into action :-)

https://tbpl.mozilla.org/php/getParsedLog.php?id=10106644&tree=Mozilla-Inbound
Rev3 WINNT 6.1 mozilla-inbound opt test mochitests-3/5 on 2012-03-15 16:06:29 PDT for push 84f9eb64e005
{
11022 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/base/tests/test_bug586662.html | Test timed out.
args: ['c:\\talos-slave\\test\\build\\bin\\screenshot.exe', 'c:\\users\\cltbld\\appdata\\local\\temp\\mozilla-test-fail_okrms0']
SCREENSHOT: data:image/png;base64,[...]
}
Comment on attachment 604605 [details] [diff] [review]
working patch

[Approval Request Comment]
Regression caused by (bug #): (enhancement)
User impact if declined: None, but less help to diagnose test timeouts.
Testing completed (on m-c, etc.): comment 33.
Risk to taking this patch (and alternatives if risky): None, test (harness) only.
String changes made by this patch: None.
Attachment #604605 - Flags: approval-mozilla-beta?
Attachment #604605 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/84f9eb64e005
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1331928910.1331934391.22023.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/03/16 13:15:10
{
6507 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/relations/test_tabbrowser.xul | Test timed out.
args: ['e:\\builds\\slave\\test\\build\\bin\\screenshot.exe', 'c:\\docume~1\\seabld\\locals~1\\temp\\mozilla-test-fail_fgfyct']
SCREENSHOT: data:image/png;base64,[...]
}

V.Fixed
Status: RESOLVED → VERIFIED
Attachment #604605 - Flags: approval-mozilla-beta?
Attachment #604605 - Flags: approval-mozilla-aurora?
You need to log in before you can comment on or make changes to this bug.