Serge Gautherie (:sgautherie) Assignee
	Description • 16 years ago

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())

	Benjamin Smedberg
	Comment 1 • 16 years ago

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

	Serge Gautherie (:sgautherie) Assignee
	Comment 2 • 16 years ago

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...

	Serge Gautherie (:sgautherie) Assignee
	Comment 3 • 16 years ago

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

	Serge Gautherie (:sgautherie) Assignee
	Comment 4 • 16 years ago

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.)

	Serge Gautherie (:sgautherie) Assignee
	Comment 5 • 16 years ago

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

Jeff ?

	Jeff Walden [:Waldo]
	Comment 6 • 16 years ago

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.

	Serge Gautherie (:sgautherie) Assignee
	Comment 7 • 16 years ago

(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 :-)

	Jeff Walden [:Waldo]
	Comment 8 • 16 years ago

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.

	Serge Gautherie (:sgautherie) Assignee
	Comment 9 • 16 years ago

ccooper, ping ?

	Chris Cooper [:coop] (he/him)
	Comment 10 • 16 years ago

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

	Serge Gautherie (:sgautherie) Assignee
	Comment 11 • 16 years ago

Av2, extended to TUnit.

	Chris Cooper [:coop] (he/him)
	Comment 12 • 16 years ago

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)

	Serge Gautherie (:sgautherie) Assignee
	Comment 13 • 16 years ago

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.

	Chris Cooper [:coop] (he/him)
	Comment 14 • 16 years ago

Av2a is now running in production.

	Serge Gautherie (:sgautherie) Assignee
	Comment 15 • 16 years ago

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
}

	Serge Gautherie (:sgautherie) Assignee
	Comment 16 • 16 years ago

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

	John O'Duinn [:joduinn] (please use "needinfo?" flag)
	Comment 17 • 15 years ago

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

	Chris Cooper [:coop] (he/him)
	Comment 18 • 14 years ago

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.

	Chris Cooper [:coop] (he/him)
	Comment 19 • 14 years ago

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