838918 - mozprofile.prefs.write function signature is wrong and it is unused

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Jeff Hammel]

@classmethod
    def write(_file, prefs, pref_string='user_pref("%s", %s);'):

So this method is hard core broken ;) For classmethods, the first
argument is the class (usually `cls`).  Fortunately we don't use this
method....but we should!  Since we've gone to all the trouble to
support file-like objects, we should use this in
mozprofile.profile.Profile.set_preferences:
https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/profile.py#L130
. Note that the method used there, simplejson.dumps is far superior to
the craziness we do in preferences.py.  So this code should be moved
back there and the logic not repeated.  We should also change the
in-file name to `json` rather than simplejson, as `json` is the
standard.

Lots of tiny cleanup.  Probably harder to ticket than to fix (I'm
working on other issues at the moment, but it should be a quick fix
when we get around to it.

Comment 1

[Jeff Hammel]

...that said, we don't want this to languish and persist.  Lets aim on fixing this in a few days/weeks

Comment 2

[moijes]

ok. Let me see if I have understood it correct. Fixing this would involve atleast the following tasks (if not more):

1. Correcting the signature of mozprofile/mozprofile/preferences.Preferences.write
2. We should also change the in-file name to `json` rather than simplejson, as `json` is the
standard. So this would mean changing the code from line 15 - 18 and interchanging json with simplejson.
3. simplejson.dumps is far superior to the craziness we do in preferences.py. -- I didn't understand this part.

Comment 3

[moijes]

Attachment: Made the change to prefs.py and profile.py.

1. Made the change to prefs.py and profile.py
2. I didn't get the part about the logic.

Comment 4

[moijes]

Also, I've only run the test.py and I don't know how to interpret the results and I'm a little confident of this patch simply because test.py completed without crashing itself. I haven't written any unit tests to check if my fixes work.

Comment 5

[Jeff Hammel]

(In reply to moijes from comment #2)
> ok. Let me see if I have understood it correct. Fixing this would involve
> atleast the following tasks (if not more):
> 
> 1. Correcting the signature of
> mozprofile/mozprofile/preferences.Preferences.write
> 2. We should also change the in-file name to `json` rather than simplejson,
> as `json` is the
> standard. So this would mean changing the code from line 15 - 18 and
> interchanging json with simplejson.
> 3. simplejson.dumps is far superior to the craziness we do in
> preferences.py. -- I didn't understand this part.

I've looked over the mozprofile code in more detail and have decided to save 3. for another bug.  In short, when filing the bug I noticed that instead of using prefs.py's write() method, profile.py just has its own method for dumping preferences to a file.  And....its mostly better, just less flexible. So I had intended to move the good parts of profile.py's set_preferences() to prefs.py's write() and have set_preferences() make use of that.

In I still want this, but in looking over the code its a bit more work than the few lines of code I hoped to shuffle. And its not really related to the actual bug, the missing `cls` from the function signature (the simplejson/json part isn't really related either, but would have been necessary for part 3. That said, since you wrote it anyway, I think its probably a good thing to have).

I'll file a follow-up about making set_preferences etc use the prefs.py module, but I think this is good to go.

Comment 6

[Jeff Hammel]

(In reply to moijes from comment #4)
> Also, I've only run the test.py and I don't know how to interpret the
> results and I'm a little confident of this patch simply because test.py
> completed without crashing itself. I haven't written any unit tests to check
> if my fixes work.

So one of the dangers in testing is that assuming that if no tests fail that things are good :) The reason that no tests fail currently is that the write() method has no tests associated with it.  If it did, tests would have failed a long time ago and this issue would have then been fixed instead of me noticing long after the fact that what is there couldn't possibly work.

I will add a test here in https://github.com/mozilla/mozbase/blob/master/mozprofile/tests/test_preferences.py that will show the success of this patch.  Going to go ahead and r+ it if that passes, and land, if that is okay with you moijes.

Comment 7

[Jeff Hammel]

Attachment: basic test for prefs.Preferences.write

This basically writes some prefs and reads them back in.  With the patch and a minor fix (string -> str at https://github.com/mozilla/mozbase/blob/master/mozprofile/mozprofile/prefs.py#L210 ) it works as expected.

Without, it fails with a rather (at first glance) unexpected error:

"""
(mozbase)│./test_preferences.py 
......F.
======================================================================
FAIL: test_prefs_write (__main__.PreferencesTest)
test that the Preferences.write() method correctly serializes preferences
----------------------------------------------------------------------
Traceback (most recent call last):
  File "./test_preferences.py", line 238, in test_prefs_write
    self.assertEqual(dict(Preferences.read_prefs(path)), _prefs)
AssertionError: {} != {'browser.startup.homepage': 'http://planet.mozilla.org', 'zoom.maxPercent': 300 [truncated]...
- {}
+ {'browser.startup.homepage': 'http://planet.mozilla.org',
+  'zoom.maxPercent': 300,
+  'zoom.minPercent': 30}

----------------------------------------------------------------------
Ran 8 tests in 0.013s

FAILED (failures=1)
"""

This is actually all well and good (or hummy-chummy, even), but imho confusing at first glance even knowing about this bug.

This should be taken with attachment 711412 [details] [diff] [review] and a fix for the string -> str issue I'm about to post.

Comment 8

[Jeff Hammel]

Comment on attachment 711412 [details] [diff] [review]
Made the change to prefs.py and profile.py.

Giving this the formal thumbs up for landing

Comment 9

[Jeff Hammel]

Attachment: string -> str (oops!)

You might guess that this function hasn't been used much.  Really, these are just band-aids for expediency.  Will write shortly about how much more should change (wrt profile.py not using preferences.py).

Comment 10

[William Lachance (:wlach)]

Comment on attachment 712237 [details] [diff] [review]
string -> str (oops!)

Hehe, yup.

Comment 11

[William Lachance (:wlach)]

Comment on attachment 712236 [details] [diff] [review]
basic test for prefs.Preferences.write

Thanks for writing this!

Nits:

1. I think "fd" is a misleading name to use for the variable, since a NamedTemporaryFile object is not really a file descriptor. Isn't "f" the canonical name we use in python for such things?
2. Also, I'm a bit confused about why you used a try: finally: instead of reading the file back inside the with statement and leaving delete=True.

I'd appreciate if at least (1) were fixed before push (just make the correction(s), then git commit --amend).

>+    def test_prefs_write(self):
>+        """test that the Preferences.write() method correctly serializes preferences"""
>+
>+        _prefs = {'browser.startup.homepage': "http://planet.mozilla.org",
>+                  'zoom.minPercent': 30,
>+                  'zoom.maxPercent': 300}
>+
>+        # make a Preferences manager with the testing preferences
>+        preferences = Preferences(_prefs)
>+
>+        # write them to a temporary location
>+        path = None
>+        try:
>+            with tempfile.NamedTemporaryFile(suffix='.js', delete=False) as fd:
>+                path = fd.name
>+                preferences.write(fd, _prefs)

Comment 12

[Jeff Hammel]

(In reply to William Lachance (:wlach) from comment #11)
> Comment on attachment 712236 [details] [diff] [review]
> basic test for prefs.Preferences.write
> 
> Thanks for writing this!
> 
> Nits:
> 
> 1. I think "fd" is a misleading name to use for the variable, since a
> NamedTemporaryFile object is not really a file descriptor. Isn't "f" the
> canonical name we use in python for such things?

I'll change it. not

> 2. Also, I'm a bit confused about why you used a try: finally: instead of
> reading the file back inside the with statement and leaving delete=True.

So....I'm not sure. With NamedTemporaryFile is it guaranteed to be readable until its closed (which here I rely on `with` to do with scope vs an explicit call)? (And, worse, is it true on windows?) On linux I'm guessing you'd get away with this 99+% of the time, but I have no idea how its buffered so I was assuming that there was some chance that one would meet a race condition reading with Preferences vs writing.  On windows, I really don't know, and you could tell me this would fail 100% of the time and I wouldn't argue.  If you're confident that this won't happen, then I'm happy to change it, as it really bothered me I had to use two levels of indentation here anyway. Or if my doubt is founded or you're unsure, I'm happy to comment on why this is done the way that I did it.

Comment 13

[Jeff Hammel]

> I'll change it. not

Er...just "I'll change it."  I have no idea how the 'not' got in there but suspect my computer has been watching Wayne's World in the background.

Comment 14

[Jeff Hammel]

Going to land this as one commit which I'll credit to moijes. For lack of feedback I'm not going to change 2. from comment 11

Comment 15

[Jeff Hammel]

pushed: https://github.com/mozilla/mozbase/commit/3cbfae690bfb94a7835c8442a8da3a2d885d72a5