Closed Bug 600413 Opened 11 years ago Closed 7 years ago

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

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jgriffin, Unassigned)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Status: NEW → ASSIGNED
Blocks: waronorange
Attached patch mochitest patch (obsolete) — Splinter Review
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.
Attachment #480316 - Flags: review?
Attachment #480316 - Flags: review? → review?(ctalbert)
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.
Attachment #480316 - Flags: review?(ctalbert) → review+
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
Historically we have not required approval for test/test harness changes.
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.
(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.
This is a better patch, which I don't think will require any buildbot changes.  Baking on tryserver now to verify.
Attachment #480316 - Attachment is obsolete: true
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.
Attachment #482727 - Flags: review?(ctalbert)
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.
Attachment #482727 - Flags: review?(ctalbert) → review+
Attached patch bustage fixSplinter Review
This broke manually running browser-chrome tests, this seems to be the fix
Assignee: jgriffin → nobody
Component: Mochitest → BrowserTest
QA Contact: mochitest → browsertest
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.
Attachment #485445 - Flags: review?
Attachment #485445 - Flags: review? → review?(gavin.sharp)
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...
It can be tricky, getting all the dependencies right is hard. There can be platform dependencies, build system dependencies, etc.
I think the browser-chrome harness itself hasn't changed much. I'll give it a shot and see what happens!
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 on attachment 485445 [details] [diff] [review]
1.9.2 mochitest patch

No, I don't think we need this patch any longer.
Attachment #485445 - Flags: review?(gavin.sharp)
Is this still tracking something useful?
Flags: needinfo?(jgriffin)
Duplicate of this bug: 473506
I'd say it's been resolved as much as we need it to be.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(jgriffin)
Resolution: --- → FIXED
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.