812727 - make summary() an optional action, rather than a requirement for every mozharness script run

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Phil Ringnalda (:philor)]

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 :)

Comment 1

[Aki Sasaki (not active)]

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.

Comment 2

[Aki Sasaki (not active)]

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.

Comment 3

[Phil Ringnalda (:philor)]

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 4

[Ed Morley [:emorley]]

Agree with Philor

Comment 5

[Ed Morley [:emorley]]

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
}

Comment 6

[Aki Sasaki (not active)]

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.

Comment 7

[Aki Sasaki (not active)]

Attachment: (mozharness) make summary an action

* 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.

Comment 8

[Aki Sasaki (not active)]

Attachment: (configs) add --summary to android multilocale release

Comment 9

[Aki Sasaki (not active)]

Attachment: (custom) add --summary to android multilocale nightly

Comment 10

[Aki Sasaki (not active)]

Attachment: (configs) add --summary to android multilocale release + b2g desktop

Comment 11

[Rail Aliiev [:rail]]

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?

Comment 12

[Aki Sasaki (not active)]

(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

Comment 13

[Aki Sasaki (not active)]

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

Comment 14

[Aki Sasaki (not active)]

Comment on attachment 693166 [details] [diff] [review]
(custom) add --summary to android multilocale nightly

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

Comment 15

[Aki Sasaki (not active)]

Comment on attachment 691650 [details] [diff] [review]
(mozharness) make summary an action

http://hg.mozilla.org/build/mozharness/rev/68cc7bd93265

Comment 16

[Aki Sasaki (not active)]

This is in production.