1091274 - Move leak log functions out of automationutils and into mozbase

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jonathan Griffin (:jgriffin)]

We should move all the leak log related methods from automationutils into a mozbase module, probably mozrunner.  Any other opinions?

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Bug 1087474 and related discussion might suggest moztest as a candidate.

Comment 2

[Jonathan Griffin (:jgriffin)]

(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?

Comment 3

[(not currently active) Ted Mielczarek]

We had discussed making a Mozrunner subclass, maybe "TestRunner" and putting it in moztest as a place to hang common harness functionality off of.

Comment 4

[Andrew Halberstadt [:ahal] (PTO Until Apr 3rd)]

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.

Comment 5

[Julien Pagès (:parkouss)]

Attachment: 1091274.patch

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.

Comment 6

[Julien Pagès (:parkouss)]

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 7

[Julien Pagès (:parkouss)]

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.

Comment 8

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1091274 - Move leak log functions out of automationutils and into mozbase. r?jgriffin

Bug 1091274 - Move leak log functions out of automationutils and into mozbase. r?jgriffin

Comment 9

[Julien Pagès (:parkouss)]

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 10

[Jonathan Griffin (:jgriffin)]

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.

Comment 11

[Julien Pagès (:parkouss)]

Pushed to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1255077fe5a0

Comment 12

[Julien Pagès (:parkouss)]

Some fails because of undefined variable in reftest - fixed and re pushed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0049826035c5

Comment 13

[Julien Pagès (:parkouss)]

Attachment: bug1091274.patch

carrying r+ from the previous review done by :jgriffin. the try push was good.

Tree is closed, so I will ask for checkin-needed.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/450f972140de

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/450f972140de