1076969 - Make setting options.leakThresholds and options.ignoreMissingLeaks defaults less fragile

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Andrew McCreight [:mccr8]]

I was missing two places where this had to be done, plus there was an additional place where I wasn't setting them correctly at all.  It is also scary because it apparently fails without turning the tree orange when you forget to do it, which could potentially cause us to hide leaks.

I think the easiest way to fix this is to change
  options.leakThresholds to options.get("leakThresholds", {})
  options.ignoreMissingLeaks to options.get("ignoreMissingLeaks", [])

or something like that.  Then if somebody forgets to set it, we default to the strictest setting.  We'd also want to rip out the existing code that just uses these default values.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: processLeakLog should come up with reasonable defaults itself.

Instead of grabbing attributes off options at every call site, pass
in the options object to processLeakLog, and attempt to get the attributes
there. If not present, use a restrictive default value.

This will prevent silent harness failures if one of the many ways to invoke
processLeakLog fails to set up these options, and makes it so they
don't have to set it up if they don't care.

try run: https://tbpl.mozilla.org/?tree=Try&rev=e1c2327be395

I checked that Android, Linux and B2G don't throw in Python like they did with older stuff I had.

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8500548 [details] [diff] [review]
processLeakLog should come up with reasonable defaults itself.

Review of attachment 8500548 [details] [diff] [review]:
-----------------------------------------------------------------

this patch seems to be much more complete.  I know this is a frustrating system to work within, so many different variations of the same thing.

::: layout/tools/reftest/runreftest.py
@@ +510,5 @@
>          self.error("cannot specify focusFilterMode with parallel tests")
>        if options.debugger is not None:
>          self.error("cannot specify a debugger with parallel tests")
>  
>      options.leakThresholds = {"default": options.defaultLeakThreshold}

do we still need this instance of leakThreadholds?

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Joel Maher (:jmaher) from comment #2)
> this patch seems to be much more complete.  I know this is a frustrating
> system to work within, so many different variations of the same thing.

I think I'm starting to learn where all the places are, so hopefully it will be a little easier for me in the future. ;)

> >      options.leakThresholds = {"default": options.defaultLeakThreshold}
> do we still need this instance of leakThreadholds?

Yeah, if in theory somebody actually set this command line variable, then we need to set this.

Comment 4

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b74418f4245

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b74418f4245