Currently, if you're using a pre-specified profile, Mozmill operates directly on that profile. As a side-effect, you can't guarantee that the profile you are operating on has not been changed by the tests. Usually, in a practical use case, this is a bad thing, since you can't really reset the profile to the state that it was before. Instead of doing this, Mozmill (Mozprofile actually) could clone from a basis profile each time it needs to reset.

If this is +ed for 2.0, I may take this as part of bug 641956 as I will need to improve profile handling there anyway

It may also be worthwhile when this bug is taken or subsequently to introduce a command-line flag that allows the profile to accumulate state. I know thunderbird wants this (bug 579929).

I wouldn't mark it as enhancement. Anyone who is using Mozmill on an existing profile and runs any of our tests will swipe away all the profile data. This is a dataloss situation, which we do not really prevent at the moment. I talked with Jeff about that earlier this week and cloning an existing profile seems to be the best way to fix this problem.

(In reply to comment #3) > I wouldn't mark it as enhancement. Anyone who is using Mozmill on an existing > profile and runs any of our tests will swipe away all the profile data. This is > a dataloss situation, which we do not really prevent at the moment. I'm not sure I understand or agree with this assessment. Currently, reseting the profile for a profile specified by --profile (in MozProfile or in MozRunner in legacy versions) removes the preferences and addons set by the runner (though not those specified by the tests). So, if the tests do not change anything (I realize this is unlikely) then the profile will be as it was before the run. Of course this only applies to specified profiles -- transient profiles (if no --profile is specified) will be wiped, though there is a bug about whether this is correct behaviour or not. I also disagree that this is a bug. This is, for better or worse, designed behaviour, and whether it is a bug or not depends on your point of view. From the point of view of MozMill + MozProfile + MozRunner as a test harness associated with hg.m.o/qa/mozmill-tests, it is a good idea to clone from a profile such that it is not corrupted. However, the slated target for MozMill is a general automation tool. You may *want* to modify the profile in place! You may want to use MozMill to generate a profile that may be used externally. For instance, fuzzing and profile testing. I would hate to throw this use-case out. To summarize: - MozProfile should have the ability to clone the profile; in fact, MozProfile should have much more functionality than it currently has - MozMill should have a way to really reset the profile via cloning; perhaps/probably this should be the default - but we shouldn't throw away the ability to have MozMill (etc) actually being able to add/delete/etc to a profile as this is useful to

Not really dataloss. any test tool will munge the profile. That's why we tell people not to run test tools on their profiles.

TPS has need of this functionality as well. I may volunteer to implement this soon if it isn't picked up by someone else.

(In reply to Jeff Hammel [:jhammel] from comment #4) > To summarize: > - MozProfile should have the ability to clone the profile; in fact, > MozProfile should have much more functionality than it currently has By cloning a profile you mean just copying the whole directory? > - MozMill should have a way to really reset the profile via cloning; > perhaps/probably this should be the default > - but we shouldn't throw away the ability to have MozMill (etc) actually > being able to add/delete/etc to a profile as this is useful to Maybe I'm missing something but from what I see, MozMill interacts with the profile via mozrunner, is that correct? Should the reset via cloning functionality be implemented in mozrunner (since MozMill already seems to call this here [1]) ? [1] https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__.py#L302

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #7) > (In reply to Jeff Hammel [:jhammel] from comment #4) > > To summarize: > > - MozProfile should have the ability to clone the profile; in fact, > > MozProfile should have much more functionality than it currently has > > By cloning a profile you mean just copying the whole directory? There may be nuances encountered that aren't obvious but yes, a recursive copy should be sufficient. I was thinking something like: @classmethod def clone(cls, path, **kwargs): """instantiate a mozprofile instance via cloning - path : path of the basis to clone - kwargs: arguments to the profile ctor """ # NOTE: this is a cartoon tempdir = tempfile.mkdtemp() shutil.copytree(path, tempdir) return cls(tempdir, **kwargs) I don't know if there are reasons not to do it this way, but this LGTM. I'm open to architectural suggestions here. > > - MozMill should have a way to really reset the profile via cloning; > > perhaps/probably this should be the default > > - but we shouldn't throw away the ability to have MozMill (etc) actually > > being able to add/delete/etc to a profile as this is useful to > > Maybe I'm missing something but from what I see, MozMill interacts with the > profile via mozrunner, is that correct? Should the reset via cloning > functionality be implemented in mozrunner (since MozMill already seems to > call this here [1]) ? > > [1] > https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__. > py#L302 This bug was filed a long time ago when mozprofile was part of mozrunner and both lived in the mozmill repository. Currently, mozmill in production is on 1.5 and is uninterested in upgrading to mozbase software until 2.0 which has no ETA. If we end up using this in mozmill and other harnesses, mozrunner will (probably) need a front end to this functionality.

(In reply to Jeff Hammel [:jhammel] from comment #8) > (In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #7) > > (In reply to Jeff Hammel [:jhammel] from comment #4) > > > To summarize: > > > - MozProfile should have the ability to clone the profile; in fact, > > > MozProfile should have much more functionality than it currently has > > > > By cloning a profile you mean just copying the whole directory? > > There may be nuances encountered that aren't obvious but yes, a recursive > copy should be sufficient. I was thinking something like: > > @classmethod > def clone(cls, path, **kwargs): > """instantiate a mozprofile instance via cloning > - path : path of the basis to clone > - kwargs: arguments to the profile ctor > """ > # NOTE: this is a cartoon > tempdir = tempfile.mkdtemp() > shutil.copytree(path, tempdir) > return cls(tempdir, **kwargs) > > I don't know if there are reasons not to do it this way, but this LGTM. I'm > open to architectural suggestions here. That's exactly what I was thinking, plus some checks and cleanup after the job is done. I guess I'll come up with a patch and if you have any other suggestions I can add to that. > > > > - MozMill should have a way to really reset the profile via cloning; > > > perhaps/probably this should be the default > > > - but we shouldn't throw away the ability to have MozMill (etc) actually > > > being able to add/delete/etc to a profile as this is useful to > > > > Maybe I'm missing something but from what I see, MozMill interacts with the > > profile via mozrunner, is that correct? Should the reset via cloning > > functionality be implemented in mozrunner (since MozMill already seems to > > call this here [1]) ? > > > > [1] > > https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__. > > py#L302 > > This bug was filed a long time ago when mozprofile was part of mozrunner and > both lived in the mozmill repository. Currently, mozmill in production is > on 1.5 and is uninterested in upgrading to mozbase software until 2.0 which > has no ETA. If we end up using this in mozmill and other harnesses, > mozrunner will (probably) need a front end to this functionality. OK, I'll look into the easiest way to integrate this into mozrunner.

I guess deleting the cloned profile on cleanup() makes the most sense. Let me know if you think otherwise.

Comment on attachment 711268 [details] [diff] [review] implement the clone operation Sorry its taken a bit to get to this. I've thought about this a bit and instead of just the path to clone from you will probably also want the path to clone to, which will be None by default (identical to the Profile.__init__'s argument). You may want to clone to a particular path, in which case you specify it. Or you may not and leave it as None, and keep the tempfile logic as you have. In general, I don't like instances to have knowledge of the factory that created them. In some cases, it is unavoidable, but I don't think think think this is one of them. Currently to persist changes to the filesystem past cleanup(), as in the command line case, `restore` is set to False in the ctor. I think this should be identical for clones: if restore=False, just keep it around. If restore=True, delete the clone. In any case, I don't think that having cloned as an argument to the ctor is a good idea. As far as how to achieve this, I would probably decorate the cleanup (and probably __del__ method on the instantiated object before returning it from `clone()`. There are alternatives....if you wanted to touch a special _im_a_clone attribute and have cleanup() check for this, I think that's a lot better that passing an argument as its not exposed API. Anyway, this is mostly a good attempt. I feel like this is mostly my fault for leading you down the path, but I didn't think of these issues until now. In any case, despite my r-, excellent work and thanks.

Ok then, so let's just have the clone classmethod that takes in both path arguments and optionally you can just pass in restore=True/False as a kwarg.

Comment on attachment 712892 [details] [diff] [review] implement the clone operation This is mostly good but now we're not cleaning up the profile if restore=False. We should do this. Again, I'd recommend a decorator. r- for this, though I'm happy to help out with this + rmtree(tempdir) # copytree requires that dest does not exist :sigh: one of my least favorite python bugs :/ The worst part is it is one line of code that can be changed in stdlib

(In reply to Jeff Hammel [:jhammel] from comment #13) > Comment on attachment 712892 [details] [diff] [review] > implement the clone operation > > This is mostly good but now we're not cleaning up the profile if > restore=False. We should do this. Again, I'd recommend a decorator. r- > for this, though I'm happy to help out with this Oh, so you mean even if restore=False, if this is a clone, remove the profile? Ok, will do! > > + rmtree(tempdir) # copytree requires that dest does not exist > > :sigh: one of my least favorite python bugs :/ The worst part is it is one > line of code that can be changed in stdlib I don't like it either. :(

Is this what you meant? I need the MethodType thing if I want the method to stay bound to the object. If restore is True, then the cloned profile remains on the disk. Do we want that?

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #14) > (In reply to Jeff Hammel [:jhammel] from comment #13) > > Comment on attachment 712892 [details] [diff] [review] > > implement the clone operation > > > > This is mostly good but now we're not cleaning up the profile if > > restore=False. We should do this. Again, I'd recommend a decorator. r- > > for this, though I'm happy to help out with this > > Oh, so you mean even if restore=False, if this is a clone, remove the > profile? Ok, will do! Yeah, sorry if this wasn't clear from comment 11 . To re-explain, and hopefully not re-confuse you ;), my line of thought was in existing `restore=False`/`restore=True` cases, the former means "I want all the changes kept for the profile" and the latter means "I want to revert the profile to the state it was before Profile was instantiated". I found this line of thought directly extensible to clone(). For the `restore=False`, its obvious what to do: the clone stays around (with whatever changes mozprofile, etc makes to it). For the case where `restore=True`, before the clone, the profile didn't exist. So to restore to its previous state to me says to remove the clone, it was ephemeral. Otherwise clone(..., restore=True) gives you a copy of the profile as it was before the clone started, which doesn't make any sense, really (if you wanted to do that, you could copy the directory yourself). Make sense?

After confusing, de-confusing and re-confusing myself, I think I got it! :) Btw, wouldn't it be easier to use the create_new attr instead of performing magic on the methods?

Comment on attachment 715107 [details] [diff] [review] implement the clone functionality A few nits: + """Deletes a cloned profile even if restore == False""" So that's wrong ;) Please fix the docstring. + if self.restore: + if os.path.exists(self.profile): this should be a one line + c.cleanup = types.MethodType(cleanup_clone(cls.cleanup), c) + c.__del__ = types.MethodType(cleanup_clone(cls.__del__), c) shouldn't this be c.__del__ = c.cleanup = ... ? So now I'm confusing myself wrt this bug ;) My apologies that I haven't been clearer about things and thanks for bearing through the confusion. So I notice that you don't call the function that you pass the decorator (Profile.cleanup, the original). My intention was that the decorated function would call the original function and so do whatever needed to be done normally on cleanup and then rmtree the path. Does that make sense? So I'll give this an f+ given understanding of that. The code looks good, I just evidently can't communicate what I want :/

My bad, I forgot to call the wrapped method. Sorry for this taking so long, I think it's ok now.

Comment on attachment 715202 [details] [diff] [review] implement the clone functionality Nice :) Looks about perfect, and I meant to add using types.MethodType is going beyond what's really needed (assuming that that all works out; i actually haven't done this for a long time, and as you might imagine not very often). I'm sorry for the lengthy process my self, and self-contradictory and vague directions which, in the end, you did quite well with and despite of :) As an aside, you could have only bound cleanup on restore=True as an argument to clone and I probably wouldn't have noticed or even if I did I may not have said anything, but this is certainly the correct way and not that as the method could conceivably be changed during the instance lifetime (there are probably bugs that exist in this regard; that said, how often will people do that or will it be a good idea?)

