1100880 - CLI resets specified --pref options from the command line

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Martin Kimmerle]

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:

mozmill needs to be installed. run the following command:

mozmill --manual -b /moz/firefox/nightly/firefox --pref="startup.homepage_welcome_url:"


Actual results:

The browser starts and shows the welcome page:
https://www.mozilla.org/en-US/firefox/nightly/firstrun/




Expected results:

The welcome page shouldn't be shown. The result should be exactly like when running

mozrunner -b /moz/firefox/nightly/firefox --pref="startup.homepage_welcome_url:"

(this command works.)

Comment 1

[Martin Kimmerle]

I did some debugging and obviously mozmill.CLI overwrites [1] the
preferences that have been previously set by MozProfileCLI [2].

references:
[1] https://github.com/mozilla/mozmill/blob/d8527df3d3a4c8272f509b958cf971075a46ef33/mozmill/mozmill/__init__.py#L872
[2] https://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/cli.py#57

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Very good investigation of the problem, Martin! Thanks for the details. I think it is clear why those preferences are lost. Lets try to get it into version 2.1. Maybe you want to fix the bug?

Comment 3

[Martin Kimmerle]

Thank you! Sure! But I'll need some days as I'm learning for exams.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Totally understandable Martin! Take your time and good luck. I will already assign the bug to you.

Comment 5

[Martin Kimmerle]

Attachment: 0001-Bug-1100880-CLI-resets-specified-pref-options-from-t.patch

This patch is created on top of the hotfix-2.0 branch. All mutt tests have passed.

Comment 6

[Martin Kimmerle]

Attachment: 0001-Bug-1100880-CLI-resets-specified-pref-options-from-t.patch

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8526809 [details] [diff] [review]
0001-Bug-1100880-CLI-resets-specified-pref-options-from-t.patch

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

Martin, this patch looks great. Can I ask you to do one more thing? It would be good to have a test for this added feature, so we do not regress it again. It should be kinda easy to create when you take an already existent test as can be found here: https://github.com/mozilla/mozmill/tree/master/mutt/mutt/tests/python/cli

Just update it to pass in the preference, and add a js test, which verifies that the preference set via CLI is present.

Please update the patch by including the test. When it is done we can get it landed.

Comment 8

[Martin Kimmerle]

Yes I can create the test. But still, I'll need some days :)

Comment 9

[Martin Kimmerle]

Attachment: 0001-Bug-1100880-CLI-resets-specified-pref-options-from-t.patch

finally I've created a test against this bug.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8532859 [details] [diff] [review]
0001-Bug-1100880-CLI-resets-specified-pref-options-from-t.patch

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

This test looks fine! Lets make sure to update it regarding my comment as you can see below and we can get it landed. So it will be shipped with Mozmill 2.1. Thanks!

::: mutt/mutt/tests/python/js-modules/useMozmill/testPref.js
@@ +7,5 @@
> +
> +function testPref() {
> +  try {
> +    var value = Services.prefs.getIntPref("abc");
> +    expect.ok(value === 123, "The pref 'abc' matches the specified value.");

Instead of expect.ok() you can directly make use of assert.equal() and get rid of the try/catch. Even if getIntPref() fails, the thrown error would make the test to fail.

Comment 11

[Martin Kimmerle]

Attachment: 0001-Bug-1100880-CLI-resets-specified-pref-options-from-t.patch

Thank you Henrik for your review. Right, I forgot that try/catch is not necessary in tests.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8533852 [details] [diff] [review]
0001-Bug-1100880-CLI-resets-specified-pref-options-from-t.patch

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

::: mutt/mutt/tests/python/cli/manifest.ini
@@ +3,4 @@
>  
>  [test_manifest_and_tests_exclusive.py]
>  [test_profile_relative_path.py]
> +[test_pref.py]

Looks like you didn't pull the latest source for a while. So this line fails to be applied. I will fix it locally.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The patch has been landed as:
https://github.com/mozilla/mozmill/commit/385d6453520eff8fe5f7ccf80ce7c04b822a1096

Thanks Martin! If you find the time to assist us with the other preferences (mozbase) bug you found, please let me know. I would appreciate it!

Comment 14

[Martin Kimmerle]

Nice to see it's merged! I'll try to remember to pull before creating the patch for the next time.
Yes, I'll fix Bug 1100363 as well – is there a deadline for MozMill 2.1? If necessary I could fix that big in the next few days, but I'd prefer to work on it in 2015 as my other project – RequestPolicy – has several quite important tasks that I'd like to finish in 2014. :)

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozmill 2.1 is the last version we want to get out before we transition everything over to Marionette in bug 1080766. But given that bug 1100363 is mozprofile, we will even benefit from it in Marionette. So don't worry about ETAs and feel free to continue when you have the time. In the meantime I will open it up for everyone else to work on.