Closed Bug 1091274 Opened 5 years ago Closed 4 years ago

Move leak log functions out of automationutils and into mozbase

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jgriffin, Assigned: parkouss)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

We should move all the leak log related methods from automationutils into a mozbase module, probably mozrunner.  Any other opinions?
Bug 1087474 and related discussion might suggest moztest as a candidate.
(In reply to Chris Manchester [:chmanchester] from comment #1)
> Bug 1087474 and related discussion might suggest moztest as a candidate.

Hmm, moztest is currently just a result container; this (and the stuff discussed in bug 1087474) are fairly orthogonal to that.  Maybe we need a new module, ala mozautomation?
We had discussed making a Mozrunner subclass, maybe "TestRunner" and putting it in moztest as a place to hang common harness functionality off of.
Yeah, I think originally moztest was going to be the integration layer for many of the other mozbase modules into something resembling a bare bones test harness. The only reason it contains results is because that's what Mihnea started with, then priorities changed and/or he ran out of time and we never pursued it since. If we *really* wanted results to live in their own module, I'd vote we make mozresults.. but I think that is unnecessary.
Depends on: 1087474
Attached patch 1091274.patch (obsolete) — Splinter Review
Hm, not sure if I should ask you for a feedback here Jonathan or someone else - feel free to redirect the feedback. :)

As suggested, this is a first attempt in leak log function integration into mozrunner subclasses.
Attachment #8628505 - Flags: feedback?(jgriffin)
Well more exacly this add a new method "process_leak_log" on BaseRunner, so it should be available on all runner subclasses. Tell me what you think about it, and how you would like to see it improved if needed - I know for example that this may fail due to a get_default_logger() that can return None, but I'm not sure how to get around this now.

Anyway, feedback will be much appreciated!
Comment on attachment 8628505 [details] [diff] [review]
1091274.patch

Eh, we are going in a different way!

I asked :jgraham to look at this, and he found it looked like mozcrash. So, we are going to create a new mozleak package and use that (ala mozcrash).

Steps for the new proposal:

1. create mozleak and move the functions into mozleak
2. use that, removing old stuff around.
3. create an appropriate structured logger to report leaks in a standardized way.

It seems like :ted agreed on this. So I hope we will stay on this road (hehe)!

Well it seems like the best approach we found until now, and I would be surprised if this time somebody complained about it. ;)

So 1 and 2 may be handled with this bug (I think), and 3 should be a followup bug.
Attachment #8628505 - Flags: feedback?(jgriffin)
Assignee: nobody → j.parkouss
Status: NEW → ASSIGNED
Bug 1091274 - Move leak log functions out of automationutils and into mozbase. r?jgriffin
Attachment #8629219 - Flags: review?(jgriffin)
Hm, I added 'mozleak' into a bunch of files that I suppose are required to take the new mozleak package into account. It is based on a dxr "moztest" search.
Comment on attachment 8629219 [details]
MozReview Request: Bug 1091274 - Move leak log functions out of automationutils and into mozbase. r?jgriffin

https://reviewboard.mozilla.org/r/12537/#review11141

Looks good; we definitely should have a try run here first.
Attachment #8629219 - Flags: review?(jgriffin) → review+
Some fails because of undefined variable in reftest - fixed and re pushed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0049826035c5
Attached patch bug1091274.patchSplinter Review
carrying r+ from the previous review done by :jgriffin. the try push was good.

Tree is closed, so I will ask for checkin-needed.
Attachment #8628505 - Attachment is obsolete: true
Attachment #8629219 - Attachment is obsolete: true
Attachment #8630511 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/450f972140de
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.