Remove systemMemory, environment from automationutils

RESOLVED FIXED in Firefox 42

Status

Testing
General
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: jgriffin, Assigned: parkouss, Mentored)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

User Story

Delete the systemMemory() and environment() methods from http://dxr.mozilla.org/mozilla-central/source/build/automationutils.py

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

4 years ago
User Story: (updated)
(Reporter)

Comment 1

4 years ago
(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?
Whiteboard: [good first bug][lang=python]
(Reporter)

Comment 2

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

Comment 3

3 years ago
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 ?
(Assignee)

Comment 4

3 years ago
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 ?
Flags: needinfo?(jgriffin)
(Reporter)

Comment 5

3 years ago
Yes, that makes sense, thanks Julien.
Flags: needinfo?(jgriffin)
(Assignee)

Comment 6

3 years ago
Created attachment 8628411 [details]
MozReview Request: Bug 1091284 - Remove systemMemory, environment from automationutils. r?jgriffin

Bug 1091284 - Remove systemMemory, environment from automationutils. r?jgriffin
Attachment #8628411 - Flags: review?(jgriffin)
(Reporter)

Comment 7

3 years ago
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.
Attachment #8628411 - Flags: review?(jgriffin) → review+
(Assignee)

Comment 8

3 years ago
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).
https://hg.mozilla.org/mozilla-central/rev/9d831b3b2a6f
Assignee: nobody → j.parkouss
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.