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

RESOLVED FIXED

Status

Release Engineering
General
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: sgautherie, Assigned: sgautherie)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [triagefollowup][// Leave bug open], URL)

Attachments

(9 attachments, 4 obsolete attachments)

1.77 KB, patch
coop
: review+
Details | Diff | Splinter Review
10.19 KB, patch
coop
: review+
Details | Diff | Splinter Review
6.83 KB, patch
coop
: review+
sgautherie
: checked-in+
Details | Diff | Splinter Review
5.77 KB, patch
coop
: review+
sgautherie
: checked-in+
Details | Diff | Splinter Review
18.15 KB, patch
coop
: review+
Details | Diff | Splinter Review
15.71 KB, patch
coop
: review+
Details | Diff | Splinter Review
2.09 KB, patch
coop
: review+
Details | Diff | Splinter Review
2.08 KB, patch
coop
: review+
sgautherie
: checked-in+
Details | Diff | Splinter Review
4.24 KB, patch
coop
: review+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
As I wrote I would do in bug 468023 comment 18.
(Assignee)

Updated

10 years ago
Depends on: 469444
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

10 years ago
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.
Depends on: 469518, 469523, 460077
(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.
(Assignee)

Comment 3

10 years ago
Created attachment 356743 [details] [diff] [review]
(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.
Attachment #356743 - Flags: review?(ccooper)

Updated

10 years ago
Attachment #356743 - Flags: review?(ccooper) → review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [c-n: Av1 // Leave bug open]

Comment 4

10 years ago
(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.
(Assignee)

Comment 5

10 years ago
(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.

Updated

10 years ago
Attachment #356743 - Flags: checked‑in+ checked‑in+

Comment 6

10 years ago
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

Updated

10 years ago
Keywords: checkin-needed
Whiteboard: [c-n: Av1 // Leave bug open] → [// Leave bug open]
(Assignee)

Updated

10 years ago
No longer depends on: 469444
(Assignee)

Comment 7

10 years ago
Created attachment 356887 [details] [diff] [review]
(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.
Attachment #356887 - Flags: review?(ccooper)
(Assignee)

Updated

10 years ago
Depends on: 473506
(Assignee)

Updated

10 years ago
Attachment #356743 - Attachment description: (Av1) summaryText() : support None value for leaked argument ++ → (Av1) summaryText() : support None value for leaked argument ++ [Checkin: Comment 6]
(Assignee)

Updated

10 years ago
Duplicate of this bug: 473762

Comment 9

10 years ago
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
Attachment #356887 - Flags: review?(ccooper) → review+

Updated

10 years ago
Attachment #356887 - Flags: checked‑in+ checked‑in+
(Assignee)

Updated

10 years ago
Attachment #356887 - Attachment description: (Bv1) Inherit from MozillaMochitest → (Bv1) Inherit from MozillaMochitest [Checkin: Comment 9]
(Assignee)

Comment 10

10 years ago
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
(Assignee)

Updated

9 years ago
Depends on: 478165
we use the brand_name to be able to distinguish from "firefox" or "shiretoko" for example, when launching the mac .app
(Assignee)

Updated

9 years ago
Depends on: 480034
(Assignee)

Updated

9 years ago
Depends on: 479225
No longer depends on: 480034
(Assignee)

Updated

9 years ago
Blocks: 482236
(Assignee)

Updated

9 years ago
Depends on: 445611
(Assignee)

Comment 12

9 years ago
Created attachment 368313 [details] [diff] [review]
(Cv1) Merge 'FAIL' handling into summaryText()

*Merge (and synchronize) 'FAIL' handling.
*s/emphasizeFailureText(summary)/emphasizeFailureText(failCountStr)/ (only).
*s/'browser'/self.name/ (== 'mochitest-browser-chrome').
Attachment #368313 - Flags: review?(ccooper)
(Assignee)

Comment 13

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

Cv1, with missing implicit case.
Attachment #368313 - Attachment is obsolete: true
Attachment #368319 - Flags: review?(ccooper)
Attachment #368313 - Flags: review?(ccooper)
(Assignee)

Comment 14

9 years ago
Created attachment 368383 [details] [diff] [review]
(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
Attachment #368383 - Flags: review?(ccooper)

Updated

9 years ago
Attachment #368319 - Flags: review?(ccooper) → review+
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?
Attachment #368383 - Flags: review?(ccooper) → review+
(Assignee)

Updated

9 years ago
Attachment #368319 - Attachment description: (Cv1a) Merge 'FAIL' handling into summaryText() → (Cv1a) Merge 'FAIL' handling into summaryText() [Checkin: Comment 16]
Attachment #368319 - Flags: checked‑in+ checked‑in+
(Assignee)

Comment 16

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


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

Comment 17

9 years ago
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
Attachment #368383 - Attachment description: (Dv1) Merge duplicated code in class MozillaMochitest → (Dv1) Merge duplicated code in class MozillaMochitest [Checkin: Comment 17]
Attachment #368383 - Flags: checked‑in+ checked‑in+
Cv1a and Dv1 are now running in production.
I see the whiteboard says 'leave the bug open', but I'm not sure why. Serge?
(Assignee)

Comment 20

9 years ago
Because there will be more patches ;->
(Assignee)

Updated

9 years ago
Depends on: 383136
(Assignee)

Updated

9 years ago
Depends on: 491784
(Assignee)

Comment 21

9 years ago
Created attachment 377340 [details] [diff] [review]
(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.
Attachment #377340 - Flags: review?(ccooper)
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).
Attachment #377340 - Flags: review?(ccooper) → review-
(Assignee)

Comment 23

9 years ago
Created attachment 377806 [details] [diff] [review]
(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...
Attachment #377340 - Attachment is obsolete: true
Attachment #377806 - Flags: review?(ccooper)

Updated

9 years ago
Attachment #377806 - Flags: review?(ccooper) → review+
Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

changeset:   308:e59f0ff8d308
Attachment #377806 - Flags: checked‑in+ checked‑in+
(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.
(Assignee)

Updated

9 years ago
Attachment #377806 - Attachment description: (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() → (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Checkin: Comment 24]
(Assignee)

Updated

9 years ago
Attachment #377806 - Attachment description: (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Checkin: Comment 24] → (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Checkin: See comment 24]
Depends on: 494220

Updated

9 years ago
Attachment #377806 - Flags: review-
Attachment #377806 - Flags: review+
Attachment #377806 - Flags: checked‑in-
Attachment #377806 - Flags: checked‑in+
Attachment #377806 - Flags: checked‑in- review-
Attachment #377806 - Flags: checked‑in+ review+
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.
(Assignee)

Updated

9 years ago
Depends on: 494220
(Assignee)

Updated

9 years ago
Attachment #377806 - Attachment description: (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Checkin: See comment 24] → (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Backout: Comment 26]
Attachment #377806 - Attachment is obsolete: true
(Assignee)

Comment 27

9 years ago
Created attachment 378992 [details] [diff] [review]
(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 :-<
Attachment #378992 - Flags: review?(ccooper)
Comment on attachment 378992 [details] [diff] [review]
(Ev2) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()

Patch is baking in staging right now.
Attachment #378992 - Flags: review?(ccooper) → review+
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.
Attachment #378992 - Flags: review+ → review-
(Assignee)

Comment 30

9 years ago
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...
Attachment #378992 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #377806 - Attachment is obsolete: false
Attachment #377806 - Flags: review?(ccooper) review?(ccooper)
Attachment #377806 - Flags: review-
Attachment #377806 - Flags: checked‑in-
Attachment #377806 - Flags: checked‑in- review-
(Assignee)

Comment 31

9 years ago
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.
Attachment #377806 - Flags: checked‑in? checked‑in?
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.

Updated

9 years ago
Attachment #377806 - Flags: review?(ccooper) → review+

Updated

9 years ago
Attachment #377806 - Flags: checked‑in? checked‑in? → checked‑in+ checked‑in+
Comment on attachment 377806 [details] [diff] [review]
(Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand()
[Checkin: See comment 41+42]

changeset:   315:1ad485bd9527
(Assignee)

Updated

9 years ago
Attachment #377806 - Attachment description: (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Backout: Comment 26] → (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Checkin: See comment 33]
(Assignee)

Updated

9 years ago
Depends on: 484231
Attachment #377806 - Flags: checked‑in+ checked‑in+
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).
(Assignee)

Comment 35

9 years ago
(In reply to comment #34)

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

Obviously, Tracemonkey misses bug 473506 fix... :-/
(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?
(Assignee)

Comment 37

9 years ago
(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
}
(Assignee)

Comment 38

9 years ago
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 :-|
Attachment #377806 - Flags: checked‑in? checked‑in?
(Assignee)

Comment 39

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

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

Updated

9 years ago
Attachment #377806 - Flags: checked‑in? checked‑in? → checked‑in+ checked‑in+
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
(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
(Assignee)

Updated

9 years ago
Attachment #377806 - Attachment description: (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Checkin: See comment 33] → (Ev1a) Sync' Mochitest and Reftest: summarizeLog() and evaluateCommand() [Checkin: See comment 41+42]
(Assignee)

Comment 43

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

(Nearly untested.)
Attachment #392006 - Flags: review?(ccooper)
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.
Attachment #392006 - Flags: review?(ccooper) → review+
(Assignee)

Comment 45

9 years ago
(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...
(Assignee)

Updated

9 years ago
Attachment #392006 - Flags: checked-in?
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
Attachment #392006 - Flags: checked-in? → checked-in+
(Assignee)

Updated

9 years ago
Attachment #392006 - Attachment description: (Fv1) Create and use summarizeLogXpcshelltests(), plus some internal nits → (Fv1) Create and use summarizeLogXpcshelltests(), plus some internal nits [Checkin: Comment 46]
(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.
Created attachment 395664 [details] [diff] [review]
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.
Attachment #395664 - Flags: review?(ccooper)
(Assignee)

Comment 49

9 years ago
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!

Updated

9 years ago
Attachment #395664 - Flags: review?(ccooper) → review+
(Assignee)

Comment 50

9 years ago
Created attachment 396114 [details] [diff] [review]
(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.
Attachment #396114 - Flags: review?(ccooper)

Updated

9 years ago
Attachment #396114 - Flags: review?(ccooper) → review+
(Assignee)

Updated

9 years ago
Attachment #396114 - Flags: checked-in?
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
Attachment #396114 - Flags: checked-in? → checked-in+
(Assignee)

Updated

9 years ago
Attachment #396114 - Attachment description: (Hv1) mochitest-browser-chrome: support Passed and Failed too → (Hv1) mochitest-browser-chrome: support Passed and Failed too [Checkin: Comment 51]
(Assignee)

Comment 52

9 years ago
Created attachment 402841 [details] [diff] [review]
(Iv1) mochitest-browser-chrome: support Passed and Failed too, 2 missed occurrences
[Checkin: Comment 57]
Attachment #402841 - Flags: review?(ccooper)

Updated

9 years ago
Attachment #402841 - Flags: review?(ccooper) → review+
(Assignee)

Updated

9 years ago
Attachment #402841 - Flags: checked-in?

Updated

9 years ago
Attachment #402841 - Flags: checked-in? → checked-in+
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
(Assignee)

Updated

9 years ago
Attachment #402841 - Attachment description: (Iv1) mochitest-browser-chrome: support Passed and Failed too, 2 missed occurrences → (Iv1) mochitest-browser-chrome: support Passed and Failed too, 2 missed occurrences [Checkin: Comment 53]
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.
Attachment #402841 - Flags: checked-in+ → checked-in-
(Assignee)

Comment 55

9 years ago
(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!)
I'll run with the patch in staging today to try to reproduce the failure.
(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.
(Assignee)

Updated

9 years ago
Attachment #402841 - Attachment description: (Iv1) mochitest-browser-chrome: support Passed and Failed too, 2 missed occurrences [Checkin: Comment 53] → (Iv1) mochitest-browser-chrome: support Passed and Failed too, 2 missed occurrences [Checkin: Comment 57]
(Assignee)

Updated

9 years ago
Attachment #402841 - Flags: checked-in- → checked-in+
(Assignee)

Comment 58

9 years ago
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

9 years ago
(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

9 years ago
(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.
(Assignee)

Comment 61

9 years ago
Created attachment 410917 [details] [diff] [review]
(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.
Attachment #410917 - Flags: review?(ccooper)
(Assignee)

Updated

9 years ago
Depends on: 527169
(Assignee)

Comment 62

9 years ago
(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.

Updated

9 years ago
Attachment #410917 - Flags: review?(ccooper) → review+
(Assignee)

Updated

9 years ago
Attachment #410917 - Flags: checked-in?

Updated

9 years ago
Attachment #410917 - Flags: checked-in? → checked-in+
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
(Assignee)

Updated

9 years ago
Attachment #395664 - Attachment description: fix parsing for mozmill tests → fix parsing for mozmill tests [Superseded by bug 512592]
Attachment #395664 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #410917 - Attachment description: (Jv1) mochitest-browser-chrome: remove Pass and Fail support now → (Jv1) mochitest-browser-chrome: remove Pass and Fail support now [Checkin: Comment 63]
Serge: Found this old bug during triage. Is there anything left to do here, or can we close this now?
Whiteboard: [// Leave bug open] → [triagefollowup][// Leave bug open]
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.