484231 - A Mochitest crashdump turns the waterfall orange but misses to |TinderboxPrint|

Status: RESOLVED INCOMPLETE

(Release Engineering :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

Example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1237459052.1237466973.12663.gz&fulltext=1
Linux mozilla-central unit test on 2009/03/19 03:37:32

Build Error Summary

TEST-UNEXPECTED-FAIL | (automation.py) | Exited with code 1 during test run
TEST-UNEXPECTED-FAIL | (automation.py) | Browser crashed (minidump found)
TEST-UNEXPECTED-FAIL | runtests-leaks | missing output line for total leaks!
}

***

automation.py > runApp()
{
  if checkForCrashes(profileDir, symbolsPath):
    status = -1

  ...

  return status
}
which is fine.


runtests.py.in > main()
{
  status = automation.runApp(testURL, browserEnv, options.app,

  ...

  # hanging due to non-halting threads is no fun; assume we hit the errors we
  # were going to hit already and exit with a success code
  sys.exit(0)
}
This code was added by bug 418009.
In general, can't we use |sys.exit(status)| here ?
(Should help bug 483230 too, shouldn't it ?)


steps.py:
I think this should be specifically handled in ShellCommandReportTimeout().
(or at least MozillaReftest() and MozillaMochitest())

Comment 1

[Benjamin Smedberg]

I don't understand what you're asking for here: what do you think should be TinderboxPrinted?

Comment 2

[Serge Gautherie (:sgautherie)]

I think the "expected" report for a crash would be "FAIL".
But, because it seems this crash happened after the test suite completed, steps.py doesn't catch it in MozillaMochitest() either.

I'm thinking about a patch for the steps.py part: hold on...

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) steps.py: Add 'CRASH' support to summaryText() and use it

(To be applied on top of bug 473259 patch Cv1a and Dv1.)

Comment 4

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) steps.py: Add 'CRASH' support to summaryText() and use it, Enhance leak detection and use it for Reftest too

Av1 Reftest |elif crashRe.match(line):| could not work :-<

While there:
*Enhance leak detection which can have the same waterfall issue.
*Keep Reftest in sync'. (So I can proceed with bug 469518 too.)
*Explicitly differentiate test counts and leak 'FAIL's.

(To be applied on top of bug 473259 patch Cv1a and Dv1.)

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> In general, can't we use |sys.exit(status)| here ?

Jeff ?

Comment 6

[Jeff Walden [:Waldo]]

I think you could exit with status there, but I suspect it might cut tinderbox builds short if the first mochitest run fails, even if it's useful to know whether the other mochitest runs pass or not.

I think the important part there was that it exited rather than returned, because other threads in Python itself might hang (processing server output in that case) and prevent runtests.py from finishing at all.  That might or might not be a problem any more, but I'd be inclined to leave it alone and not worry about it.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) runtests.py.in: return |status| instead of |0|, and 3 (unrelated) nits [Checkin: See comment 15 & 16]

(Following our irc discussion, with nthomas too.)

All other |runApp()| callers return |status|:
http://mxr.mozilla.org/mozilla-central/search?string=runApp%5C%28&regexp=on&case=on


