Closed Bug 1100363 Opened 5 years ago Closed 5 years ago

[mozprofile] preference names from INI file should be read case-sensitively

Categories

(Testing :: Mozbase, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37

People

(Reporter: dev, Assigned: ujjwal, Mentored)

References

()

Details

(Whiteboard: [lang=py])

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 Iceweasel/31.2.0
Build ID: 20141102014832

Steps to reproduce:

I downloaded and installed mozrunner. Then I created an INI file called prefs.ini with the following content:

[dev]
; show about:config on startup
startup.homepage_welcome_url=about:config
; no warning on about=config
general.warnOnAboutConfig=false

Then I ran this command:

mozrunner --preferences=prefs.ini:dev


Actual results:

Firefox gets opened with a tab for `about:config`, which shows a warning.

When I search for `warnonaboutconfig` in `about:config` I get:
general.warnOnAboutConfig;true
general.warnonaboutconfig;false (all characters lowercase)



Expected results:

The warning shouldn't be displayed.

When searching for `warnonaboutconfig` in `about:config` there should be only one pref:
general.warnOnAboutConfig;false

By the way, specifying the prefs via --pref= works. See:
mozrunner --pref="startup.homepage_welcome_url:about:config" --pref="general.warnOnAboutConfig:false"
Thank you Martin for filing the bug. Looks like no-one really used that method so far? I had a quick look at our code and it looks like we are missing a simple line right after instantiating the config parser:

https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigParser.optionxform

Here we should replace the internal method for optionxform with 'str', to not let the config parser lowercase the letters:

https://docs.python.org/2/library/configparser.html#ConfigParser.RawConfigParser.optionxform

So it should be an easy bug to be fixed.
Summary: preference names from INI file should be read case-sensitively → [mozprofile] preference names from INI file should be read case-sensitively
Thank you Henrik for your quick response. Yes, adding the line

parser.optionxform = str

directly after

parser = ConfigParser()

works for me. The comment could be

// Don't let the parser convert the pref name to lowercase

(sorry, I didn't make a patch yet, and in this case it was easier to write a comment.)
Fantastic! Martin, would you be interested to work on the real patch for mozprofile? We can mentor you for all the questions you have.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Yes, I could create the real patch. I suppose you'll assign this bug to me then.
Great! Yes, I will assign this bug to you (as done now). When you add the patch please also have a look at existent unit tests for the profile:

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozfile/tests/

It would be good to get another test file with a test method this does the following:

* Read a preferences config file with a preference in camel-case style
* Check that the read preference name has not been lower-cased

If you have questions or need assistance you can also contact me on IRC (irc.mozilla.org) in the #ateam channel. My nick is whimboo. Thanks!
Assignee: nobody → dev
Mentor: hskupin
Status: NEW → ASSIGNED
Whiteboard: [lang=py]
Martin won't have time the next few weeks, so putting this bug back into the general bucket for everyone else to grab.
Assignee: dev → nobody
Status: ASSIGNED → NEW
I would like to work on this. I have mozilla build ready. Please guide me further.
Thanks Ujjwal! Lets talk on IRC to get the remaining setup steps done for you.
Assignee: nobody → w.ujjwal
Attached patch bug1100363_camelcase_fix.patch (obsolete) — Splinter Review
Comment on attachment 8542262 [details] [diff] [review]
bug1100363_camelcase_fix.patch

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

Thanks for the patch, Ujjwal! That was kinda fast. Just a short tip for you: in the future please set the review flag for the patch you upload. That way the attachment it will end-up in the review queue of the appropriate reviewer. I set it to myself now, and will have a look at it soon.
Attachment #8542262 - Flags: review?(hskupin)
Comment on attachment 8542262 [details] [diff] [review]
bug1100363_camelcase_fix.patch

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

In general this patch looks fine. It's nice to see that you were able to pull all that together without any additional help!

But there is one thing which keeps the test failing at the moment, and which forces me to give you a review-. Please see my inline comments.

::: testing/mozbase/mozprofile/tests/test_preferences.py
@@ +65,5 @@
>          for pref, value in _prefs:
>              commandline += ["--pref", "%s:%s" % (pref, value)]
>          self.compare_generated(_prefs, commandline)
>  
> +

nit: According to PEP8 it only needs a single blank line. So please revert this change.

https://www.python.org/dev/peps/pep-0008/#blank-lines

@@ +113,5 @@
> +        Read a preferences config file with a preference in camel-case style.
> +        Check that the read preference name has not been lower-cased
> +        """
> +        # write the .ini file
> +        _ini = """general.warnOnAboutConfig = False"""

You have to add the DEFAULTS section here, similar to the test method above. Because of that the test is currently broken. Please have a look at the current output when running test.py.

@@ +121,5 @@
> +            os.close(fd)
> +            commandline = ["--preferences", name]
> +
> +            _prefs = {'general.warnOnAboutConfig': 'False'}
> +            commandline[-1] = commandline[-1] 

This is a no-op because we want to use the default section. So it can be removed.
Attachment #8542262 - Flags: review?(hskupin) → review-
Attachment #8542262 - Attachment is obsolete: true
Attachment #8542523 - Flags: review?(hskupin)
Thank you for the update Ujjwal! The patch looks fine to me know, but before I will give my r+ I submitted it to try for results. If all unit tests are passing we can get the patch landed!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=fc00f01fccc3
Comment on attachment 8542523 [details] [diff] [review]
bug1100363_camelcase_fix.diff

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

Try run was all green. So this patch is good to get checked-in.
Attachment #8542523 - Flags: review?(hskupin) → review+
I fixed the author of the patch (please check your global hg config and add this information) before pushing it to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/02b4ccc6cc91
Flags: in-testsuite+
Target Milestone: --- → mozilla37
Thank you Ujjwal for creating the patch! And thanks to Henrik for mentoring :)
https://hg.mozilla.org/mozilla-central/rev/02b4ccc6cc91
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.