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?
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.
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.
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.
Ok, I think it's clear for me now - thanks for the explanation. Somewhere I got confused with the logic here.
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+
Thanks William. I ran all the unit tests with success.
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!
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...
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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.