905400 - Inherit from mozprofile's general Profile class and add all testing preferences

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

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

As of now we put more and more testing related preferences to the mozprofile FirefoxProfile class. For an example see bug 897891.

We should stop this and instead have our own profile class inherited from the base Profile class with all the necessary preferences directly in Mozmill. We should not base on the FirefoxProfile of mozbase.

This work should be part of the work for Mozmill 2.1.

Comment 1

[shikher]

I am willing to solve the bug.

Comment 2

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

That's great to hear shikher! I will assign this bug to you now. If you have further questions please don't hesitate to ask here or join us on IRC in #automation. We will give you all the necessary information to get started.

Comment 3

[shikher]

Hi Henrik,

Can u tell me a step by step procedure on how can i use my python/java knowledge to solve this bug as this will be my first bug. I downloaded the repository on github, now i just need your guidance on which file i should understand and edit, etc.

Thanks,
Shikher

Comment 4

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

You have to clone two repositories:
https://github.com/mozilla/mozmill
https://github.com/mozilla/mozbase

The base classes for profiles you will find in mozbase under mozprofile. There is a profile module. Just get familiar with the profile and FirefoxProfile classes. For the changes in  Mozmill you will have to do edits in the __init__.py module of mozmill.

Surprisingly it seems easier as I thought given that we can already set preferences in Mozmill directly via the profile_args argument of mozrunner:

https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/__init__.py#L116

But before we start with that I would like to have feedback from Jeff if that is the correct way or if we should really start implementing a subclass of Profile instead.

Comment 5

[shikher]

IS it profile.py in mozprofile? What do you mean by add all testing preferences, just so i know what exactly i am doing? I will wait till you confirm from Jeff, In the meantime, i will start reading the class.

Comment 6

[Jeff Hammel]

Re comment 4 , either way seems fine to me.

Comment 7

[shikher]

Since, Jeff has finally given his verdict. It would be great if i could have instructions on what to do now.

Thanks Jeff

Comment 8

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

Hi Shikher, it looks like no-one followed up here. Would you still be interested to work on this bug?

Comment 9

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

When we continue on this bug we should take the following list of preferences into account:
http://mxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js#57

Comment 10

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

Looks like Shikher is not working on this bug.

Comment 11

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

We quickly found a new assignee! So Fredrik would like to work on it. Great!

Comment 12

[Fredrik Broman [:bromaniac]]

How can I test this? I've tried mutt (maybe not the best way to test this?) both with and without the patch I'm working on but I can't see any difference in the output.

Comment 13

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

What I would do here is to have all the testing preferences globally and exported in the mozmill module. Then we can import them in a mutt test, and push the list via the persisted property to the extension side. There we can check each of the preferences if they have been set.

Please keep in mind that you will also have to remove all those testing preferences from mozprofile's FirefoxProfile. This would just be for testing Mozmill. Once this bug is fixed those settings will have to be removed via bug 905404.

I hope that helps you Fredrik. I'm looking forward to your patch!

Comment 14

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

Hi Fredrik, I would like to ask if you can give us some updates regarding your assignment on this bug. Do you need further help or was my last explanation clear enough. Please let us know. Thanks!

Comment 15

[Fredrik Broman [:bromaniac]]

Henrik,

I've been working on it and hopefully I'll have a first attempt for a patch ready soon. Maybe even tonight!

Comment 16

[Fredrik Broman [:bromaniac]]

Attachment: 0001-Bug-905400-Inherit-from-mozprofile-s-general-Profile.patch

I'm looking forward to comments on this one!

Comment 17

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

Comment on attachment 8407057 [details] [diff] [review]
0001-Bug-905400-Inherit-from-mozprofile-s-general-Profile.patch

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

Wow, lots of preferences here! It should have been a massive work to get all this added. So I hope you are not sad about my proposal below. I digged through all of the preferences, and you can find my comments inline.

