mozversion should throw some kind of exception if binary specified and nothing found

RESOLVED FIXED in mozilla36

Status

Testing
Mozbase
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: wlach, Assigned: parkouss, Mentored)

Tracking

Trunk
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good next bug])

Attachments

(1 attachment, 1 obsolete attachment)

If we're passing a binary to mozversion's get_version method (http://mozbase.readthedocs.org/en/latest/mozversion.html#mozversion.get_version), we should throw an exception if we can't get version info as it indicates something happened which we didn't expect. 

The current behaviour is to assume in this case that we want b2g version info, which doesn't make a lot of sense and will generate confusing errors.

This would not be a terrible next bug for someone at least a little bit familiar with mozbase (https://wiki.mozilla.org/Auto-tools/Projects/MozBase#Installing_Mozbase_for_Development). The logic you'd need to change is around here:

http://hg.mozilla.org/mozilla-central/file/4f2cac8d72da/testing/mozbase/mozversion/mozversion/mozversion.py#l262
See also bug 1064811, where I discussed this with Dave. Maybe we should really change something here?
Flags: needinfo?(dave.hunt)
I agree that if a binary is passed and we're unable to find the binary or determine the version details that we should not fallback to remote B2G. The binary is omitted for a remote B2G instance.
Flags: needinfo?(dave.hunt)
It seems like the previous bug got hung up on what would happen with the cli program. I'm really not worried about that at all, the important thing is that the behaviour of mozversion inside a test harness is predictable and understandable.
(Assignee)

Comment 4

3 years ago
Hey,

If I understand well, you want to throw an exception (say LocalAppNotFoundError) if binary is not None and that no valid local version is found.

However, looking at the code, I can see that some LovalVersion class handle the case where binary is None by looking in os.getcwd (example here http://hg.mozilla.org/mozilla-central/file/4f2cac8d72da/testing/mozbase/mozversion/mozversion/mozversion.py#l99).

Does that mean that you want to make this feature obsolete ? Say:

 - if binary is not None, look in LocalVersions and raise errors if needed
 - else look for RemoteB2GVersion

If you want to keep the feature to get local version from a given dir without passing a binary, maybe we can add a *binary_path* argument:

 - if binary or binary_path is given, look in LocalVersions and raise errors if needed (LocalFennecVersion will only use binary I suppose)
 - else look for RemoteB2GVersion

It will be easy to get versions from current dir:

mozversion.get_version(binary_path='.')

And in the same time the code will never have to check for RemoteB2GVersion and can safely raise good exceptions on errors. (RemoteB2GVersion will only be used when binary is None and binary_path is None)

I hope i have understood well the concern, and that I made myself clear. What do you think ?
The binary argument is already a path to the binary. If it's omitted then you're right in that we check the current working directory. In the case where get_version is called with a binary path we don't want to catch the LocalAppNotFoundError and fallback to RemoteB2GVersion as we're indicating that we're expecting a local application. I would probably just change get_version to only catch the LocalAppNotFoundError exception if binary is None.
(Assignee)

Comment 6

3 years ago
Ok, I think it's clear for me now - thanks for the explanation. Somewhere I got confused with the logic here.
(Assignee)

Comment 7

3 years ago
Created attachment 8504685 [details] [diff] [review]
mozversion should throw some kind of exception if binary specified and nothing found
Attachment #8504685 - Flags: review?(wlachance)
Comment on attachment 8504685 [details] [diff] [review]
mozversion should throw some kind of exception if binary specified and nothing found

Good stuff. Could you confirm that you ran the unit tests and mark this as checkin-needed if so?
Attachment #8504685 - Flags: review?(wlachance) → review+
(Assignee)

Comment 9

3 years ago
Thanks William.

I ran all the unit tests with success.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Assignee: nobody → j.parkouss
Hi, the patch didn't apply cleanly:

patching file testing/mozbase/mozversion/tests/test_binary.py
Hunk #1 FAILED at 91
1 out of 1 hunks FAILED -- saving rejects to file testing/mozbase/mozversion/tests/test_binary.py.rej


could you take a look, thanks!
Keywords: checkin-needed
(Assignee)

Comment 11

3 years ago
Created attachment 8505404 [details] [diff] [review]
mozversion should throw some kind of exception if binary specified and nothing found

It is caused by another patch (bug 1064731) that i wrote - I must have wait a bit before writing this one;

Everything is fine now, it was easy to fix. Sorry William for the review again...
Attachment #8504685 - Attachment is obsolete: true
Attachment #8505404 - Flags: review?(wlachance)
Comment on attachment 8505404 [details] [diff] [review]
mozversion should throw some kind of exception if binary specified and nothing found

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

Looks good.
Attachment #8505404 - Flags: review?(wlachance) → review+
I ran the unit tests locally and they worked fine. Should be good to land whenever.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c04673e080a3
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c04673e080a3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1088112
You need to log in before you can comment on or make changes to this bug.