Need way to set prefs that get read at browser startup

RESOLVED FIXED in mozilla34

Status

Testing
Marionette
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dmose, Assigned: mdas)

Tracking

({ateam-marionette-runner})

Trunk
mozilla34
x86
Mac OS X
ateam-marionette-runner
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [affects=loop])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Blocks: 976114
blocks testing, bumping to P1
Priority: -- → P1
Whiteboard: [affects=loop]
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.
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?.

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
Attachment #8451193 - Flags: feedback?(jgriffin)
(Reporter)

Comment 4

4 years ago
(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 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
Attachment #8451193 - Flags: feedback?(jgriffin) → feedback+
Keywords: ateam-marionette-runner
Created attachment 8455714 [details] [diff] [review]
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
Attachment #8455714 - Flags: review?(jgriffin)
Attachment #8451193 - Attachment is obsolete: true
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.
Attachment #8455714 - Flags: review?(jgriffin) → review+
thanks, I've updated accordingly and landed on m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b96e18dd924
Depends on: 1038937
Created attachment 8460321 [details] [diff] [review]
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
Attachment #8455714 - Attachment is obsolete: true
Comment on attachment 8460321 [details] [diff] [review]
prefs

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

green on try!
Attachment #8460321 - Flags: review?(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.
Attachment #8460321 - Flags: review?(jgriffin) → review+
https://hg.mozilla.org/mozilla-central/rev/0197076215ee
Assignee: nobody → mdas
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.