1030442 - Need way to set prefs that get read at browser startup

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Dan Mosedale (:dmosedale, :dmose)]

In order to do Loop functional tests, we need to use non-default settings for multiple prefs, some of which we don't control, and some of which are only read at startup.  

We'd like to be able to specify this for all of our functional tests (eg all the files which will live in browser/components/loop/test/functional/.  It'd be great for this to "just work" for folks who run "mach marionette-test", in order to avoid adding developer friction for running tests.

A couple of possible approaches include: 

1) feed args (or even just the name of a pref file) into Marionette() constructor from a Python file

or

2) run arbitrary Python code at the beginning of the group of tests that can construct the instance of Marionette that will be used for this set of tests.

At the moment, we're mostly interested in getting unblocked, so whichever of these could be landed quickest would be ideal.  That said, if they're similar amounts of work, the second is notably more powerful.

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

blocks testing, bumping to P1

Comment 2

[Malini Das [:mdas] - Away, not checking bugmail]

spoke on IRC about this, and had different approaches. One was to put the prefs in the manifest, which would work for batches of tests, but would become annoying if you needed to just run one test out of the entire manifest file, which you'd need to do if you broke a test and need to fix it locally. Another was to just change the Marionette() to take in the prefs, but we don't want to spawn a browser each time.

In the end, we decided that in each test class, in the setUp code, you declare what prefs you want the instance to have. The harness will verify if the current running instance (if any) has the prefs you want already. This way, you can reuse already running browsers if they match the prefs you want, and if not, we can kill that browser and spawn one with the requested prefs. It will also let you easily run single tests outside of a manifest.

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: add 'enforce_gecko_prefs' function

This is a fully working patch, but I have some concerns we can address in this bug or a follow up so I'm marking it as f?.

This patch adds the 'enforce_gecko_prefs' function which accepts a dictionary of preferences, and will verify if the current running browser has these prefs, and if so, it will use it for the test. If not, it kills it, and starts a new browser with the correct prefs.

One concern is that we don't have an easy, client-facing way to reset the gecko profile. We should add something like 'enforce_clean_profile' function for tests who want the most basic pref setup, and this function can check if the current running instance has no modified preferences. If it does, then we can kill it and spawn an instance with the minimal profile.

Another minor concern is by default, the runner will start a browser with the minimal profile. We may want to add a way to suppress the auto launching of the browser, but I don't really have any good ideas about that. It's also not a very impactful problem.

Desktop Mn try:
https://tbpl.mozilla.org/?tree=Try&rev=1f7e18aabf04

Emulator Mnw try:
https://tbpl.mozilla.org/?tree=Try&rev=f5ab2939b156

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to Malini Das [:mdas] from comment #3)
> Created attachment 8451193 [details] [diff] [review]
> add 'enforce_gecko_prefs' function
> 
> This is a fully working patch, but I have some concerns we can address in
> this bug or a follow up so I'm marking it as f?.

Thanks, Malini!

> Another minor concern is by default, the runner will start a browser with
> the minimal profile. We may want to add a way to suppress the auto launching
> of the browser, but I don't really have any good ideas about that. It's also
> not a very impactful problem.

In the past, my experience has been that the slower the a given set of tests is, the less developers get around to running them.  This causes bugs to get caught later in the development cycle of a patch, when they are more expensive to fix, because the mental context available to fix them has faded.  Of course, for tiny patches, this isn't a big deal, but for larger patches, it can be meaningful.  So my gut is that there's more (somewhat hidden but meaningful) impact to this than meets the eye.

Comment 5

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8451193 [details] [diff] [review]
add 'enforce_gecko_prefs' function

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

::: testing/marionette/client/marionette/marionette.py
@@ +709,5 @@
> +    def enforce_gecko_prefs(self, prefs):
> +        """
> +        Checks if the running instance has the given prefs. If not, it will kill the
> +        currently running instance, and spawn a new instance with the requested preferences
> +        

nit: extra whitespace

@@ +711,5 @@
> +        Checks if the running instance has the given prefs. If not, it will kill the
> +        currently running instance, and spawn a new instance with the requested preferences
> +        
> +        : param prefs: A dictionary whose keys are preference names.
> +        """

This method will fail if not self.instance (i.e., if we're on B2G), so we should probably check for that and give an appropriate error.

@@ +745,5 @@
> +                                           gecko_log=self.instance.gecko_log, prefs=prefs)
> +            self.delete_session()
> +            self.instance.close()
> +            self.instance = instance
> +            self.instance.start()

