Closed
Bug 1068956
Opened 10 years ago
Closed 10 years ago
mozversion should throw some kind of exception if binary specified and nothing found
Categories
(Testing :: Mozbase, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla36
People
(Reporter: wlach, Assigned: parkouss, Mentored)
References
Details
(Whiteboard: [good next bug])
Attachments
(1 file, 1 obsolete file)
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
Comment 1•10 years ago
|
||
See also bug 1064811, where I discussed this with Dave. Maybe we should really change something here?
Flags: needinfo?(dave.hunt)
Comment 2•10 years ago
|
||
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)
Reporter | ||
Comment 3•10 years ago
|
||
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•10 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 ?
Comment 5•10 years ago
|
||
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•10 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•10 years ago
|
||
Attachment #8504685 -
Flags: review?(wlachance)
Reporter | ||
Comment 8•10 years ago
|
||
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•10 years ago
|
||
Thanks William. I ran all the unit tests with success.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → j.parkouss
Comment 10•10 years ago
|
||
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•10 years ago
|
||
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)
Reporter | ||
Comment 12•10 years ago
|
||
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+
Reporter | ||
Comment 13•10 years ago
|
||
I ran the unit tests locally and they worked fine. Should be good to land whenever.
Keywords: checkin-needed
Comment 14•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c04673e080a3
Keywords: checkin-needed
Comment 15•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c04673e080a3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•