1091284 - Remove systemMemory, environment from automationutils

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Jonathan Griffin (:jgriffin)]

It looks like these aren't actually used; the only call sites which use environment() use the copy in automation.py.in.

Comment 1

[Jonathan Griffin (:jgriffin)]

(In reply to Jonathan Griffin (:jgriffin) from comment #0)
> It looks like these aren't actually used; the only call sites which use
> environment() use the copy in automation.py.in.

Whoops, that's wrong.  It is imported in a couple of places, at least:

http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#37
http://dxr.mozilla.org/mozilla-central/source/layout/tools/reftest/runreftest.py#28

Ideally we should move this to a mozbase package, but I'm not sure we have an appropriate one.  Do we need a new package to contain things like this?

Comment 2

[Jonathan Griffin (:jgriffin)]

This probably belongs in the same place as the leak log functions, wherever that turns out to be (bug 1091274).

Comment 3

[Julien Pagès (:parkouss)]

So, as far as I can understand from bug 1091274, it is suggested to create a mozrunner subclass inside moztest to put the leak logs functions in. So I suppose it should be the same here, putting environment() inside the same subclass.

I started to look at it, and it seems that it is pretty hard to inherit from mozrunner, as real runners are built on top of two distinct classes: DeviceRunner and GeckoRuntimeRunner. Plus it appears that sometimes the environment call does not fit into one or the other I think, like here for xpcshell:

https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#408


As a starting point, I suggest that we just move the function inside moztest (maybe moztest.utils or moztest.runner). It would be still possible after that to create subclasses and call that function if desired, but for now it would help in the process of removing automationutils.py, and I think it make sense to have a public function that just populate env vars.


Guys, what do you think ?

Comment 4

[Julien Pagès (:parkouss)]

Ok, so this was discussed a bit more on irc. :)

Actually :ted, :jgraham and :Ms2Ger agreed that a better place for that should be inside mozrunner. this makes sense to me also.

So I'd like to start with that: move the function somewhere in mozrunner as a start. We'll see later for some kind of better abstraction (like maybe inside a mozrunner subclass).

:jgriffin, does that makes sense to you ?

Comment 5

[Jonathan Griffin (:jgriffin)]

Yes, that makes sense, thanks Julien.

Comment 6

[Julien Pagès (:parkouss)]

Attachment: MozReview Request: Bug 1091284 - Remove systemMemory, environment from automationutils. r?jgriffin

Bug 1091284 - Remove systemMemory, environment from automationutils. r?jgriffin

Comment 7

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8628411 [details]
MozReview Request: Bug 1091284 - Remove systemMemory, environment from automationutils. r?jgriffin

https://reviewboard.mozilla.org/r/12373/#review10933

It's a bit ugly to shove this into mozrunner, but it doesn't have a more obvious home, and in the interests of getting rid of automationutils, let's do it.  In the future, if we turn the harnesses into real Python packages, we may find another home for this.

Comment 8

[Julien Pagès (:parkouss)]

Yes, I fully agree.

I ran a first push try yesterday, covering probably more than necessary:

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

After that I changed my mind to slightly change the log interface. So I ran another try, but only on one mochitest test:

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

All went good, so I'm confident (double checked) and I will land that (ie what was reviewed).

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d831b3b2a6f

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9d831b3b2a6f