Closed Bug 1049676 Opened 9 years ago Closed 9 years ago

[mozprofile] Preferences set via set_preferences() should survive a profile reset if specified as user pref

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla34

People

(Reporter: whimboo, Assigned: andrei)

References

Details

Attachments

(1 file, 1 obsolete file)

0002-Bug-1049676-mozprofile-Preserve-new-added-prefs-duri.patch
5.04 KB, patch
ahal
: review+
Details | Diff | Splinter Review 
We store the default preferences as profile._preferences:
http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py#59

When a client calls set_preferences to write those into user.js, which is the default file name, ._preferences is not updated. So the preference does not survive a reset of the profile. Keep in mind that not in all cases a pref can already be set initially, e.g. we have the problem with mozmill on bug 1043391.

So I would suggest that when set_preferences is called and the user.js file is used, we append the pref to ._preferences. This should not happen for preferences added to prefs.js. So user prefs will remain after a reset while normal prefs are getting removed.

Andrew, how do you think about it? William is fine with it via IRC.


That way a reset would correctly initialize the new profile instance with all the user defined preferences set before.
Flags: needinfo?(ahalberstadt)
Yeah, that seems fine to me. I think the fact that there is different behaviour depending on the filename seems a little bit auto-magical to me. Maybe we could make a "set_user_preferences" method that calls "set_preferences" with "user.js" and updates self._preferences?
Flags: needinfo?(ahalberstadt)
Or we don't specify the filename but give a boolean flag for persisted, which can default to true.
I'll try fixing this.
Assignee: nobody → andrei.eftimie
Status: NEW → ASSIGNED
Working patch attached.

I went with the proposal from comment 1 to have a method that for setting user preferences since changing the filename in a boolean would be backwards incompatible.

Also tested the issue we have in bug 1043391 and it works well.
Attachment #8471644 - Flags: review?(hskupin)
Attachment #8471644 - Flags: review?(ahalberstadt)
Comment on attachment 8471644 [details] [diff] [review]
0001-Bug-1049676-mozprofile-Preserve-new-added-prefs-duri.patch

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

This looks good, but r- for now because I think the test is broken.

::: testing/mozbase/mozprofile/mozprofile/profile.py
@@ +197,5 @@
>              f.write('%s\n' % self.delimeters[1])
>  
>          f.close()
>  
> +    def set_user_preferences(self, preferences):

I know this is what I originally suggested, but maybe this should be called "set_persistent_preferences" or something so it's a little more clear what the difference between this and 'set_preferences(filename='user.js') is.

