888756 - Mozinstall's get_binary method for Mac should raise InvalidBinary Error

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Anhad Jai Singh (:ffledgling)]

Currently Mozinstall.get_binary does not raise a InvalidBinaryError if a binary is not found, instead it raises an AssertionError like so:

AssertionError: "/var/folders/fy/wr9qj9zj1kq6q121d2hz439w0000gn/T/tmppXLYhD/Contents/Info.plist" has not been found.

I think it should raise an InvalidBinaryError, like the it does for Windows and Linux, unless there is reason to do otherwise.

Comment 1

[Anhad Jai Singh (:ffledgling)]

Attachment: Patch

I'm attaching this patch in case the raising an Exception is indeed considered the correct thing to do. Incase it isn't this patch can be disregarded.

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 769517 [details] [diff] [review]
Patch

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

I'm fine with taking this patch. Technically the assertion only happens if Info.plist isn't found, an InvalidBinary exception can still be raised if there is no binary after that. But I agree with you, I think there should only be one exception that gets raised out of that method to make it easier for consumers to catch (and InvalidBinary is more readable than AssertionError). Thanks!

Comment 3

[Andrew Halberstadt [:ahal]]

Pushed: https://github.com/mozilla/mozbase/commit/625e4d754133e78d190c4f71b0204db8000c3ec6

Thanks for catching and fixing this!