473259 - /buildbotcustom/unittest/steps.py : update the rest of the code after bug 468023

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

As I wrote I would do in bug 468023 comment 18.

Comment 1

[Serge Gautherie (:sgautherie)]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/tools/buildbotcustom/unittest/steps.py&rev=1.1

rcampbell,
can |import os| be removed or why is it there for ?
(I don't see it explicitly used anywhere.)

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/tools/buildbotcustom/unittest/steps.py&rev=1.15

catlee,
in the __init__()s, what is the preferred order for |ShellCommand*.__init__(self, **kwargs)| and |self.super_class = ShellCommand*| ?
I'd like to use the same order everywhere, unless there is a reason why it is currently different.

Comment 2

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #1)
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/tools/buildbotcustom/unittest/steps.py&rev=1.1
> 
> rcampbell,
> can |import os| be removed or why is it there for ?
> (I don't see it explicitly used anywhere.)

That might be a hold-over from earlier days. Probably safe to remove as I didn't see any os. prefixes anywhere in the file.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) summaryText() : support None value for leaked argument ++ [Checkin: Comment 6]

I want to use this in a later patch.

This case should rarely happen,
but bug 471085 is an example of what looks like a crash/hang at shutdown:
the tests summary is there, but the leak report is missing.

Comment 4

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

(In reply to comment #3)
> I want to use this in a later patch.

Should I be checking this in now, or waiting for the later patch? That's the only reason I didn't check-in immediately.

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)
> Should I be checking this in now

Yes, we'll do this step by step: 1 patch, 1 checkin. Thanks.

Comment 6

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

Comment on attachment 356743 [details] [diff] [review]
(Av1) summaryText() : support None value for leaked argument ++
[Checkin: Comment 6]

Checking in steps.py;
/cvsroot/mozilla/tools/buildbotcustom/unittest/steps.py,v  <--  steps.py
new revision: 1.22; previous revision: 1.21
done

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) Inherit from MozillaMochitest [Checkin: Comment 9]

Fix the "Mochitest" class relationships and remove duplicated code.

This changes some |name|, |description| and |descriptionDone| values:
the former will, at least, be noticeable on the tinderbox waterfall.
I hope these changes are for the better, but tell me if they are not.

Comment 9

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

Comment on attachment 356887 [details] [diff] [review]
(Bv1) Inherit from MozillaMochitest
[Checkin: Comment 9]

Do we check for brand_name anywhere before using it? (not that your patch is the offender there)

Checking in steps.py;
/cvsroot/mozilla/tools/buildbotcustom/unittest/steps.py,v  <--  steps.py
new revision: 1.23; previous revision: 1.22
done

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 356887 [details] [diff] [review]
(Bv1) Inherit from MozillaMochitest
[Checkin: Comment 9]


(In reply to comment #9)
> Do we check for brand_name anywhere before using it?

I have no idea and I'm not familiar with MacOSX: see http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/tools/buildbotcustom/unittest/steps.py&mark=1.18#1.23

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

we use the brand_name to be able to distinguish from "firefox" or "shiretoko" for example, when launching the mac .app

Comment 12

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) Merge 'FAIL' handling into summaryText()

*Merge (and synchronize) 'FAIL' handling.
*s/emphasizeFailureText(summary)/emphasizeFailureText(failCountStr)/ (only).
*s/'browser'/self.name/ (== 'mochitest-browser-chrome').

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a) Merge 'FAIL' handling into summaryText() [Checkin: Comment 16]

Cv1, with missing implicit case.

Comment 14

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) Merge duplicated code in class MozillaMochitest [Checkin: Comment 17]

(To be applied on top of patch Cv1a.)

*Merge common code.
*Add 3 |*Ident| to do so more completely.
*s/if/elif/g inside the loop

Comment 15

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

Comment on attachment 368383 [details] [diff] [review]
(Dv1) Merge duplicated code in class MozillaMochitest
[Checkin: Comment 17]

Serge: do you have commit access to this repo? Do you still want/need me to land these various patches after approval?

Comment 16

[Serge Gautherie (:sgautherie)]