Well, you can't change a method without using types.MethodType if you still want it to work the same way as the original one (as a bound method). Or if you can, I did not find the way. :( (Not sure what you mean by binding cleanup only on restore=True.)

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #21) > Well, you can't change a method without using types.MethodType if you still > want it to work the same way as the original one (as a bound method). Or if > you can, I did not find the way. :( So I think for an instance method that this is absolutely correct. However, you can and I often have gotten away without binding the method, in the formal sense (since you can hang whatever functions you want on e.g. an instantiated object...well, those without rules saying you can't, anyway). For instance, this sample code works fine: http://k0s.org/hg/config/file/efeb2cc78f30/python/bind.py While I've used an object for this, for my convenience (I find it easier to double complex decorators this way), i've done the same with functions so long as the appropriate `self` argument is taken into account. It is not bound to the object; it is just unbound callable hanging off the object that the first argument happens to be the method instance. Make sense? (I mean, the explanation,not necessarily python :P). If not maybe we can figure it out on irc or email. > (Not sure what you mean by binding cleanup only on restore=True.) More rhetorics! ;) So you have: + def cleanup_clone(fn): + """Deletes a cloned profile when restore is True""" + def wrapped(self): + fn(self) + if self.restore and os.path.exists(self.profile): + rmtree(self.profile, onerror=self._cleanup_error) + return wrapped + + c = cls(path_to, **kwargs) + c.__del__ = c.cleanup = types.MethodType(cleanup_clone(cls.cleanup), c) Which, as I've said, is what I'm calling correct. Since the goal is to rm the clonedir IFF restore=True is set, you could conceivably do it like: + def cleanup_clone(fn): + """Deletes a cloned profile when restore is True""" + def wrapped(self): + fn(self) + if os.path.exists(self.profile): + rmtree(self.profile, onerror=self._cleanup_error) + return wrapped + c = cls(path_to, **kwargs) + if c.restore: + c.__del__ = c.cleanup = types.MethodType(cleanup_clone(cls.cleanup), c) That is, bind according to the information at ctor time. Again, let's email/irc if I'm still not explaining this well. I went against my own general principle of being Socratic, as it wasn't a comment on what your code did, it was a comment on the contrast of what not to do, and its only karmically viable that said attempt at communication instead resulted in more confusion. My fault, my apologies.