::: mozmill/mozmill/__init__.py
@@ +98,4 @@
>          results = m.finish()
>      """
>  
> +    class_preferences = {}

I think it would be better when we add the prefs_general.js file as resource into the Mozmill package, and read-in all the values. The mozprofile class has a read_prefs() method, which can directly load such a preferences file: http://mozbase.readthedocs.org/en/latest/mozprofile.html#mozprofile.prefs.Preferences.read_prefs

That way it would be much easier to maintain all those preferences, which are indeed a huge list.

Also you missed some of the prefs from the current list of default preferences as listed here:

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py#315

But maybe we should first figure out which of those will remain in mozprofile.

@@ +155,5 @@
> +    # Disable intalling any distribution add-ons
> +    class_preferences['extensions.installDistroAddons'] = False
> +    # XPI extensions are required for test harnesses to load
> +    class_preferences['extensions.defaultProviders.enabled'] = True
> +    class_preferences['geo.wifi.uri'] = 'http://%(server)s/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs'

Those entries with '%(server)s' for local test pages we might want to strip off. That has to be defined by tests themselves, or by a framework around those tests.

@@ +159,5 @@
> +    class_preferences['geo.wifi.uri'] = 'http://%(server)s/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs'
> +    class_preferences['geo.wifi.timeToWaitBeforeSending'] = 200
> +    class_preferences['geo.wifi.scan'] = False
> +    class_preferences['geo.wifi.logging.enabled'] = True
> +    class_preferences['camino.warn_when_closing'] = False # Camino-only: harmless to others

We do not test Camino. So kill it.

@@ +161,5 @@
> +    class_preferences['geo.wifi.scan'] = False
> +    class_preferences['geo.wifi.logging.enabled'] = True
> +    class_preferences['camino.warn_when_closing'] = False # Camino-only: harmless to others
> +    # Make url-classifier updates so rare that they won't affect tests
> +    class_preferences['urlclassifier.updateinterval'] = 172800

That's an interesting change which could indeed help our tests for Firefox.

@@ +199,5 @@
> +    # Don't allow the Data Reporting service to prompt for policy acceptance.
> +    class_preferences['datareporting.policy.dataSubmissionPolicyBypassAcceptance'] = True
> +    # Point Firefox Health Report at a local server. We don't care if it actually
> +    # works. It just can't hit the default production endpoint.
> +    class_preferences['datareporting.healthreport.documentServerURI'] = 'http://%(server)s/healthreport/'

We might want to leave this to prevent sending reports.

@@ +235,5 @@
> +    # prefs for firefox metro.
> +    # Disable first-tun tab
> +    class_preferences['browser.firstrun.count'] = 0
> +    # Tell the PBackground infrastructure to run a test at startup.
> +    class_preferences['pbackground.testing'] = True

We don't need this.

@@ +240,5 @@
> +    # Enable webapps testing mode: which bypasses native installation.
> +    class_preferences['browser.webapps.testing'] = True
> +    # Disable android snippets
> +    class_preferences['browser.snippets.enabled'] = False
> +    class_preferences['browser.snippets.syncPromo.enabled'] = False

Mozmill doesn't work on Android. So we can kill that.

@@ +242,5 @@
> +    # Disable android snippets
> +    class_preferences['browser.snippets.enabled'] = False
> +    class_preferences['browser.snippets.syncPromo.enabled'] = False
> +    # Do not turn HTTP cache v2 for our infra tests (some tests are failing)
> +    class_preferences['browser.cache.use_new_backend_temp'] = False

This can also be removed. We want to be as close as possible to the users situation.

@@ +269,3 @@
>          if isinstance(preferences, dict):
> +            if bool(profile_args['preferences']):
> +                class_prefs.update(profile_args['preferences'])

You don't need class_prefs here. Instead simply update the preferences dict.

@@ +271,5 @@
> +                class_prefs.update(profile_args['preferences'])
> +
> +            profile_args['preferences'] = class_prefs
> +            profile_args['preferences']['extensions.jsbridge.port'] = jsbridge_port
> +            profile_args['preferences']['focusmanager.testmode'] = True

This line should actually be part of the default profiles. Not sure why it has been added here.

@@ +277,5 @@
>          elif isinstance(preferences, list):
>              preferences.append(('extensions.jsbridge.port', jsbridge_port))
>              preferences.append(('focusmanager.testmode', True))
> +            class_prefs.update(dict(profile_args['preferences']))
> +            profile_args['preferences'] = class_prefs

See above. Please directly update preferences.

@@ +926,5 @@
>          profile_args.setdefault('addons', []).extend(ADDONS)
>  
> +        profile_args['preferences'] = copy.deepcopy(MozMill.class_preferences)
> +        profile_args['preferences']['extensions.jsbridge.port'] = self.jsbridge_port
> +        profile_args['preferences']['focusmanager.testmode'] = True

Same as above.

Comment 18

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

Jonathan, quick question here. It would be good to know for us which frameworks already make use of mozprofile, and require the default preferences as set there:

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py#315

How many of those could be removed? As far as I can remember Jeff wanted to have all of them removed over on bug 905404.

Comment 19

[Jonathan Griffin (:jgriffin)]

Most of our in-tree automation uses mozprofile now, so any changes could be potentially disruptive.  That said, since they're in-tree, you could test out removals on try.

I don't know what out-of-tree automation uses mozprofile; some of our on-device automation for B2G, and maybe a few other things.  Adding Joel in case he knows.

Comment 20

[Joel Maher ( :jmaher ) (UTC -8)]

mozmill
maybe autophone
talos does not, but it uses mozprocess.

Comment 21

[Fredrik Broman [:bromaniac]]

Some questions:

Reading the prefs_general.js was no problem but how do I know the path to it? Is it always in the same place on the testing system or should I pass it in as an argument or something?

Also, some of the things I should remove according to comment #17 (stuff for camino, android etc) is read from the prefs_general.js. Should I write som code to filter this stuff out?

Finally: if preferences are passed in to the class (via the create classmethod) should they be the only preferences used and the general_prefs.js stuff completely ignored?

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

I see thefredrik has some questions in Comment 21, could you help him get answers?

Comment 23

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

I can remember that I talked with Fredrik on IRC and I thought we sorted those questions out. Fredrik please let me know if I'm wrong, what specifically you need answered.

Comment 24

[Fredrik Broman [:bromaniac]]

I still need answers for all of my questions in #21.

Comment 25

[Andrew Halberstadt [:ahal]]

You can count on the relative (from the runtests.py files) path 'profile_data/prefs_general.py' existing. I think the mochitest harness should know to look there automatically and you only need to pass in the argument if you are using a different directory somewhere else.

If additional preferences are passed in, I think they should 'update' the prefs in prefs_general. I.e if there are two prefs the same, the one in prefs_general should be overwritten, but otherwise both should get used.

Comment 26

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

(In reply to Fredrik Broman [:bromaniac] from comment #21)
> Reading the prefs_general.js was no problem but how do I know the path to
> it? Is it always in the same place on the testing system or should I pass it
> in as an argument or something?

So I thought once more about this specific bug. Maybe we should not make it too complex, and simply get the Mozmill related preferences out of the profile.py module? And add those simply in mozmill's init.py, so without any file handling foo. That could be done later on as a follow-up bug.

http://mxr.mozilla.org/mozilla-central/source/testing/mozbase/mozprofile/mozprofile/profile.py#311

Best way here would be to decide if a given preference is necessary for a test framework only. If a user who runs mozrunner, does not need it, we should move it over to the test framework. So the list of preferences becomes small enough. One thing to test through would be how other test frameworks beside Mozmill would behave. Maybe compare entries in the prefs_general.py with those here. For those which are missing in that other file, we should add them there. 

> Also, some of the things I should remove according to comment #17 (stuff for
> camino, android etc) is read from the prefs_general.js. Should I write som
> code to filter this stuff out?

I think this question obsoletes itself with my above comment. We should do this as a second (later) step.

> Finally: if preferences are passed in to the class (via the create
> classmethod) should they be the only preferences used and the
> general_prefs.js stuff completely ignored?

Yeah, ignore general_prefs.js for now. What we would need are the base preferences as set by mozprofile, and the ones from Mozmill itself.

Fredric, does that solve your questions?

Comment 27

[Fredrik Broman [:bromaniac]]

Henrik, I think so. I will give this bug another try. If I have further questions I'll get back to you.

Comment 28

[Jonathan Griffin (:jgriffin)]

Frederick, are you still interested in this problem?  If so, has the problem been well defined enough to act on?

Comment 29

[Fredrik Broman [:bromaniac]]

No I haven't had any time to look into this. It's better if someone else takes it.

Comment 30

[Jonathan Griffin (:jgriffin)]

Ok, thanks for the update Fredrik.

Comment 31

[Tessy]

Hi.I would like to take this as my first bug

Comment 32

[Andreea Matei [:AndreeaMatei]]

Hi Tessy, welcome :) Please have a look at the documentation and the comments in the bug to understand what is requested from you. If you have further questions, we're around.
You can follow these instructions to get mozmill locally running and make the changes:
https://github.com/mozilla/mozmill

Comment 33

[Tessy]

I have clone the repo.need some mentoring.shall be working on it after 28th.is that fine.

Comment 34

[Tessy]

How to chexk it on my system.have gone through the documentation.

Comment 35

[Tessy]

How to chexk it on my system.have gone through the documentation.

Comment 36

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

Hi Tessy. Are your last two comments questions or just status updates? Anything you need help with?

Comment 37

[Tessy]

Hi  Henrik Skupin (:whimboo) :Is to possible to run the software locally on my system?I need help

Comment 38

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

Tessy, I have just seen that Andreea didn't give a URL to our documentation. So please check https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup how to setup Mozmill locally.

Comment 39

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

Tessy, do you have an update for us? If you think that you will not be able to handle this bug, please let me know. Thinking more about it's really not a good first one.

Comment 40

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

Mozmill will reach its end of life soon. We are currently working on getting
all the tests for Firefox ported to Marionette. For status updates please see
bug 1080766.

There is now bug 1123683 for getting all the necessary testing preferences added to Marionette.