mozrunner.Runner has firefox-specific logic in the generic runner class

RESOLVED FIXED

Status

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: k0scist, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Comment 1

7 years ago
Created attachment 568823 [details] [diff] [review]
this does not work

So this does not work :( The way I have it, I get

TypeError: find_binary() takes exactly 1 argument (2 given)

Of course, if instead i make it `binary = Runner.find_binary()`, I get

AttributeError: type object 'Runner' has no attribute 'names'

I'm not sure if python has a good solution for this.  I'll look, but maybe the thing to do is just remove these conveniences for windows
(Reporter)

Comment 2

7 years ago
Created attachment 568958 [details] [diff] [review]
genericize a bit

So I took a different approach and made everything class-generic.  MozRunner is a bit overcomplicated and underdocumented as to how it finds the binary and in the future we should try to figure out what of this we really care about keeping.  In the meantime, this clears up a little of the mess, I think
Attachment #568823 - Attachment is obsolete: true
Attachment #568958 - Flags: review?(ahalberstadt)
(Reporter)

Comment 3

7 years ago
Note that I have only tested this on unix
Comment on attachment 568958 [details] [diff] [review]
genericize a bit

>     profile_class = FirefoxProfile
>+    program_names = ['Mozilla Firefox', 'Minefield']

We don't have Minefield anymore. Instead we have to use 'Nightly', and 'Aurora'. Also is 'Mozilla Firefox' correct here? Shouldn't it be 'Firefox' only?
Comment on attachment 568958 [details] [diff] [review]
genericize a bit

Review of attachment 568958 [details] [diff] [review]:
-----------------------------------------------------------------

r+ on the condition that you address Henrik's comment.

Although looking at the various win32 zip files on ftp://ftp.mozilla.org/pub/firefox/tinderbox-builds/ , it seems that all channels extract to 'firefox' by default. Although I guess it doesn't hurt to have more names than necessary.
Attachment #568958 - Flags: review?(ahalberstadt) → review+
I just had another thought, though this isn't exactly related to this bug, it might be a good time to add a 'fennec' subclass to mozrunner. We'll probably need a bunch of fennec specific functionality in the future and now would be a good time to start updating our mozbase packages to support this. According to fmo windows fennec builds extract to 'fennec' by default.
(Reporter)

Comment 7

7 years ago
pushed to master: https://github.com/mozilla/mozbase/commit/3987506ac8ff4c759cd3a74f8c57be38765880e3

I took out Minefield but did not add anything.  Personally, I'd like to get rid of almost all the logic except findInPath.  You can always point directly to a binary on any operating system, and the convenience functionality is at best undocumented at confusing.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 8

7 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #6)
> I just had another thought, though this isn't exactly related to this bug,
> it might be a good time to add a 'fennec' subclass to mozrunner. We'll
> probably need a bunch of fennec specific functionality in the future and now
> would be a good time to start updating our mozbase packages to support this.
> According to fmo windows fennec builds extract to 'fennec' by default.

Off topic, though worthy of a new bug.  IIRC, desktop fennec may be going away soon anyway.
You need to log in before you can comment on or make changes to this bug.