841086 - mozprofile - test(s) should be written for bug 759594

Status: RESOLVED WONTFIX

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

Now....hopefully....with bug 759594 if a profile has addons therein
that you are overwriting with mozprofile, the addons will be backed up
and restored.  A simple test should be written to illustrate this.
Mostly likely I'm guessing there will be edge cases where this does
not (yet) work.  If we have tests, we can ensure that we do not
regress.

Comment 1

[Jeff Hammel]

So there is some nuance that make this bug a bit challenging.  Still, I think it isn't too bad to get it to a good state and we can go from there.  I was talking to ffledgling on irc://irc.mozilla.org/#ateam today and we figured out the basic approach here: http://logbot.glob.com.au/?c=mozilla%23ateam&s=13+Feb+2013&e=13+Feb+2013#c548810

It might be useful to distill the useful information there into a bug comment, but I don't have time right now.

Comment 2

[Johannes Vogel (:nebelhom)]

Attachment: irc discussion btwn jhammel and ffledgling

I took the liberty of filtering the conversation out of the IRC background noise for someone else to read and distill out the necessary information. I am not familiar with the entire backstory, so I shall leave it for someone else.

Comment 3

[Anhad Jai Singh (:ffledgling)]

:jhammel as you suspected, https://bugzilla.mozilla.org/show_bug.cgi?id=841963 is causing the tests to fail.

Comment 4

[Anhad Jai Singh (:ffledgling)]

:jhammel correction tests are not failing, but the Error message is still being shown.

I've marked the dependency, please edit flag if you think it is inappropriate.

Comment 5

[Jeff Hammel]

(In reply to Anhad Jai Singh (:ffledgling) from comment #4)
> :jhammel correction tests are not failing, but the Error message is still
> being shown.
> 
> I've marked the dependency, please edit flag if you think it is
> inappropriate.

I meant to do that in filing. Thanks!  I don't think too much can be tested without that anyway

Comment 6

[Anhad Jai Singh (:ffledgling)]

Attachment: basic script to explain issue.

This is the script to basically show the problem that seems to be happening, it's a rough code with commented out code etc, so it's not very nice to look at.

The more I run this, the more confused I get.

Comment 7

[Anhad Jai Singh (:ffledgling)]

Attachment: proposed test(s)

I'm attaching the proposed draft of the test I made here.
other than the slight confusion regarding the cleanup calls, this is the basic structure of the test.

I decided to do 3 kinds of tests
1) Adding a diff version of a previously installed addon
2) Adding the exactly the same addon 
3) Adding an entirely different addon
All with restore = True

Comment 8

[Jeff Hammel]

Comment on attachment 716805 [details]
basic script to explain issue.

http://quotes.burntelectrons.org/6016

Comment 9

[Jeff Hammel]

Comment on attachment 716820 [details]
proposed test(s)

http://quotes.burntelectrons.org/6016

Comment 10

[Jeff Hammel]

