637103 - automation.py.in: add a reminder when not taking new screenshots after first one

Status: VERIFIED FIXED

(Testing :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

Bug 414049 comment 26:
{
Karl Tomlinson (:karlt)      2010-08-11 00:50:30 PDT

The screen is only dumped on the first timeout so as not to make
the logs unnecessarily large.
}

http://mxr.mozilla.org/comm-central/source/mozilla/build/automation.py.in#766
766         if self.UNIXISH and not debuggerInfo and not self.haveDumpedScreen and "TEST-UNEXPECTED-FAIL" in line and "Test timed out" in line:
767           self.dumpScreen(utilityPath)

I suggest to enhance this to something like
{
if ...:
  if self.haveDumpedScreen:
    self.log.info("not taking sreenshot here: see the one that was previously logged")
  else:
    self.dumpScreen(utilityPath)
}
so they don't wonder why there is no screenshot nor miss to check the (only) one that was taken.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Sam said he'd like to work on this.

Comment 2

[Sam Garrett]

Attachment: Remind about multiple screenshots [Whole file]

I have changed the lines 750-754 to catch this bug and only dump one screenshot.

Comment 3

[Sam Garrett]

Attachment: Remind about multiple screenshots

Comment 4

[Sam Garrett]

Attachment: Remind about multiple screenshots

Comment 5

[Serge Gautherie (:sgautherie)]

Comment on attachment 603028 [details] [diff] [review]
Remind about multiple screenshots

Review of attachment 603028 [details] [diff] [review]:
-----------------------------------------------------------------

Just what I suggested in comment 0 ;-)

::: build/automation.py.in
@@ +800,5 @@
> +            self.log.info("Not taking screenshot here: see the one that was previously logged")
> +          else:
> +            self.dumpScreen(utilityPath)
> +        #if not debuggerInfo and not self.haveDumpedScreen and "TEST-UNEXPECTED-FAIL" in line and "Test timed out" in line:
> +          #self.dumpScreen(utilityPath)

Remove these commented out lines.

Comment 6

[Serge Gautherie (:sgautherie)]

I hadn't noticed, but
738   def killAndGetStack(self, proc, utilityPath, debuggerInfo):
739     """Kill the process, preferrably in a way that gets us a stack trace."""
740     if not debuggerInfo and not self.haveDumpedScreen:
741       self.dumpScreen(utilityPath)
needs this too.

Comment 7

[Serge Gautherie (:sgautherie)]

See also
https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 8

[Sam Garrett]

(In reply to Serge Gautherie (:sgautherie) from comment #6)
> I hadn't noticed, but
> 738   def killAndGetStack(self, proc, utilityPath, debuggerInfo):
> 739     """Kill the process, preferrably in a way that gets us a stack
> trace."""
> 740     if not debuggerInfo and not self.haveDumpedScreen:
> 741       self.dumpScreen(utilityPath)
> needs this too.

Do I need to add this to the patch or something?

Comment 9

[Serge Gautherie (:sgautherie)]

(In reply to Sam Garrett from comment #8)
> Do I need to add this to the patch or something?

Yes please: no need to split this bug in two patches.

Comment 10

[Sam Garrett]

Attachment: Remind about multiple screenshots

Including patch to killAndGetStack.

Comment 11

[cmtalbert]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3609fba2b721

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3609fba2b721

And fixed on trunk! Thank you :-)

Comment 13

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1331292785.1331297387.23923.gz
WINNT 5.2 comm-central-trunk debug test mochitest-other on 2012/03/09 03:33:05
{
13144 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/suite/common/downloads/tests/test_ui_stays_open_on_alert_clickback.xul | Test timed out.
If you fixed bug 589668, you'd get a screenshot here

33466 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/toolkit/mozapps/downloads/tests/chrome/test_destinationURI_annotation.xul | Test timed out.
Not taking screenshot here: see the one that was previously logged

38214 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_bug429954.xul | Test timed out.
Not taking screenshot here: see the one that was previously logged

38245 ERROR TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/widget/tests/test_bug517396.xul | Test timed out.
Not taking screenshot here: see the one that was previously logged
38246 ERROR TEST-UNEXPECTED-FAIL | (SimpleTest/TestRunner.js) | 4 test timeouts, giving up.
}

V.Fixed (for the non-kill case).