:mihnea, could i trick you into writing a test for this? Can be pretty basic. Otherwise I'll write a follow up bug

Ah, ok, I see what you meant. I didn't think that's what you meant because I couldn't see the point of doing it that way :). I'll get back with a test.

pushed: https://github.com/mozilla/mozbase/commit/34d3acd9a6a36ecb0565aa2ff99ad767bb3d1c74

(In reply to Mihnea Dobrescu-Balaur (:mihneadb) from comment #24) > Ah, ok, I see what you meant. I didn't think that's what you meant because I > couldn't see the point of doing it that way :). Heh, yes. Reading back I can see the confusion a-brewin'. Well, lesson learned on my part ;) Bugzilla == not the right forum. I knew that but since we're not in the same timezone these days, I made a poor decision to try to use it as one. > I'll get back with a test. Awesome, thank you very much! And thanks for the fix too for the *oldest outstanding mozbase bug*! That should come with a trophy or something Leaving the bug open for the test

This tests both possibilities (restore being True or False). I'll wait for that trophy! :)

Comment on attachment 715983 [details] [diff] [review] test this Mostly okay, but copy+paste ftl. I've written bug 843286 for having a self-executing template to avoid the copy+paste reflectively magnifying developer time sink bug. The tests are pretty basic, but better a safety net than none IMHO. +""" +test nonce in prefs delimeters +see https://bugzilla.mozilla.org/show_bug.cgi?id=722804 +""" Nope! And you'll need a license block. +import time You don't use this +from mozprofile.prefs import Preferences You don't use this

Whoops!

Comment on attachment 716201 [details] [diff] [review] test lgtm, thanks!

pushed: https://github.com/mozilla/mozbase/commit/a6b8e5b001e38baaf7fb256a1ec83db5deb4db0c