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

VERIFIED FIXED in mozilla13

Status

defect
--
minor
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: sgautherie, Assigned: samdgarrett)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Whiteboard: [good first bug]
Whiteboard: [good first bug] → [good first bug][mentor=ctalbert]
Whiteboard: [good first bug][mentor=ctalbert] → [good first bug][mentor=ctalbert][lang=py]
Sam said he'd like to work on this.
Assignee: nobody → samdgarrett
Status: NEW → ASSIGNED
I have changed the lines 750-754 to catch this bug and only dump one screenshot.
Attachment #603018 - Attachment is obsolete: true
Attachment #603028 - Attachment is patch: true
Attachment #603028 - Attachment is obsolete: true
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.
Attachment #603028 - Attachment is obsolete: false
Attachment #603028 - Flags: feedback+
Attachment #603018 - Attachment description: Handling double screenshot. → Remind about multiple screenshots [Whole file]
Attachment #603028 - Attachment description: Handling double screenshot. → Remind about multiple screenshots
Attachment #603028 - Attachment is obsolete: true
Attachment #603028 - Flags: feedback+
Attachment #603031 - Attachment description: Handling double screenshot. → Remind about multiple screenshots
Attachment #603031 - Flags: review?(karlt)
Attachment #603031 - Flags: feedback+
Attachment #603031 - Attachment is patch: true
Attachment #603031 - Flags: review?(karlt) → review+
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.
Flags: in-testsuite-
Hardware: x86 → All
Target Milestone: --- → mozilla13
(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?
(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.
Including patch to killAndGetStack.
Attachment #603112 - Flags: review?(karlt)
Attachment #603112 - Flags: feedback+
Attachment #603112 - Flags: review?(karlt) → review+
Attachment #603031 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3609fba2b721

And fixed on trunk! Thank you :-)
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
OS: Linux → All
Whiteboard: [good first bug][mentor=ctalbert][lang=py]
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).
Status: RESOLVED → VERIFIED
Blocks: 736344
You need to log in before you can comment on or make changes to this bug.