Comment on attachment 368319 [details] [diff] [review]
(Cv1a) Merge 'FAIL' handling into summaryText()
[Checkin: Comment 16]


http://hg.mozilla.org/build/buildbotcustom/rev/270081e890ae

Comment 17

[Serge Gautherie (:sgautherie)]

Comment on attachment 368383 [details] [diff] [review]
(Dv1) Merge duplicated code in class MozillaMochitest
[Checkin: Comment 17]


http://hg.mozilla.org/build/buildbotcustom/rev/29f63b61fe06

Comment 18

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

Cv1a and Dv1 are now running in production.

Comment 19

[[e-mail] (:bhearsum)]

I see the whiteboard says 'leave the bug open', but I'm not sure why. Serge?

Comment 20

[Serge Gautherie (:sgautherie)]

Because there will be more patches ;->

Comment 21

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()

Untested, but should be +/- "trivial".

***

{
438         if self.name != 'mochitest-browser-chrome':
439             if not re.search('TEST-PASS', cmd.logs['stdio'].getText()):
440                 return WARNINGS
}
is removed ftb but will be re-added: see bug 483407 comment 20.
We can easily live without it in the meantime.

Comment 22

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

Comment on attachment 377340 [details] [diff] [review]
(Ev1) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()

>+def summarizeLog(name, log, successIdent, failureIdent, otherIdent, infoRe):
>+    # Counts and flags.
>+    successCount = -1
>+    failureCount = -1
>+    otherCount = -1
>     crashed = False
>     leaked = False
> 
>     # Regular expression for result summary details.
>-    infoRe = re.compile(r"REFTEST INFO \| (Successful|Unexpected|Known problems): (\d+) \(")
>+    # Reuse 'infoRe'.
>+    infoRe = re.compile(infoRe)

You're re-compiling a regexp that you've already compiled in the calling function.

>         # Assume that having the "Unexpected: 0" line means the tests run completed.
>         # Also check for "^TEST-UNEXPECTED-" for harness errors.
>         if superResult != SUCCESS or \
>                 not re.search(r"^REFTEST INFO \| Unexpected: 0 \(", cmd.logs["stdio"].getText(), re.MULTILINE) or \
>-                re.search(r"^TEST-UNEXPECTED-", cmd.logs["stdio"].getText(), re.MULTILINE):
>+                re.search("^TEST-UNEXPECTED-", cmd.logs["stdio"].getText(), re.MULTILINE):

Can we line-up the various conditionals in this compound condition with the first one, please?

> class MozillaPackagedXPCShellTests(ShellCommandReportTimeout):
>     warnOnFailure = True
>     warnOnWarnings = True
>     name = "xpcshell"
> 
>     def __init__(self, symbols_path=None, **kwargs):
>+        self.super_class = ShellCommandReportTimeout
>         ShellCommandReportTimeout.__init__(self, **kwargs)
>-        self.super_class = ShellCommandReportTimeout

Is there a reason for this reordering?
      
There are also a bunch of whitespace-only changes. I'm all for standardizing on spacing between functions, but let's get rid of the tab vs. space changes.

r- for the regexp recompilation (and various nits).

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Checkin: See comment 41+42]

Ev1, with comment 22 suggestion(s).

