Closed
Bug 812727
Opened 12 years ago
Closed 12 years ago
make summary() an optional action, rather than a requirement for every mozharness script run
Categories
(Release Engineering :: Applications: MozharnessCore, defect)
Release Engineering
Applications: MozharnessCore
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: philor, Assigned: mozilla)
References
Details
(Whiteboard: [mozharness])
Attachments
(3 files, 1 obsolete file)
(deleted),
patch
|
rail
:
review+
mozilla
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
mozilla
:
checked-in+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
rail
:
review+
mozilla
:
checked-in+
|
Details | Diff | Splinter Review |
https://tbpl.mozilla.org/php/getParsedLog.php?id=17123132&tree=Cedar is a typical crash in a mozharness run, with the useful lines that tbpl knows to highlight
18:16:14 WARNING - TEST-UNEXPECTED-FAIL | /home/cltbld/talos-slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug412945.js | test failed (with xpcshell return code: -11), see following log:
and
18:16:20 ERROR - PROCESS-CRASH | /home/cltbld/talos-slave/test/build/tests/xpcshell/tests/netwerk/test/unit/test_bug412945.js | application crashed (minidump found)
but then at the end of the run we get
18:28:18 ERROR - Return code: 1
18:28:18 INFO - TinderboxPrint: xpcshell-xpcshell<br/>1578/<em class="testfail">1</em>
18:28:18 ERROR - # TBPL FAILURE #
18:28:18 ERROR - The xpcshell suite: xpcshell ran with return status: FAILURE
18:28:18 INFO - #####
18:28:18 INFO - ##### DesktopUnittest summary:
18:28:18 INFO - #####
18:28:18 ERROR - # TBPL FAILURE #
18:28:18 ERROR - The xpcshell suite: xpcshell ran with return status: FAILURE
where the return code is moderately unlikely to be worth highlighting (in the case of mochitests, it looks to be the same exit code that the test failure line reports, in the case of xpcshell it's not even that), and the doubled "# TBPL FAILURE #" and "The xpcshell suite: xpcshell ran with return status: FAILURE" are completely without interest - if it wasn't a failure, nobody would be looking at it :)
Assignee | ||
Comment 1•12 years ago
|
||
This is code that's oriented towards humans running the scripts rather than automation.
Since the ERRORs can happen in the middle of hundreds of screens of output, anything marked as a summary message is repeated at the end.
Thinking about marking this ASDESIGNED.
Assignee | ||
Comment 2•12 years ago
|
||
As designed.
I wonder if there's a way we can get tbpl's parsing to stop at the ##### ScriptName summary: line. That might be ugly, though.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Reporter | ||
Comment 3•12 years ago
|
||
Why is it prefixed with "ERROR -" (or "WARNING -" in the case of test failures, though as long as tbpl can avoid having to highlight them, they don't bother me as much) if it's for humans?
If it's just information for humans to read at the bottom of their run, INFO would work as well, and if they are parsing out ERROR or WARNING, then they've already seen the actual ones, and don't need to be told the exact same thing twice over.
I'm entirely in favor of having a really easy to read thing that will tell you whether you need to dig back through the run you just did to see what went wrong, but being told "# TBPL FAILURE #" which sounds like tbpl is broken, and being told that the suite failed *twice* in the space of six lines, does not sound like it's designed for humans. If anything, it just makes it even harder to see the actual DesktopUnittest summary, which is the 1578/<em class="testfail">1</em>, not the word FAILURE or WARNING. If you tell me WARNING, that doesn't even make me think one of my tests failed, it makes me think that there were warnings, those non-fatal things of which there are hundreds or thousands in every test run.
Comment 5•12 years ago
|
||
We're also showing "xpcshell-xpcshell", which I guess is due to having to cater for mochitest-other etc, but seems a bit redundant.
Instead of:
{
18:28:18 ERROR - Return code: 1
18:28:18 INFO - TinderboxPrint: xpcshell-xpcshell<br/>1578/<em class="testfail">1</em>
18:28:18 ERROR - # TBPL FAILURE #
18:28:18 ERROR - The xpcshell suite: xpcshell ran with return status: FAILURE
18:28:18 INFO - #####
18:28:18 INFO - ##### DesktopUnittest summary:
18:28:18 INFO - #####
18:28:18 ERROR - # TBPL FAILURE #
18:28:18 ERROR - The xpcshell suite: xpcshell ran with return status: FAILURE
}
How about:
{
18:28:18 INFO - Return code: 1
18:28:18 INFO - #####
18:28:18 INFO - ##### DesktopUnittest summary:
18:28:18 INFO - #####
18:28:18 INFO - The xpcshell suite: xpcshell ran with return status: FAILURE
18:28:18 INFO - TinderboxPrint: xpcshell<br/>1578/<em class="testfail">1</em>
}
and if the "enable tinderbox output" mozharness option is not specified, then we would instead display the final line as:
{
18:28:18 INFO - Run: 1578 / Failed: 1
}
Assignee | ||
Comment 6•12 years ago
|
||
Thought about this a bit, and remembered that mozharness has a built-in solution to run certain actions in one environment (e.g., run locally) vs another (e.g. production).
Changing the summary.
Summary: Mozharness crashes include unhelpful repeated "ERROR - " lines → make summary() an optional action, rather than a requirement for every mozharness script run
Whiteboard: [mozharness]
Assignee | ||
Comment 7•12 years ago
|
||
* Make summary an action that needs to be added per-script as needed
* Add summary action to appropriate scripts
* Change add_summary() to log() or info() where a summary isn't needed
passes unit.sh
Verified this works via configtest.py.
Todo: add buildbot-configs/custom changes to pass --summary to multilocale steps for android + desktop b2g, do more testing.
Assignee | ||
Comment 8•12 years ago
|
||
Assignee | ||
Comment 9•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → aki
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #693165 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #691650 -
Flags: review?(rail)
Assignee | ||
Updated•12 years ago
|
Attachment #693166 -
Flags: review?(rail)
Assignee | ||
Updated•12 years ago
|
Attachment #693954 -
Flags: review?(rail)
Updated•12 years ago
|
Attachment #691650 -
Flags: review?(rail) → review+
Comment 11•12 years ago
|
||
Comment on attachment 693166 [details] [diff] [review]
(custom) add --summary to android multilocale nightly
Review of attachment 693166 [details] [diff] [review]:
-----------------------------------------------------------------
::: process/factory.py
@@ +977,5 @@
> self.mozharnessMultiOptions = mozharnessMultiOptions
> else:
> + self.mozharnessMultiOptions = ['--pull-locale-source',
> + '--add-locales',
> + '--package-multi',
Why did you change --only prefixed actions to ones without it? Was "summary" the only reason we used --only here?
Assignee | ||
Comment 12•12 years ago
|
||
(In reply to Rail Aliiev [:rail] from comment #11)
> Comment on attachment 693166 [details] [diff] [review]
> (custom) add --summary to android multilocale nightly
>
> Review of attachment 693166 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: process/factory.py
> @@ +977,5 @@
> > self.mozharnessMultiOptions = mozharnessMultiOptions
> > else:
> > + self.mozharnessMultiOptions = ['--pull-locale-source',
> > + '--add-locales',
> > + '--package-multi',
>
> Why did you change --only prefixed actions to ones without it? Was "summary"
> the only reason we used --only here?
I've been thinking about getting rid of the --only for a while now.
We take both options:
http://hg.mozilla.org/build/mozharness/file/a4518e40c78d/mozharness/base/config.py#l253
Updated•12 years ago
|
Attachment #693166 -
Flags: review?(rail) → review+
Updated•12 years ago
|
Attachment #693954 -
Flags: review?(rail) → review+
Assignee | ||
Comment 13•12 years ago
|
||
Comment on attachment 693954 [details] [diff] [review]
(configs) add --summary to android multilocale release + b2g desktop
http://hg.mozilla.org/build/buildbot-configs/rev/5b9916f6732e
Attachment #693954 -
Flags: checked-in+
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 693166 [details] [diff] [review]
(custom) add --summary to android multilocale nightly
http://hg.mozilla.org/build/buildbotcustom/rev/e6d24fdf361c
Attachment #693166 -
Flags: checked-in+
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 691650 [details] [diff] [review]
(mozharness) make summary an action
http://hg.mozilla.org/build/mozharness/rev/68cc7bd93265
Attachment #691650 -
Flags: checked-in+
Assignee | ||
Comment 16•12 years ago
|
||
This is in production.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Product: mozilla.org → Release Engineering
Updated•10 years ago
|
Component: General Automation → Mozharness
You need to log in
before you can comment on or make changes to this bug.
Description
•