Ideally, I think a lot of this logic would live in geckoinstance.py, in a method called e.g., restart(prefs=None).  This would allow us to simplify the logic quite a bit (i.e., no need to re-instantiate an instance).

::: testing/marionette/client/marionette/tests/unit/test_enforce_prefs.py
@@ +16,5 @@
> +        int_value = self.marionette.execute_script("return SpecialPowers.getIntPref('marionette.test.int');")
> +        self.assertTrue(bool_value)
> +        self.assertEqual(string_value, "testing")
> +        self.assertEqual(int_value, 3)
> +        

nit: extra whitespace

Comment 6

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: prefs

Updated according to feedback.

This patch has different behaviour than the previous patch. In the previous patch's enforce_gecko_prefs, we'd reuse the previous preferences if they were not reset, ie: if you did enforce_gecko_prefs({"customPref1": 0}), then called enforce_gecko_prefs({"customPref2": 1}, then the new profile will have both customPref1 and customPref2 defined, when only customPref2 should be defined. This is unwanted, you should state exactly what prefs you want to add on top of the *clean* profile (not previously altered profile).

I've also added a restart_with_clean_profile command.

Desktop try:
https://tbpl.mozilla.org/?tree=Try&rev=6140f9af2a42

Emu try:
https://tbpl.mozilla.org/?tree=Try&rev=14df33ff8a89

Comment 7

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8455714 [details] [diff] [review]
prefs

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

::: testing/marionette/client/marionette/geckoinstance.py
@@ +79,5 @@
>              self.runner.stop()
>              self.runner.cleanup()
>  
> +    def restart(self, prefs=None):
> +        self.close() 

nit: extra whitespace at EOL

::: testing/marionette/client/marionette/tests/unit/test_profile_management.py
@@ +17,5 @@
> +        int_value = self.marionette.execute_script("return SpecialPowers.getIntPref('marionette.test.int');")
> +        self.assertTrue(bool_value)
> +        self.assertEqual(string_value, "testing")
> +        self.assertEqual(int_value, 3)
> +        

nit: extra whitespace here

@@ +29,5 @@
> +    def test_clean_profile(self):
> +        self.marionette.restart_with_clean_profile()
> +        try:
> +            bool_value = self.marionette.execute_script("return SpecialPowers.getBoolPref('marionette.test.bool');")
> +        except JavascriptException as e:

This should use self.assertRaises to make sure we get the expected exception.

Comment 8

[Malini Das [:mdas] - Away, not checking bugmail]

thanks, I've updated accordingly and landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b96e18dd924

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows Marionette failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c54077a44ad8

https://tbpl.mozilla.org/php/getParsedLog.php?id=43833869&tree=Mozilla-Inbound

Comment 10

[Malini Das [:mdas] - Away, not checking bugmail]

Attachment: prefs

The problem was that the gecko.log file that we create could not be deleted between restarts on Windows because its filesystem is wonky. Mozprocess closes the file 'successfully', but if we call os.remove() on it too soon, we get a 'file still in use' error. I spoke with ted and others on #ateam, and there isn't a real solution around it, so I added a couple retries. If it still fails to remove it after the retry, we continue anyway, since the subsequent instances will just append their logs to the existing gecko.log file, and it doesn't impede testing.

will r? if try passes:
https://tbpl.mozilla.org/?tree=Try&rev=858567e41bd3

Comment 11

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8460321 [details] [diff] [review]
prefs

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

green on try!

Comment 12

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8460321 [details] [diff] [review]
prefs

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

Sad, but seems like a good workaround.

::: testing/marionette/client/marionette/geckoinstance.py
@@ +54,5 @@
>  
>          self.gecko_log = os.path.realpath(self.gecko_log)
>          if os.access(self.gecko_log, os.F_OK):
> +            if platform.system() is 'Windows':
> +                #NOTE: windows has a weird filesystem where it happily 'closes'

nit: need space before 'NOTE'

@@ +56,5 @@
>          if os.access(self.gecko_log, os.F_OK):
> +            if platform.system() is 'Windows':
> +                #NOTE: windows has a weird filesystem where it happily 'closes'
> +                # the file, but complains if you try to delete it. You get a
> +                # 'file still in use' error. Sometimes you can wait a bit and 

nit: extra whitespace at EOL

::: testing/marionette/client/marionette/marionette.py
@@ +716,5 @@
>  
> +    def enforce_gecko_prefs(self, prefs):
> +        """
> +        Checks if the running instance has the given prefs. If not, it will kill the
> +        currently running instance, and spawn a new instance with the requested preferences

nit:  end sentence with period and newline.

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

Comments addressed and pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0197076215ee

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0197076215ee