(In reply to Anhad Jai Singh (:ffledgling) from comment #6)
> Created attachment 716805 [details]
> basic script to explain issue.
> 
> This is the script to basically show the problem that seems to be happening,
> it's a rough code with commented out code etc, so it's not very nice to look
> at.
> 
> The more I run this, the more confused I get.

I am confused as well.

So your source of confusion is that profile2 does not get deleted?  It looks like it should.

If you remove the addons from the ctor, do things behave differently?

I think we'll need to boil this down to a minimal test case, as I can't really run as is.  The output would also be helpful.

Comment 11

[Jeff Hammel]

Comment on attachment 716820 [details]
proposed test(s)

Not important for f?, but the final patch should have...
- a shebang
- a license

>import os
>import shutil
>import unittest
>import mozprofile

In general it is encourage to alphabetize imports.

>class Bug841086(unittest.TestCase):
>    """
>    Test Case for Bug 759594 - mozprofile will delete addons if
>    you specify the same addon to install.
>    """
>    here = os.path.dirname(os.path.abspath(__file__))

There's no reason for this to be a class/instance variable.  This is actually a good place to use a global.

>    def setUp(self):
>        self.addons = os.path.join(self.here, 'addon')
>        self.addonOne = os.path.join(self.addons, "firebug-1.11.1-fx.xpi")
>        self.addonTwo = os.path.join(self.addons, "firebug-1.11.0-fx.xpi")
>        self.addonThree = os.path.join(self.addons, "ghostery-2.8.4.xpi")

we can't and probably shouldn't use these addons in production.  I'm not sure what we want to do here.  Even were there not legal concerns, we would not want to add these to the repository.  What we should probably do is make addons similar to https://github.com/mozilla/mozbase/tree/master/mozprofile/tests/empty or have a utility to make these addons.

>    def test_restoreCaseOne(self):
>        """ Install a different version of a previously installed add-on. """
>
>        #sanity check
>        self.assertTrue(os.path.exists(self.addons))
>        self.assertTrue(os.path.exists(self.addonOne))
>        self.assertTrue(os.path.exists(self.addonTwo))

Won't be necessary if the above is fixed.

>        #create temp profile, install add-on
>        profile = mozprofile.profile.FirefoxProfile(addons=[self.addonOne],
>                                                    restore=False)
>        self.path = profile.profile
>        existingExtensions = os.listdir(os.path.join(self.path,
>                                                     "extensions",
>                                                     "staged"))
>
>        #Install new add-on to created profile
>        profileNew = mozprofile.profile.FirefoxProfile(profile=self.path,
>                                                       addons=[self.addonTwo],
>                                                       restore=True)
>        newInstalledExtensions = os.listdir(os.path.join(self.path,
>                                                         "extensions",
>                                                         "staged"))
>
>        #clean-up
>        #profileNew.cleanup()

Please don't include commented out code in the final patch.

>        postCleanupExtensions = os.listdir(os.path.join(self.path,
>                                                        "extensions",
>                                                        "staged"))
>
>        self.assertEqual(existingExtensions, postCleanupExtensions)
>
>    def test_restoreCaseTwo(self):
>        """ Install an addon that is previously installed. """
>
>        #sanity check
>        self.assertTrue(os.path.exists(self.addons))
>        self.assertTrue(os.path.exists(self.addonOne))
>
>        #create temp profile, install add-on
>        profile = mozprofile.profile.FirefoxProfile(addons=[self.addonOne],
>                                                    restore=False)
>        self.path = profile.profile
>        existingExtensions = os.listdir(os.path.join(self.path,
>                                                     "extensions",
>                                                     "staged"))
>
>        #Install new add-on to created profile
>        profileNew = mozprofile.profile.FirefoxProfile(profile=self.path,
>                                                       addons=[self.addonOne],
>                                                       restore=True)
>        newInstalledExtensions = os.listdir(os.path.join(self.path,
>                                                         "extensions",
>                                                         "staged"))
>
>        #clean-up
>        #profileNew.cleanup()
>
>        postCleanupExtensions = os.listdir(os.path.join(self.path,
>                                                        "extensions",
>                                                        "staged"))
>
>        self.assertEqual(existingExtensions, postCleanupExtensions)
>
>    def test_restoreCaseThree(self):
>        """
>        Install an entirely different addon
>        from one that is previously installed.
>        """
>
>        #sanity check
>        self.assertTrue(os.path.exists(self.addons))
>        self.assertTrue(os.path.exists(self.addonOne))
>        self.assertTrue(os.path.exists(self.addonThree))
>
>        #create temp profile, install add-on
>        profile = mozprofile.profile.FirefoxProfile(addons=[self.addonOne],
>                                                    restore=False)
>        self.path = profile.profile
>        existingExtensions = os.listdir(os.path.join(self.path,
>                                                     "extensions",
>                                                     "staged"))
>
>        #Install add-on to created profile
>        profileNew = mozprofile.profile.FirefoxProfile(profile=self.path,
>                                                       addons=[self.addonThree],
>                                                       restore=True)
>        newInstalledExtensions = os.listdir(os.path.join(self.path,
>                                                         "extensions",
>                                                         "staged"))
>
>        #clean-up
>        #profileNew.cleanup()
>
>        postCleanupExtensions = os.listdir(os.path.join(self.path,
>                                                        "extensions",
>                                                        "staged"))
>
>        self.assertEqual(existingExtensions, postCleanupExtensions)
>
>    def tearDown(self):
>        shutil.rmtree(self.path)
>
>
>if __name__ == '__main__':
>    unittest.main()

Comment 12

[Anhad Jai Singh (:ffledgling)]

:jhammel re: the proposed test, I understand your concerns, the test file was just was for giving a rough idea of the proposed test cases. I'll make sure to do proper ordering, licensing, and appropriate comments on the final patch.

re: addons, so even using the mozprofile addon might be an issue here? What kindof utility do you propose to generate addons?

re: the attachment 716805 [details], I will try to rerun tests and elaborate on the issue with proper output.

Comment 13

[Anhad Jai Singh (:ffledgling)]

So here are the outputs for the test script.

http://pastebin.mozilla.org/2181312 (With addon in ctor)

http://pastebin.mozilla.org/2181322 (without addon in ctor)

I've double checked the addons.py I'm using for mozprofile using vimdiff and checked it against https://github.com/mozilla/mozbase/commit/6c09199d4bf42e670efa9a3855261fb5996634cb so I hope it isn't a file mismatch problem.

So profile should be deleted after I call cleanup but it doesn't seem to be happening. Am I doing something wrong here?

Comment 14

[Anhad Jai Singh (:ffledgling)]

Hey Jeff, I hope you're feeling better now! Any update on this?

Comment 15

[Jeff Hammel]

Sorry, this got buried in my inbox.  Flagging myself as a reminder to look at this soon.

Note: the pastebin links have rotted. bugzilla attachments ftw

Comment 16

[Jeff Hammel]

(In reply to Anhad Jai Singh (:ffledgling) from comment #12)
> :jhammel re: the proposed test, I understand your concerns, the test file
> was just was for giving a rough idea of the proposed test cases. I'll make
> sure to do proper ordering, licensing, and appropriate comments on the final
> patch.
> 
> re: addons, so even using the mozprofile addon might be an issue here? What
> kindof utility do you propose to generate addons?

The mozprofile addon is fine to use; it is just only one addon, empty.xpi which I don't believe is enough to test.ver

An addon generator could use the folder structure for the empty addon and modify whatever fields are necessary to make unique addons

Comment 17

[Jeff Hammel]

:ffledgling , could you reattach the output as bugzilla attachments (for posterity)?

Comment 18

[Anhad Jai Singh (:ffledgling)]

Hi Jeff,

I seem to have run into another strange problem. I get an error when I try to run what you described in - https://bugzilla.mozilla.org/show_bug.cgi?id=759594#c0

Attaching the code and the output as Bugzilla attachments (can't trust pastebin anymore ;) )

Comment 19

[Anhad Jai Singh (:ffledgling)]

Attachment: Code that returns error

Basically implements the test case mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=759594#c0

Comment 20

[Anhad Jai Singh (:ffledgling)]

Attachment: Output for the aforementioned code

Comment 21

[Jeff Hammel]

(In reply to Anhad Jai Singh (:ffledgling) from comment #20)
> Created attachment 748361 [details]
> Output for the aforementioned code

Ah, good catch.  This is indeed a bug -- we do not define addon.  I'll try to figure out what I meant to do here.

Comment 22

[Jeff Hammel]

Repro:

(mozmill)│mozprofile -a /home/jhammel/mozmill/src/mozbase/mozprofile/tests/addons/empty.xpi
/tmp/tmpgJsvbt.mozrunner
(mozmill)│ls /tmp/tmpgJsvbt.mozrunner
extensions  prefs.js  user.js
(mozmill)│ls /tmp/tmpgJsvbt.mozrunner/extensions/
staged
(mozmill)│ls /tmp/tmpgJsvbt.mozrunner/extensions/staged/
[e-mail]
(mozmill)│mozprofile -p /tmp/tmpgJsvbt.mozrunner -a /home/jhammel/mozmill/src/mozbase/mozprofile/tests/addons/empty.xpi
Exception UnboundLocalError: "local variable 'addon' referenced before assignment" in <bound method AddonManager.clean_addons of <mozprofile.addons.AddonManager object at 0xa4c72ac>> ignored

Comment 23

[Anhad Jai Singh (:ffledgling)]

Hey Jeff,
So the issue I was talking about earlier.
I'm not able to reproduce the bug when I use temporary profiles created by mozprofile.profile.FirefoxProfile. (I'm able to repro when I use my actual firefox profile ~/.mozilla/firefox/foobar )
I'm using the addons.py just before :mihneadb's patch was applied to try to reproduce it.

It's probably me doing something wrong, but I can't figure out what it is.

Attaching the code and outputs.

Comment 24

[Anhad Jai Singh (:ffledgling)]

Attachment: Input and output, unable to repro bug 759594

Comment 25

[Jeff Hammel]

So...I'm a bit confused.  Isn't this what we expect?

Comment 26

[Jeff Hammel]

Oh, nm, the old code.  It is a mystery to me.

Comment 27

[Anhad Jai Singh (:ffledgling)]

Attachment: diff b/w current addons.py and https://github.com/mozilla/mozbase/blob/3cbfae690bfb94a7835c8442a8da3a2d885d72a5/mozprofile/mozprofile/addons.py

adding diff, with reference to IRC discussion with :jhammel

Comment 28

[David Burns :automatedtester]

:ahal 

adding you as mentor, if you can't do it please update with someone better suited

Comment 29

[Andrew Halberstadt [:ahal]]

Sure, though it looks like there was some confusion in the last couple of comments. I'm not sure what the current status of this is.

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

if this is a good first bug, we should explain what to do and make it clear what the attachments that ffledgling wrote are doing?  right now it is assigned to ffledgling, but this isn't actionable-

I would like to see this resolved:
* either outlined what is left to do
* accept defeat and mark this as resolved/wontfix
* if ffledgling isn't available to hack on it, lets unassign this bug.

Comment 31

[Anhad Jai Singh (:ffledgling)]

If I recall correctly, there were issues actually trying to duplicate the issue in the original bug (Bug 759594). Therefore replicating the issue (using the older changeset pre Bug 759594) reliably is probably the first order of business.

Once that is done, one could probably build on the approach for the test proposed above.

I'm un-assigning myself from the bug, because I don't think I'll have time to hack on it in the immediate future.

I would probably not recommend this as a good first bug since it requires knowledge of both Firefox profiles and mozprofile code (as pointed out in https://bugzilla.mozilla.org/show_bug.cgi?id=759594#c2), but it is simple enough to probably count as a good second bug or good next bug.

If there's anything else needed, please feel free to needinfo? me.

Comment 32

[Mollie]

Hi, I'm really new

Comment 33

[Mollie]

(In reply to Mollie from comment #32)
> Hi, I'm really new

I want to work on this bug, but I don't exactly know how to get started. Where could I get help with this?

Comment 34

[Aditya Addepalli]

Hello I know a fair bit of python! With a little help and information on the bug, I hope to solve it! Can anyone help?

Comment 35

[Andrew Halberstadt [:ahal]]

Hi dyex719, I'm not actually sure this is something we need anymore, because:

1. ~3 years ago we couldn't reproduce the original issue (so hard to write a test for it)
2. In a few weeks, the old style of addons that mozprofile deals with won't even work in Firefox anymore, so mozprofile development has basically stalled
3. Mozprofile has changed a ton since this bug was filed, including new tests. I'm not sure if this is even missing test coverage anymore.

For those reasons I'm going to WONTFIX the bug, sorry about that. But I can definitely help you find something else to work on! If you want something relatively straightforward, I'd suggest bug 1369710 or bug 1306122. Or (depending on your python experience) if you want something that will be a challenge and likely take awhile, you could try bug 1379151 or even bug 1346026. If you like start with one of the easier ones, get a feel for the process and move on to one of the more difficult ones if you feel up to it.

Of course these are things that I work on. If you have interest in other areas of the code base, I'd be happy to put you in contact with the owner! And please feel free to reach out to me by e-mail or on #ateam on irc.mozilla.org (nick: ahal).

Thanks for your interest!

Comment 36

[Aditya Addepalli]

(In reply to Andrew Halberstadt [:ahal] from comment #35)
> Hi dyex719, I'm not actually sure this is something we need anymore, because:
> 
> 1. ~3 years ago we couldn't reproduce the original issue (so hard to write a
> test for it)
> 2. In a few weeks, the old style of addons that mozprofile deals with won't
> even work in Firefox anymore, so mozprofile development has basically stalled
> 3. Mozprofile has changed a ton since this bug was filed, including new
> tests. I'm not sure if this is even missing test coverage anymore.
> 
> For those reasons I'm going to WONTFIX the bug, sorry about that. But I can
> definitely help you find something else to work on! If you want something
> relatively straightforward, I'd suggest bug 1369710 or bug 1306122. Or
> (depending on your python experience) if you want something that will be a
> challenge and likely take awhile, you could try bug 1379151 or even bug
> 1346026. If you like start with one of the easier ones, get a feel for the
> process and move on to one of the more difficult ones if you feel up to it.
> 
> Of course these are things that I work on. If you have interest in other
> areas of the code base, I'd be happy to put you in contact with the owner!
> And please feel free to reach out to me by e-mail or on #ateam on
> irc.mozilla.org (nick: ahal).
> 
> Thanks for your interest!

Thank you for your help, please refers to my post on bug 1369710. I hope to move to bug 1346026 later. Thanks again.

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1] (away from Feb 12th - Feb 22nd)]

(In reply to Andrew Halberstadt [:ahal] from comment #35)
> 2. In a few weeks, the old style of addons that mozprofile deals with won't
> even work in Firefox anymore, so mozprofile development has basically stalled

FYI: There is bug 1377510 which will deal with adding support of installing extensions built on top of the WebExtensions API.  There shouldn't be that much to update.