600413 - Make mochitest-browser-chrome logging consistent with other mochitest types

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Jonathan Griffin (:jgriffin)]

Currently mochitest-browser-chrome logs some things a little differently than other mochitest types, which is a pain for log parsers.

E.g.

Browser Chrome Test Summary
	Passed: 9848
	Failed: 0
	Todo: 7

instead of

INFO TEST-START | Shutdown
INFO Passed: 486
INFO Failed: 0
INFO Todo:   0

and

TEST-INFO | chrome://mochikit/content/browser/toolkit/mozapps/plugins/tests/browser_bug435788.js | Test took 5.171s to complete

instead of

INFO SimpleTest finished /tests/modules/plugin/test/test_twostreams.html in 180ms

These should be made to be consistent.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest patch

This patches browser-chrome logging to more closely conform to regular mochitest standards, so that the logparser in bug 601216 doesn't have to special-case browser-chrome logs.

Comment 2

[cmtalbert]

Comment on attachment 480316 [details] [diff] [review]
mochitest patch

Looks good.  I am concerned about downstream fallout of this patch.  So, I propose we do the following:

1. Run it on try server (with only a mochitest-other test specified)
2. File a follow-on RelEng bug with a simple patch to fix the buildbot log parsing code: http://mxr.mozilla.org/build/source/buildbotcustom/steps/unittest.py#119
3. Add a maemkit patch to ensure that picks up the necessary changes as well (see jmaher for help with hunting down the maemkit code).
4. Flag this bug to land on all active branches (1.9.2, 1.9.1).

Once we have the buildbot changes landed (that will require a downtime), we can take this patch on m-c, 1.9.2, 1.9.1.  You'll probably want to make sure the patch will apply cleanly to the 1.9.2 and 1.9.1 trees and attach specific versions of the patch for those branches if it doesn't apply.

Thanks so much for helping make our logs sane.

Comment 3

[cmtalbert]

Christian,

Due to the fact that buildbot does not branch per tree, we will need to land this change on all active security branches.  Jgriffin will create patches for those branches, but can I get approval for him to land there since this is in no way a part of the build on those branches?  Or is approval even needed for this since it doesn't affect the products being built from those branches?  Thanks

Comment 4

[(not currently active) Ted Mielczarek]

Historically we have not required approval for test/test harness changes.

Comment 5

[Jonathan Griffin (:jgriffin)]

Is it actually possible to guarantee that a change lands in the buildbot parser, + m-c, + 1.9.1 + 1.9.2, at the same time?  It seems that if these land separately, they'll break tinderbox log parsing.

To avoid this potential problem, perhaps we should land a patch on all the branches which adds new logging in addition to the old logging, then patch the buildbot parser, then a followup patch to the branches which removes the obsolete logging.

Comment 6

[cmtalbert]

(In reply to comment #5)
> Is it actually possible to guarantee that a change lands in the buildbot
> parser, + m-c, + 1.9.1 + 1.9.2, at the same time?  It seems that if these land
> separately, they'll break tinderbox log parsing.
> 
> To avoid this potential problem, perhaps we should land a patch on all the
> branches which adds new logging in addition to the old logging, then patch the
> buildbot parser, then a followup patch to the branches which removes the
> obsolete logging.
Sounds like a much better way forward.  Let's do that.

Comment 7

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest logging patch v0.2

This is a better patch, which I don't think will require any buildbot changes.  Baking on tryserver now to verify.

Comment 8

[Jonathan Griffin (:jgriffin)]

Comment on attachment 482727 [details] [diff] [review]
mochitest logging patch v0.2

This patch passes on tryserver, and doesn't require any buildbot changes.  It modifies both 'regular' mochitest and browser-chrome, introducing a TEST-END 'status' type that contains the duration of a test.

Comment 9

[cmtalbert]

Comment on attachment 482727 [details] [diff] [review]
mochitest logging patch v0.2

This looks good.  I'd still like to change the results output of browser-chrome one of these days to match the other test frameworks, but let's just file a follow-on bug for that.

Comment 10

[Jonathan Griffin (:jgriffin)]

Pushed to m-c as http://hg.mozilla.org/mozilla-central/rev/b1d1e0873373

Comment 11

[Dave Townsend [:mossop]]

Attachment: bustage fix

This broke manually running browser-chrome tests, this seems to be the fix

Comment 12

[Dave Townsend [:mossop]]

Landed the fix: http://hg.mozilla.org/mozilla-central/rev/e4e3f54c851a

Comment 13

[Jonathan Griffin (:jgriffin)]

Attachment: 1.9.2 mochitest patch

Here's a patch for 1.9.2. It does something similar, but the patch itself is different, as there are quite a few differences in browser-test between m-c and 1.9.2.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Can we just merge all of the harness files from trunk to 1.9.2? Hopefully none of the changes since then depend on other mozilla-central changes...

Comment 15

[(not currently active) Ted Mielczarek]

It can be tricky, getting all the dependencies right is hard. There can be platform dependencies, build system dependencies, etc.

Comment 16

[:Gavin Sharp [email: [e-mail]]]

I think the browser-chrome harness itself hasn't changed much. I'll give it a shot and see what happens!

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 485445 [details] [diff] [review]
1.9.2 mochitest patch

Johnathan, do we still want to do this? Not sure that updating 1.9.2 is worth the effort at this point. Ridiculous of me to keep this in my queue for so long - sorry about that.

Comment 18

[Jonathan Griffin (:jgriffin)]

Comment on attachment 485445 [details] [diff] [review]
1.9.2 mochitest patch

No, I don't think we need this patch any longer.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Is this still tracking something useful?

Comment 21

[Jonathan Griffin (:jgriffin)]

I'd say it's been resolved as much as we need it to be.