@@ +207,5 @@
> +                preferences = preferences.items()
> +            # add new prefs to preserve them during reset
> +            for new_pref in preferences:
> +                # if dupe remove item from original list
> +                self._preferences[:] = [

I don't think this [:] is necessary.

@@ +210,5 @@
> +                # if dupe remove item from original list
> +                self._preferences[:] = [
> +                    pref for pref in self._preferences if not new_pref[0] == pref[0]]
> +                self._preferences.append(new_pref)
> +        self.set_preferences(preferences)

Maybe add "filename='user.js'" here just to be explicit.

::: testing/mozbase/mozprofile/tests/test_preferences.py
@@ +150,5 @@
> +        profile.set_user_preferences(prefs1)
> +        self.assertEqual(prefs1, Preferences.read_prefs(prefs_file))
> +        lines = file(prefs_file).read().strip().splitlines()
> +        self.assertTrue(bool([line for line in lines
> +                              if line.startswith('#MozRunner Prefs Start')]))

This works, but fyi Python has an 'any' keyword for this purpose:

any(line.startswith('#MozRunner Prefs Start') for line in lines)

@@ +152,5 @@
> +        lines = file(prefs_file).read().strip().splitlines()
> +        self.assertTrue(bool([line for line in lines
> +                              if line.startswith('#MozRunner Prefs Start')]))
> +        self.assertTrue(bool([line for line in lines
> +                              if line.startswith('#MozRunner Prefs End')]))

Same.

@@ +157,5 @@
> +
> +        profile.reset()
> +        self.assertEqual(prefs1,
> +                         Preferences.read_prefs(os.path.join(profile.profile, 'user.js')),
> +                         "I pity the fool who left my pref")

Shouldn't this be "i aint getting on no plane!"?
Attachment #8471644 - Flags: review?(ahalberstadt) → review-
Comment on attachment 8471644 [details] [diff] [review]
0001-Bug-1049676-mozprofile-Preserve-new-added-prefs-duri.patch

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

I agree with Andrews comments, but also have some more.

::: testing/mozbase/mozprofile/mozprofile/profile.py
@@ +199,5 @@
>          f.close()
>  
> +    def set_user_preferences(self, preferences):
> +        """Adds preferences dict to profile preferences and save them during a
> +        profile reset"""

nit: please make this a single line or a real multiline docstring.

@@ +200,5 @@
>  
> +    def set_user_preferences(self, preferences):
> +        """Adds preferences dict to profile preferences and save them during a
> +        profile reset"""
> +        if preferences:

This check is not necessary given that preferences doesn't fallback to None as for the constructor.

@@ +204,5 @@
> +        if preferences:
> +            # this is a dict sometimes, convert
> +            if isinstance(preferences, dict):
> +                preferences = preferences.items()
> +            # add new prefs to preserve them during reset

nit: Please an empty line above.

::: testing/mozbase/mozprofile/tests/test_preferences.py
@@ +135,5 @@
>  
> +    def test_reset_should_keep_user_added_prefs(self):
> +        """Check that when we call reset the items we expect are updated"""
> +
> +        profile = Profile()

nit: no empty line above.

@@ +139,5 @@
> +        profile = Profile()
> +        prefs_file = os.path.join(profile.profile, 'user.js')
> +
> +        # we shouldn't have any initial preferences
> +        initial_prefs = Preferences.read_prefs(prefs_file)

I don't see why we have to mangle with the prefs file at all. The problem here is not related to the file at all, but that the added preferences weren't part of ._preferences.

All the tests for the right files should become their own test functions.

@@ +156,5 @@
> +                              if line.startswith('#MozRunner Prefs End')]))
> +
> +        profile.reset()
> +        self.assertEqual(prefs1,
> +                         Preferences.read_prefs(os.path.join(profile.profile, 'user.js')),

It would be enough to compare ._preferences from before the reset and after the reset.
Attachment #8471644 - Flags: review?(hskupin) → review-
The test itself is just a clone of the above test (which tests that prefs _are_ cleared after a reset). The only difference is the final assert and that we use the new method which saves the prefs between profile resets.

> @@ +157,5 @@
> > +
> > +        profile.reset()
> > +        self.assertEqual(prefs1,
> > +                         Preferences.read_prefs(os.path.join(profile.profile, 'user.js')),
> > +                         "I pity the fool who left my pref")
> 
> Shouldn't this be "i aint getting on no plane!"?
So I can't take credit for the messages :)

I'll improve it based on the feedback nonetheless
I've addressed or answered all review issues below.

Henrik is on PTO this week, so Andrew, as said a couple of times, I based the
test on the test `test_reset_should_remove_added_prefs`. Should I make a simpler
test as Henrik suggested to only test _preferences before and after a reset()?

I'm open to further improve these 2 tests if needed.

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Comment on attachment 8471644 [details] [diff] [review]
> 0001-Bug-1049676-mozprofile-Preserve-new-added-prefs-duri.patch
> 
> Review of attachment 8471644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good, but r- for now because I think the test is broken.
> 
> ::: testing/mozbase/mozprofile/mozprofile/profile.py
> @@ +197,5 @@
> >              f.write('%s\n' % self.delimeters[1])
> >  
> >          f.close()
> >  
> > +    def set_user_preferences(self, preferences):
> 
> I know this is what I originally suggested, but maybe this should be called
> "set_persistent_preferences" or something so it's a little more clear what
> the difference between this and 'set_preferences(filename='user.js') is.
I do like 'set_persistent_preferences'.

> @@ +207,5 @@
> > +                preferences = preferences.items()
> > +            # add new prefs to preserve them during reset
> > +            for new_pref in preferences:
> > +                # if dupe remove item from original list
> > +                self._preferences[:] = [
> 
> I don't think this [:] is necessary.
Done.

> @@ +210,5 @@
> > +                # if dupe remove item from original list
> > +                self._preferences[:] = [
> > +                    pref for pref in self._preferences if not new_pref[0] == pref[0]]
> > +                self._preferences.append(new_pref)
> > +        self.set_preferences(preferences)
> 
> Maybe add "filename='user.js'" here just to be explicit.
Sure.

> ::: testing/mozbase/mozprofile/tests/test_preferences.py
> @@ +150,5 @@
> > +        profile.set_user_preferences(prefs1)
> > +        self.assertEqual(prefs1, Preferences.read_prefs(prefs_file))
> > +        lines = file(prefs_file).read().strip().splitlines()
> > +        self.assertTrue(bool([line for line in lines
> > +                              if line.startswith('#MozRunner Prefs Start')]))
> 
> This works, but fyi Python has an 'any' keyword for this purpose:
> 
> any(line.startswith('#MozRunner Prefs Start') for line in lines)
This test is just the reverse of the above
`test_reset_should_remove_added_prefs` test, so I can't take credit for these
constructs. I wouldn't like them to diverge, so I updated both.

> @@ +157,5 @@
> > +
> > +        profile.reset()
> > +        self.assertEqual(prefs1,
> > +                         Preferences.read_prefs(os.path.join(profile.profile, 'user.js')),
> > +                         "I pity the fool who left my pref")
> 
> Shouldn't this be "i aint getting on no plane!"?
As said above I can't take credit for these ;)
Should we change both to reflect the actual test?

(In reply to Henrik Skupin (:whimboo) [away 08/16 - 08/23] from comment #6)
> Comment on attachment 8471644 [details] [diff] [review]
> 0001-Bug-1049676-mozprofile-Preserve-new-added-prefs-duri.patch
> 
> Review of attachment 8471644 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I agree with Andrews comments, but also have some more.
> 
> ::: testing/mozbase/mozprofile/mozprofile/profile.py
> @@ +199,5 @@
> >          f.close()
> >  
> > +    def set_user_preferences(self, preferences):
> > +        """Adds preferences dict to profile preferences and save them during a
> > +        profile reset"""
> 
> nit: please make this a single line or a real multiline docstring.
Done

> @@ +200,5 @@
> >  
> > +    def set_user_preferences(self, preferences):
> > +        """Adds preferences dict to profile preferences and save them during a
> > +        profile reset"""
> > +        if preferences:
> 
> This check is not necessary given that preferences doesn't fallback to None
> as for the constructor.
True, removed the check.

> @@ +204,5 @@
> > +        if preferences:
> > +            # this is a dict sometimes, convert
> > +            if isinstance(preferences, dict):
> > +                preferences = preferences.items()
> > +            # add new prefs to preserve them during reset
> 
> nit: Please an empty line above.
Done.

> ::: testing/mozbase/mozprofile/tests/test_preferences.py
> @@ +135,5 @@
> >  
> > +    def test_reset_should_keep_user_added_prefs(self):
> > +        """Check that when we call reset the items we expect are updated"""
> > +
> > +        profile = Profile()
> 
> nit: no empty line above.
Done (also changed the reference test to keep them in sync)

> @@ +139,5 @@
> > +        profile = Profile()
> > +        prefs_file = os.path.join(profile.profile, 'user.js')
> > +
> > +        # we shouldn't have any initial preferences
> > +        initial_prefs = Preferences.read_prefs(prefs_file)
> 
> I don't see why we have to mangle with the prefs file at all. The problem
> here is not related to the file at all, but that the added preferences
> weren't part of ._preferences.

If we would have initial prefs, the final check would fail because we compare
the final prefs with the ones we supply. As said above, this is the same test 
as the one above, but we're using the new 'presistent' method and the final
check is reversed (eg. we check that the added prefs are still there)

> All the tests for the right files should become their own test functions.
I don't understand what you mean here.

> @@ +156,5 @@
> > +                              if line.startswith('#MozRunner Prefs End')]))
> > +
> > +        profile.reset()
> > +        self.assertEqual(prefs1,
> > +                         Preferences.read_prefs(os.path.join(profile.profile, 'user.js')),
> 
> It would be enough to compare ._preferences from before the reset and after
> the reset.

Is there a downside to compare the actual preferences from the profile folder?
Attachment #8471644 - Attachment is obsolete: true
Attachment #8474536 - Flags: review?(ahalberstadt)
Comment on attachment 8474536 [details] [diff] [review]
0002-Bug-1049676-mozprofile-Preserve-new-added-prefs-duri.patch

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

This lgtm!
Attachment #8474536 - Flags: review?(ahalberstadt) → review+
Thanks Andrew,
could you please also push this, I don't have commit rights on mc.

Or should this first go through a try build?
I think as long as the test has ran locally it should be fine.

Fyi, you can add the "checkin-needed" keyword if you'd like a patch to land and you don't have push access (be sure to follow https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F first though).
Thanks for the tip.

All tests are passing locally.
Also retested that this works in fixing the issue we have in mozmill and it works perfectly.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1ba16df879d8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Blocks: 1056037
No longer blocks: 1056037
You need to log in before you can comment on or make changes to this bug.