Open Bug 1591678 Opened 3 years ago Updated 6 months ago

task with message 'TEST-UNEXPECTED-FAIL | leakcheck | tab missing output line for total leaks!' set as successful


(Testing :: Mozbase, defect, P2)



(firefox72 fixed)

Tracking Status
firefox72 --- fixed


(Reporter: aryx, Assigned: ahal)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

Pushed by
add process name to regexps for harness errors to let their occurrences trigger the task to fail. r=jlund
Closed: 3 years ago
Resolution: --- → FIXED
See Also: → 1535922

Can somebody please look at fixing this? This failure is still not turning jobs orange. Thanks.

The green jobs here are an example of this:

Fortunately a sheriff noticed that this was happening so we were able to back out my patch that caused it, but it would be better to not have to rely on that. It would also be harder to notice if the failure to create a log was only happening in some test directories instead of in all of them.

Flags: needinfo?(aryx.bugmail)
See Also: → 1649331

See the 'Failure Summary' tab for this task as example.

For missing output line for total leaks!, self.leaked gets set to false (src. In other words, nothing tracks it as an error and the logic to set the task as failure will keep the default TBPL_SUCCESS.

Shall in this case self.num_errors be set and self.tbpl_status be set based on that?

Flags: needinfo?(aryx.bugmail) → needinfo?(gbrown)

I don't recall ever being involved in mozharness leak reporting -- I may not know what I'm talking about.

I note the harness_error_re is defined by Reading through, I think the intent has been:

    if "negative leaks caught!" or "\d+ bytes leaked" is found on a "TEST-UNEXPECTED-FAIL" line:
         self.leaked = True
   elif "missing output line for total leaks!" is found on a "TEST-UNEXPECTED-FAIL" line:
         self.leaked = None
         self.leaked = False

With, "<something> missing output line for total leaks!" is now treated the same as "missing output line for total leaks!" was originally - that seems right.

I don't see any existing code that links self.leaked to success/failure of the mozharness run.

I don't think self.num_errors should be modified for leaks.

If we want to fail, I think this is sufficient:
self.tbpl_status = self.worst_level(TBPL_FAILURE, self.tbpl_status, levels=TBPL_WORST_LEVEL_TUPLE)
but I note that we do not currently do that for other leak conditions, nor for crashes (which seems odd/wrong).

Flags: needinfo?(gbrown)

Andrew, can you provide insight into the reasoning for the current behavior of the leak reporting and guide how it shall be done?

Flags: needinfo?(ahal)

It's also been a long time since I've looked at mozharness log parsing and status setting, but those regexes should only be used for unstructured output. The mochitest harness uses mozlog actions to handle leak messages:

I believe this is a bug in mozlog's StatusHandler here:

Basically we default the bytes value to 0, though according to the other formatters, a None value means the total line is missing. I'll put up a patch shortly, though may be a little bit before I can test it. I'll leave the needinfo to remind myself.

Assignee: nobody → ahal
Flags: needinfo?(ahal)
Severity: normal → S2
Component: Applications: MozharnessCore → Mozbase
Priority: -- → P2
Product: Release Engineering → Testing
QA Contact: jlund
Pushed by
[mozlog] Ensure missing leak totals count as a failure in the StatusHandler, r=gbrown

Thank you for fixing this!

Closed: 3 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Regressions: 1651716
Resolution: FIXED → ---
No longer regressions: 1651716

Any interest in the remaining unlanded patch?

Flags: needinfo?(ahal)

Yeah, I think it's still something we'd want. Though if I'm remembering properly fixing the thing that caused the backout was going to be tricky. There should be more context in bug 1651716 (I see there have been many comments since the backout that I haven't been following though).

Flags: needinfo?(ahal)
Duplicate of this bug: 1733138
Attachment #9162304 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.