Status

Testing
Mozbase
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ffledgling, Assigned: ffledgling)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
Add unit tests for mozprofile, especially mozprofile's addon methods.
(Assignee)

Comment 1

4 years ago
Created attachment 781679 [details] [diff] [review]
0001-Bug-898265-Add-tests-for-mozprofile.patch

I'm uploading a rough version of some of the tests for the addons, please tell me if they are okay, and suggest any changes.

Jeff, I'd also like you to take a look and tell me if you feel anything needs to be added/changed.

Thanks! :)
Assignee: nobody → ffledgling
Status: NEW → ASSIGNED
Attachment #781679 - Flags: feedback?(ahalberstadt)
Flags: needinfo?(jhammel)
Comment on attachment 781679 [details] [diff] [review]
0001-Bug-898265-Add-tests-for-mozprofile.patch

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

I think this is a good start.. but I wonder whether addon_stubs.py is necessary at all? We could just check in the .xpi files into the tree. I guess generating them makes it easier to modify though, so I'm fine with leaving it as is. If you want more ideas of stuff to test, you can look through https://github.com/mozilla/mozbase/commits/master/mozprofile and write some tests that fail without the various patches and pass with them.

Thanks! All these tests are seriously awesome :)

::: mozprofile/tests/addon_stubs.py
@@ +98,5 @@
> +            </Description>
> +        </em:targetApplication>
> +    </Description>
> +</RDF>
> +"""}

Can't we just store these manifests in actual install.rdf files in a sub directory? I'd rather not store them as python strings.

@@ +118,5 @@
> +            # Raise exception here, or don't use if and use default?
> +            pass
> +    else:
> +        # Default addon stub
> +        addon = 'empty-0-1.xpi'

I think we should just make 'name' a required field and not worry about the concept of a default.

::: mozprofile/tests/test_addons.py
@@ +47,5 @@
> +        m = ManifestParser()
> +        m.read(temp_manifest)
> +        addons = m.get()
> +        # Obtain details of addons to install from the manifest
> +        addons_to_install = [ self.am.addon_details(x['path'])['id'] for x in addons ] 

nit: whitespace
Attachment #781679 - Flags: feedback?(ahalberstadt) → feedback+

Comment 3

4 years ago
> Can't we just store these manifests in actual install.rdf files in a sub directory? > I'd rather not store them as python strings.

I'm with Andrew here (and in fact would echo most of his other comments, too).  Good work!
Flags: needinfo?(jhammel)
(Assignee)

Comment 4

4 years ago
Created attachment 783747 [details] [diff] [review]
0002-Bug-898265-Add-tests-for-mozprofile.patch

I've moved the xml out of python and into separate files in a sub-directory, and fixed the rest of the nits, Please take a look and tell me if this is okay!
Attachment #783747 - Flags: feedback?(ahalberstadt)
(Assignee)

Comment 5

4 years ago
Created attachment 783752 [details] [diff] [review]
0003-Bug-898265-Add-tests-for-mozprofile.patch

Added test to manifest.ini, which I'd forgotten in the previous patch.
Attachment #783747 - Attachment is obsolete: true
Attachment #783747 - Flags: feedback?(ahalberstadt)
Attachment #783752 - Flags: feedback?(ahalberstadt)
Comment on attachment 783752 [details] [diff] [review]
0003-Bug-898265-Add-tests-for-mozprofile.patch

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

Looks good to me!

::: mozprofile/tests/addon_stubs.py
@@ +39,5 @@
> +    try:
> +        if path:
> +            tmpdir = path
> +        else:
> +            tmpdir = tempfile.mkdtemp()

nit: this isn't necessarily a tmpdir, why not just use addon_dir the whole way?

@@ +69,5 @@
> +
> +    if path:
> +        tmpdir = path
> +    else:
> +        tmpdir = tempfile.mkdtemp()

nit: same as above
Attachment #783752 - Flags: feedback?(ahalberstadt) → feedback+
(Assignee)

Comment 7

4 years ago
Created attachment 784128 [details] [diff] [review]
0004-Bug-898265-Add-tests-for-mozprofile.patch

Added a test for Bug 900154 - "Mozprofile AddonManger's cleanup() method needs fixing"

I still didn't quite get what you meant by "why not just use addon_dir the whole way?". Can you elaborate?
Attachment #781679 - Attachment is obsolete: true
Attachment #783752 - Attachment is obsolete: true
Attachment #784128 - Flags: review?(ahalberstadt)
Comment on attachment 784128 [details] [diff] [review]
0004-Bug-898265-Add-tests-for-mozprofile.patch

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

Looks good to me with that one comment fixed!

::: mozprofile/tests/test_addons.py
@@ +27,5 @@
> +            temp_addon = addon_stubs.generate_addon(name=t, path=tmpdir)
> +            addons_to_install.append(self.am.addon_details(temp_addon)['id'])
> +            self.am.install_from_path(temp_addon)
> +        # Generate a list of addons installed in the profile
> +        addons_installed = [unicode(x[:-4]) for x in os.listdir(os.path.join(

Use -len('.xpi') instead of -4 (I assume that's where the 4 comes from). Same for the other three locations below.
Attachment #784128 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 9

4 years ago
Created attachment 785011 [details] [diff] [review]
0001-Bug-898265-Add-tests-for-mozprofile-r-ahal.patch

Carrying over the r+ from ahal, after fixing the nit pointed out.
Attachment #784128 - Attachment is obsolete: true
Attachment #785011 - Flags: review+
https://github.com/mozilla/mozbase/commit/b5d2de061446e3d5ab0b0ed2ac8fbb1d174e9652

I'll leave open as I'm not sure if ffledgling was planning to write some more tests for other components or not.
(Assignee)

Comment 11

4 years ago
Hi Andrew, Thanks for the push.

I'm not planning to write more tests as part of this bug at the moment, I do have a few patches/regressions that I will try to write tests for, but I was hoping to do that on the corresponding bugs filed for the testing of those patches.

So maybe we can close this? Or we can add those bugs as a dependency to this Bug? Whichever you think is more appropriate.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.