Closed Bug 1064731 Opened 10 years ago Closed 10 years ago

[mozversion] Add unit test to cover the case when binary is a symlink

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla36

People

(Reporter: whimboo, Assigned: parkouss, Mentored)

References

()

Details

(Whiteboard: [lang=py])

Attachments

(1 file, 2 obsolete files)

In case of the binary being a symlink, we currently fail to retrieve the application and platform info. This mostly happens on Ubuntu with the default Firefox installation. That's a feature our team highly wants to have given that Mozmill tests are getting executed by users, who do not have a custom Firefox version installed by default. 

In the above case mozversion should simply check if the binary as specified is a symlink, and resolve it. Then the information retrieval will work as usual.
The necessary code landed along with my patch on bug 1061809. What I see now is that it misses a test. Dave, do you think we should add one? In that case we should keep this bug open and rephrase the summary.
Depends on: 1061809
Flags: needinfo?(dave.hunt)
We wouldn't want to regress here, so a test makes sense to me.
Flags: needinfo?(dave.hunt)
Alright. So I hope you don't mind to mentor. :) What has to be done here, is to add such a test to the test_binary.py module, which checks for such a condition on Linux and OS X only.

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozversion/tests/test_binary.py#51

Best would be when the symlink would point to another temp folder, so we can correctly check that the path is getting updated correctly.
Mentor: dave.hunt
Summary: [mozversion] Retrieve application details if binary is a symlink → [mozversion] Add unit test to cover the case when binary is a symlink
Whiteboard: [lang=py]
Assignee: hskupin → j.parkouss
Hi,

I think I understood what needs to be done and attached a patch.

Tell me if something is wrong. :)
Attachment #8503570 - Flags: review?(dave.hunt)
Comment on attachment 8503570 [details] [diff] [review]
Add unit test to cover the case when binary is a symlink

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

Thanks for the patch Julien! I think we should just tweak this so we're not depending on a real binary, but otherwise it looks good to me.

::: testing/mozbase/mozversion/tests/test_binary.py
@@ +50,5 @@
>  
> +    @unittest.skipIf(not hasattr(os, 'symlink'),
> +                     'os.symlink not supported on this platform')
> +    @unittest.skipIf(not os.environ.get('BROWSER_PATH'),
> +                     'No binary has been specified.')

We shouldn't need to depend on a real binary. If we have os.symlink available then we should be able to create a symlink to the fake binary provided by setUp.

@@ +57,5 @@
> +        browser_link = os.path.join(self.tempdir,
> +                                    os.path.basename(browser_path))
> +        os.symlink(browser_path, browser_link)
> +        v = get_version(browser_link)
> +        self.assertTrue(isinstance(v, dict))

Instead of just checking the instance, you can call self._check_version following once you're using the fake binary.
Attachment #8503570 - Flags: review?(dave.hunt) → review-
I followed your recommendations - I didn't see the fake binary. :)

I refactored a bit the file also.

By the way, I don't know which versions of python need to be supported here, but it may be better to use addCleanup instead of tearDown to ensure that cleanup is done even when the test failed. This may be a future improvement. :)
Attachment #8503570 - Attachment is obsolete: true
Attachment #8504062 - Flags: review?(dave.hunt)
Comment on attachment 8504062 [details] [diff] [review]
[mozversion] Add unit test to cover the case when binary is a symlink

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

(In reply to Julien Pagès from comment #6)
> Created attachment 8504062 [details] [diff] [review]
> [mozversion] Add unit test to cover the case when binary is a symlink
> 
> I followed your recommendations - I didn't see the fake binary. :)
> 
> I refactored a bit the file also.
> 
> By the way, I don't know which versions of python need to be supported here,
> but it may be better to use addCleanup instead of tearDown to ensure that
> cleanup is done even when the test failed. This may be a future improvement.
> :)

Looks good, thanks! I agree that addCleanup would be nice, but afaik we're still limited to Python 2.4
Attachment #8504062 - Flags: review?(dave.hunt) → review+
Nothing on our side should depend on Python 2.4 anymore. Some tools still have the need for 2.6, but that should really be the minimum supported version.
Julien: If you could just tweak the commit message to include the bug number and correct the reviewer then we can mark this as checkin-needed.

The commit message should be something like:
Bug 1064731 - [mozversion] Add unit test to cover the case when binary is a symlink; r=dhunt
Flags: needinfo?(j.parkouss)
Sorry for this. :)
Attachment #8504062 - Attachment is obsolete: true
Attachment #8504654 - Flags: review?(dave.hunt)
Flags: needinfo?(j.parkouss)
Attachment #8504654 - Flags: review?(dave.hunt) → review+
https://hg.mozilla.org/mozilla-central/rev/1ac8890ee0a3
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Removing dependency to the 0.7 release of mozversion, which didn't include this change.
No longer blocks: 1064193
You need to log in before you can comment on or make changes to this bug.