(In reply to comment #6)
> I think you could exit with status there, but I suspect it might cut tinderbox
> builds short if the first mochitest run fails

nthomas and I think a non-zero return code is not expected to abort following test suites.

What I expect it to do is trigger the catch-all 'Unknown Error: command finished with exit code: %d' from
http://mxr.mozilla.org/build/source/buildbotcustom/unittest/steps.py
... in order to catch (other) "unexpected" cases.

> I think the important part there was that it exited rather than returned,

That's how I understood the comment too :-)

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 369190 [details] [diff] [review]
(Bv1) runtests.py.in: return |status| instead of |0|, and 3 (unrelated) nits
[Checkin: See comment 15 & 16]

If it works, fine; I just wouldn't have spent any time trying to figure that out given that we already get error messages when a non-zero status happens, even if 0 gets returned here.

Comment 9

[Serge Gautherie (:sgautherie)]

ccooper, ping ?

Comment 10

[Chris Cooper [:coop] (he/him)]

end of quarter == really bad time to expect quick turnaround on reviews. Sorry.

Comment 11

[Serge Gautherie (:sgautherie)]

Attachment: (Av2a) steps.py: Add 'CRASH' support to summaryText() and use it, Enhance leak detection and use it for TUnit and Reftest too [Checkin: Comment 13]

Av2, extended to TUnit.

Comment 12

[Chris Cooper [:coop] (he/him)]

Comment on attachment 369803 [details] [diff] [review]
(Av2a) steps.py: Add 'CRASH' support to summaryText() and use it, Enhance leak detection and use it for TUnit and Reftest too
[Checkin: Comment 13]

>-def summaryText(passCount, failCount, knownFailCount = None, leaked = False):
>+def summaryText(passCount, failCount, knownFailCount = None,
>+        crashed = False, leaked = False):

Not that it really matters because you're touching all the affected files, but is there a reason you changed the args order here, inserting crashed before leaked rather than just adding crashed to the end?

>+        # Regular expression for crash and leak detections.
>+        harnessErrorsRe = re.compile(r"TEST-UNEXPECTED-FAIL \| .* \| (missing output line for total leaks!|negative leaks caught!|leaked \d+ bytes during test execution)$")
>+        # Process the log.
>         for line in log.readlines():
>             if "TEST-PASS" in line:
>                 passCount = passCount + 1
>+                continue
>             if "TEST-UNEXPECTED-" in line:
>-                failCount = failCount + 1
>+                # Set the error flags.
>+                # Or set the failure count.
>+                m = harnessErrorsRe.match(line)
>+                if m:
>+                    r = m.group(1)
>+                    if r == "missing output line for total leaks!":
>+                        leaked = None
>+                    else:
>+                        leaked = True
>+                else:
>+                    failCount = failCount + 1
>+                # continue

Do we need a elif case for negative leaks, or is reporting it as an UNEXPECTED-FAIL + leak sufficient? (same for other cases below)

Comment 13

[Serge Gautherie (:sgautherie)]

Comment on attachment 369803 [details] [diff] [review]
(Av2a) steps.py: Add 'CRASH' support to summaryText() and use it, Enhance leak detection and use it for TUnit and Reftest too
[Checkin: Comment 13]


http://hg.mozilla.org/build/buildbotcustom/rev/6ed37da063bd


(In reply to comment #12)

Per your irc approval:

> is there a reason you changed the args order here, inserting crashed before
> leaked rather than just adding crashed to the end?

Simply seemed more ordered that way.

> Do we need a elif case for negative leaks, or is reporting it as an
> UNEXPECTED-FAIL + leak sufficient?

On the waterfall, no need to distinguish negative/positive leaks.

Comment 14

[Chris Cooper [:coop] (he/him)]

Av2a is now running in production.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 369190 [details] [diff] [review]
(Bv1) runtests.py.in: return |status| instead of |0|, and 3 (unrelated) nits
[Checkin: See comment 15 & 16]


http://hg.mozilla.org/mozilla-central/rev/fe9f9ba0b6ff

After fixing context for
{
patching file build/automation.py.in
Hunk #1 FAILED at 62
1 out of 1 hunks FAILED
patching file layout/tools/reftest/runreftest.py
Hunk #1 FAILED at 142
1 out of 1 hunks FAILED
}

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 369190 [details] [diff] [review]
(Bv1) runtests.py.in: return |status| instead of |0|, and 3 (unrelated) nits
[Checkin: See comment 15 & 16]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e57603b0c758

Comment 17

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

Its been over a year since last comment. Is this still a valid bug or can we close?

Comment 18

[Chris Cooper [:coop] (he/him)]

Serge: based on the whiteboard, can you post a patch that contains *only* the files you do need from bug 483062 on 1.9.1? 

That will take the guesswork out of someone (me) landing it.

Comment 19

[Chris Cooper [:coop] (he/him)]

I'm happy to land an updated patch when it appears.