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

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

https://github.com/mozilla/mozbase/blob/master/mozrunner/mozrunner/runner.py#L150

This should go in FirefoxRunner

Comment 1

[Jeff Hammel]

Attachment: 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

Comment 2

[Jeff Hammel]

Attachment: 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

Comment 3

[Jeff Hammel]

Note that I have only tested this on unix

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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 5

[Andrew Halberstadt [:ahal]]

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.

Comment 6

[Andrew Halberstadt [:ahal]]

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.

Comment 7

[Jeff Hammel]

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.

Comment 8

[Jeff Hammel]

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