Closed Bug 1076969 Opened 5 years ago Closed 5 years ago

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

Categories

(Testing :: General, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: mccr8, Assigned: mccr8)

Details

(Whiteboard: [MemShrink])

Attachments

(1 file)

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.
Assignee: nobody → continuation
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.
Attachment #8500548 - Flags: review?(jmaher)
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?
Attachment #8500548 - Flags: review?(jmaher) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/1b74418f4245
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.