1152864 - "unsafe CPOW usage" warnings are too noisy in automation test logs

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Our tree sheriffs often look at test logs for e10s, and unfortunately, many of our tests trigger unsafe CPOW usage warnings because they haven't been ported to use async messages yet.

Most of these have been filed as blocking bug 1109869, but in the mean time, our sheriffs have to deal with a ton of log spam, and that really sucks.

As an interim solution, we would like to investigate filtering out "unsafe CPOW usage" warnings from the log viewer. RyanVM has told me that this would be an OK interim solution until we drill bug 1109869 into the ground.

Comment 1

[Ed Morley [:emorley]]

I don't think this is something that we can easily do (or would want to do), particularly with the current log viewer. At some point in the future, when we use the json (aka structured) logs with a new log viewer, it can perhaps do collapsing by test or subtest/filtering/... - however even then, I don't think we'd want to hide things by default - it would be up to the user to filter each time - which doesn't really help this use case. In addition, it still leaves the full log and local dev console pretty cluttered.

As such, I think for now this is best dealt with at the console/logging/harness level instead - eg "only report the first N warnings for each file/type" or summarise them "55 warnings in file X". So this either belongs in an e10s component, or a testing/harness component. Another option would be to only show the warnings in local jobs, or to say ask the mozharness people to add another log level that we can get the log viewer to ignore.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Alternatively, and this is something RyanVM just suggested to me, we could disable all unsafe CPOW usage warnings for test runs except for a particular project branch (like Holly, which we already use for e10s tests, I believe).

What do you think of that last idea, jimm? RyanVM suspects that filtering out the unsafe CPOW usage entries from the log view the treeherder logviewer is non-trivial. Do you know if we have any run or build-time prefs that we can use to disable the warnings just for test runs?

Comment 3

[Jim Mathies [:jimm]]

I'm not aware of any test specific environment variable we currently set that could be used here, but you can add one.. check out testing/mochitest/runtests.py - 

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1456

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: bz://1152864/mconley (r+'d by jimm and ahal)

/r/6901 - Bug 1152864 - Add DISABLE_UNSAFE_CPOW_WARNINGS environment variable for our test harnesses. r=?
/r/6903 - Bug 1152864 - Disable unsafe CPOW warnings when running mochitests by default, and add option to re-enable. r=?

Pull down these commits:

hg pull -r 19243372d98892edb673d4855938b86020ef7428 https://reviewboard-hg.mozilla.org/gecko/

Comment 5

[Mike Conley (:mconley) (:⚙️)]

So this patch will do the job of suppressing all unsafe CPOW warnings when running mochitests, and adds a --enable-CPOW-warnings argument to the mach mochitest-* commands (and runtests.py) to turn them back on.

I tested this locally with browser/base/content/test/general/browser_bug575561.js as a test case, and the warnings are gone (unless I flip them on with --enable-CPOW-warnings).

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8590972 [details]
MozReview Request: bz://1152864/mconley (r+'d by jimm and ahal)

/r/6901 - Bug 1152864 - Add DISABLE_UNSAFE_CPOW_WARNINGS environment variable for our test harnesses. r=?
/r/6903 - Bug 1152864 - Disable unsafe CPOW warnings when running mochitests by default, and add option to re-enable. r=?

Pull down these commits:

hg pull -r 19243372d98892edb673d4855938b86020ef7428 https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/6903/#review5761

::: testing/mochitest/mach_commands.py
(Diff revision 1)
> +    enableCPOWWarnings = CommandArgument(
> +        '--enable-CPOW-warnings',
> +        action='store_true',
> +        help='Run tests with unsafe CPOW usage warnings enabled.')
> +    func = enableCPOWWarnings(func)

This is fine for now, but how common do you think enabling this locally is going to be? I'm trying to improve the UX of running tests a bit, and part of that will be cleaning rarely used options out of the command line. These rarer options will still be configurable.. just more out of the way.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

(In reply to Andrew Halberstadt [:ahal] from comment #7)
> https://reviewboard.mozilla.org/r/6903/#review5761
> 
> ::: testing/mochitest/mach_commands.py
> (Diff revision 1)
> > +    enableCPOWWarnings = CommandArgument(
> > +        '--enable-CPOW-warnings',
> > +        action='store_true',
> > +        help='Run tests with unsafe CPOW usage warnings enabled.')
> > +    func = enableCPOWWarnings(func)
> 
> This is fine for now, but how common do you think enabling this locally is
> going to be?

Very uncommon to never. The only use-case I can think of, to enable this for tests, is for the e10s team to sample the logs to determine if there are still places where we're using unsafe CPOWs. At this stage, I don't see us doing that very often, though that might change in the next quarter (Milestone 8 of e10s involves us drilling into a bunch of these unsafe CPOW usage warnings).

Comment 9

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/6903/#review5763

> This is fine for now, but how common do you think enabling this locally is going to be? I'm trying to improve the UX of running tests a bit, and part of that will be cleaning rarely used options out of the command line. These rarer options will still be configurable.. just more out of the way.

Ah, whoops, responded in the bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1152864#c8

Comment 10

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/6903/#review5765

Ok I'll likely hide this from the help in the near future, though it'll still be an option should the e10s team need it for debugging. In the meantime, this looks good to me!

Comment 11

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/6903/#review5807

Ship It!

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Thanks! I switched the argument to --enable-cpow-warnings as jimm suggested.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/93f617ada149
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40a21375b99b

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/93f617ada149
https://hg.mozilla.org/mozilla-central/rev/40a21375b99b

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1152864 - Add DISABLE_UNSAFE_CPOW_WARNINGS environment variable for our test harnesses. r=?

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1152864 - Disable unsafe CPOW warnings when running mochitests by default, and add option to re-enable. r=?