(In reply to comment #22)
> You're re-compiling a regexp that you've already compiled in the calling
> function.

Oops :-<

> >     def __init__(self, symbols_path=None, **kwargs):
> >+        self.super_class = ShellCommandReportTimeout
> >         ShellCommandReportTimeout.__init__(self, **kwargs)
> >-        self.super_class = ShellCommandReportTimeout
> 
> Is there a reason for this reordering?

Only to get the same order in the whole file.

> let's get rid of the tab vs. space changes.

There's no tabs in there...

Comment 24

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

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

changeset:   308:e59f0ff8d308

Comment 25

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

(In reply to comment #24)
> (From update of attachment 377806 [details] [diff] [review])
> changeset:   308:e59f0ff8d308

production-master has been reconfig-ed with this change.

Comment 26

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

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

Had to back this out due to bustage during freeze today.

Can I get a new patch for review against current tip? I'll get it pushed once freeze is over.

Comment 27

[Serge Gautherie (:sgautherie)]

Attachment: (Ev2) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()

Ev1a as it was checked in,
plus fix for comment 26 bustage:

missed "\*\*\* " at the beginning of the count line regex :-<

Comment 28

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

Comment on attachment 378992 [details] [diff] [review]
(Ev2) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()

Patch is baking in staging right now.

Comment 29

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

Comment on attachment 378992 [details] [diff] [review]
(Ev2) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()

Your failIdent strings in, e.g., MozillaMochitest, are still missing the "***" prefix. All the mochitests are consequently reporting warnings in staging (and to MozillaTest tinderboxes) right now.

Can you please test your regexps against a downloaded log locally and then resubmit? Thanks.

Comment 30

[Serge Gautherie (:sgautherie)]

Comment on attachment 378992 [details] [diff] [review]
(Ev2) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()

(In reply to comment #29)

> Your failIdent strings in, e.g., MozillaMochitest, are still missing the "***"
> prefix.

Oh, right, I missed to update the 2 evaluateCommand() :-(

> Can you please test your regexps against a downloaded log locally and then
> resubmit? Thanks.

Ah! I find out why all this confusion happened: see bug 473506 comment 1.
Actually, patch Ev1a should simply be un-backed-out after that other issue is fixed...

Comment 31

[Serge Gautherie (:sgautherie)]

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]


(In reply to comment #30)

> Actually, patch Ev1a should simply be un-backed-out after that other issue is
> fixed...

That is fixed, ready for un-backout.

Comment 32

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

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

I'm running this patch through staging again first just to be sure.

Comment 33

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

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

changeset:   315:1ad485bd9527

Comment 34

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

I've backed this out again, after complaints about this sort of thing:

*** 90095 INFO TEST-PASS | /tests/uriloader/exthandler/tests/mochitest/test_handlerApps.xhtml | uri passed to web-handler app
*** 90097 INFO Passed: 83238
*** 90098 INFO Failed: 0
*** 90099 INFO Todo:   1293
*** 90100 INFO SimpleTest FINISHED
INFO | automation.py | Application ran for: 0:32:21.253333
...
TEST-PASS | runtests-leaks | no leaks detected!
program finished with exit code 0
elapsedTime=1943.079562
TinderboxPrint: mochitest-plain<br/><em class="testfail">T-FAIL</em>
=== Output ended ===

That is, no failed tests & no leaks & zero exit status should give a pass result, not fail. For more examples, pick any Tracemonkey unit test build that started after 14:34 and before 20:57 today (probably all logs, I didn't check).

Comment 35

[Serge Gautherie (:sgautherie)]

(In reply to comment #34)

> *** 90098 INFO Failed: 0
> 
> any Tracemonkey unit test build

Obviously, Tracemonkey misses bug 473506 fix... :-/

Comment 36

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

(In reply to comment #35) 
> Obviously, Tracemonkey misses bug 473506 fix... :-/

So you're going to get a patch posted there for tracemonkey, or at least ask for a similar check-in, right?

Comment 37

[Serge Gautherie (:sgautherie)]

(In reply to comment #36)
> So you're going to get a patch posted there for tracemonkey, or at least ask
> for a similar check-in, right?

Fwiw:
{
[17:15]	<sgautherie>	coop: about bug 473259, how do I tell people to sync' with m-c/m-1.9.1?
[17:16]	<ted>	sgautherie: it's just a matter of merging
	but JS people are heads-down on blockers
[17:17]	so i wouldn't bother them till 3.5 ships
}

Comment 38

[Serge Gautherie (:sgautherie)]

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

The missing sync' was included in
http://hg.mozilla.org/tracemonkey/rev/21d60d78c57a

Ready for 3rd attempt at landing this patch :-|

Comment 39

[Serge Gautherie (:sgautherie)]

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

Ping for landing.

Comment 40

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

We have another downtime scheduled for Friday. I'll try to land this then.

Comment 41

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

Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

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

Comment 42

[[e-mail] (:bhearsum)]

(In reply to comment #41)
> (From update of attachment 377806 [details] [diff] [review])
> http://hg.mozilla.org/build/buildbotcustom/rev/6cd6c720784e

Looks like there was copy/paste fail or something. Line 415 has broken improperly. I checked in a bustage fix for it:
changeset:   345:1c83119a1eb0

Comment 43

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) Create and use summarizeLogXpcshelltests(), plus some internal nits [Checkin: Comment 46]

(Nearly untested.)

Comment 44

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

Comment on attachment 392006 [details] [diff] [review]
(Fv1) Create and use summarizeLogXpcshelltests(), plus some internal nits
[Checkin: Comment 46]

@@ -362,22 +383,24 @@ class MozillaReftest(ShellCommandReportT
         self.addFactoryArguments(test_name=test_name,
                                  leakThreshold=leakThreshold)
    
     def createSummary(self, log):
         self.addCompleteLog('summary', summarizeLogReftest(self.name, log))
 
     def evaluateCommand(self, cmd):
         superResult = self.super_class.evaluateCommand(self, cmd)
+        if superResult != SUCCESS:
+            return WARNINGS

Is there a reason why we return WARNINGS here rather than superResult? 

You do the same in MozillaMochitest further down too.

Otherwise, the rest looks fine.

Comment 45

[Serge Gautherie (:sgautherie)]

(In reply to comment #44)
> Is there a reason why we return WARNINGS here rather than superResult?

I asked catlee in bug 383136 comment 43 (and 34 and 44), but got no answer ... then I just keep this as it is.
Yet, I can change this before check-in if you want...

Comment 46

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

Comment on attachment 392006 [details] [diff] [review]
(Fv1) Create and use summarizeLogXpcshelltests(), plus some internal nits
[Checkin: Comment 46]

http://hg.mozilla.org/build/buildbotcustom/rev/417d1b245bad

Comment 47

[Nick Thomas [:nthomas] (UTC+12)]

(In reply to comment #46)
> (From update of attachment 392006 [details] [diff] [review])
> http://hg.mozilla.org/build/buildbotcustom/rev/417d1b245bad

Got pushed into production at 1546 today.

Comment 48

[Philippe M. Chiasson (:gozer)]

Attachment: fix parsing for mozmill tests [Superseded by bug 512592]

(In reply to comment #46)
> (From update of attachment 392006 [details] [diff] [review])
> http://hg.mozilla.org/build/buildbotcustom/rev/417d1b245bad

This broke the mozmill tests, specifically, this bit:

@@ -334,14 +342,27 @@
         self.addFactoryArguments(test_name=test_name)

     def createSummary(self, log):
-        self.addCompleteLog('summary', summarizeTUnit(self.name, log))
+        if self.name == "check":
+            self.addCompleteLog('summary', summarizeTUnit(self.name, log))
+        else:
+            self.addCompleteLog('summary', summarizeLogXpcshelltests(self.name, log))

As a result, for mozmill tests (where self.name="mozmill"), the parser picked is the Xpcshelltests one, and it incorrectly reports T-FAIL.

Comment 49

[Serge Gautherie (:sgautherie)]

Comment on attachment 395664 [details] [diff] [review]
fix parsing for mozmill tests
[Superseded by bug 512592]

Irc, Standard8 told me mozmill currently lives in comm-central mail/test/mozmill.
There, I see 2 runtest*.py scripts.

For a few reasons, I think this fix is wrong as is.
I think mozmill should produce the "same" output as xpcshell-tests.

I would suggest to open a blocking bug and discuss it there:
I would gladly help on this!

Comment 50

[Serge Gautherie (:sgautherie)]

Attachment: (Hv1) mochitest-browser-chrome: support Passed and Failed too [Checkin: Comment 51]

Temporarily support both spellings, to allow bug 473506 patch B to land smoothly.

Comment 51

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

Comment on attachment 396114 [details] [diff] [review]
(Hv1) mochitest-browser-chrome: support Passed and Failed too
[Checkin: Comment 51]

http://hg.mozilla.org/build/buildbotcustom/rev/e7c604636d1e

Comment 52

[Serge Gautherie (:sgautherie)]

Attachment: (Iv1) mochitest-browser-chrome: support Passed and Failed too, 2 missed occurrences [Checkin: Comment 57]

Comment 53

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

Comment on attachment 402841 [details] [diff] [review]
(Iv1) mochitest-browser-chrome: support Passed and Failed too, 2 missed occurrences
[Checkin: Comment 57]

http://hg.mozilla.org/build/buildbotcustom/rev/e3579dabe0ce

Comment 54

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

Comment on attachment 402841 [details] [diff] [review]
(Iv1) mochitest-browser-chrome: support Passed and Failed too, 2 missed occurrences
[Checkin: Comment 57]

Backed out due to bug 525163.

Comment 55

[Serge Gautherie (:sgautherie)]

(In reply to comment #54)
> (From update of attachment 402841 [details] [diff] [review])
> Backed out due to bug 525163.

Ah :-( See bug 525163 comment 6.
Then you need to back out bug 473506, which can't pass without patch Iv1 :-/
(I'd prefer if you un-back out ftb!)

Comment 56

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

I'll run with the patch in staging today to try to reproduce the failure.

Comment 57

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

(In reply to comment #56)
> I'll run with the patch in staging today to try to reproduce the failure.

Quick tests in staging didn't reveal any problems. I've re-landed since we still seem to be failing on the Fail|Failed parsing despite the backout in https://bugzilla.mozilla.org/show_bug.cgi?id=473506#c12.

Comment 58

[Serge Gautherie (:sgautherie)]

KaiRo,
(I emailed you multiple times recently but receive no response...)
SeaMonkey' builbot needs to be updated to/with
http://hg.mozilla.org/build/buildbotcustom/rev/035d6dc87e3b
to fix orange from bug 473506 push.

Comment 59

[Robert Kaiser]

(In reply to comment #58)
> KaiRo,
> (I emailed you multiple times recently but receive no response...)

That's because I'm almost on vacation and busy with preparations for it, pushing out any real work until December.

> SeaMonkey' builbot needs to be updated to/with
> http://hg.mozilla.org/build/buildbotcustom/rev/035d6dc87e3b
> to fix orange from bug 473506 push.

I can look into this right now and do apply that small change locally and manually on the master, updating buildbotcustom to current tip is out of the question right now, as that probably needs half a day of other work to get it working right.

Comment 60

[Robert Kaiser]

(In reply to comment #59)
> I can look into this right now and do apply that small change locally and
> manually on the master, updating buildbotcustom to current tip is out of the
> question right now, as that probably needs half a day of other work to get it
> working right.

Done. And next time, if you don't catch me on IRC and I can immediately act on it, please file a bug on it and assign to me. Those things deserve to be publicly tracked and Bugzilla is our tool of choice for that.

Comment 61

[Serge Gautherie (:sgautherie)]

Attachment: (Jv1) mochitest-browser-chrome: remove Pass and Fail support now [Checkin: Comment 63]

Switch to final code, now that m-c/m-1.9.2/m-1.9.1 are fixed.

Comment 62

[Serge Gautherie (:sgautherie)]

(In reply to comment #59)
> I can look into this right now and do apply that small change locally and
> manually on the master

Thanks.

(In reply to comment #60)
> please file a bug on it and assign to me.

I filed bug 527169 now.

Comment 63

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

Comment on attachment 410917 [details] [diff] [review]
(Jv1) mochitest-browser-chrome: remove Pass and Fail support now
[Checkin: Comment 63]

http://hg.mozilla.org/build/buildbotcustom/rev/b091f85c4607

Comment 64

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

Serge: Found this old bug during triage. Is there anything left to do here, or can we close this now?

Comment 65

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

(In reply to comment #64)
> Serge: Found this old bug during triage. Is there anything left to do here, or
> can we close this now?

No response, and all depbugs are fixed, and all patches here are landed, so closing. 

If there's anything else that needs doing, please file a new separate bug with details.