1177491 - Autophone - standardize custom fennec preferences and environment variables for tests

Status: RESOLVED FIXED

(Testing Graveyard :: Autophone, defect)

Description

[Bob Clary [:bc] (inactive)]

Right now we have to edit the test files to customize preferences and environment variables when running fennec. This is error prone and not conducive to re-use.

I think a better approach would be to use an external python file that contains a dict of the name-value pairs.

For example,

for phonetest.py, we would have phonetest-prefs.py, phonetest-env.py which would be the default prefs and environment variables for all tests.

Then each test which inherits from PhoneTest would also optionaly provide -prefs.py, -env.py files which would customize that test's preferences and environment variables.

For example, perftest.py could have perftest-prefs.py, perftest-env.py which would be added to the dicts provided by phonetest-{prefs,env}.py. On so on. 

The advantage would be a standard location and format for default and customized prefs.

Thoughts?

Comment 1

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

I like the concept, not sure about the separate files being python.  Could they be .json?  Also there could be a chance that we dynamically add/remove prefs/envvars depending on a device or build info.  If this is the case, then we are better off defining the core set of prefs and having a standard way/api for changing the defaults.

Ideally we will only have a few exceptions to the normal mode.

Comment 2

[Bob Clary [:bc] (inactive)]

I've been thinking about this and was wondering if it would be better to define the dicts containing the prefs and env variables as a property of PhoneTest.

The defaults could be set in PhoneTest, then the tests which inherit can customize for their needs by adding to/overriding the parent's values.

In addition we can further customize based on the individual test configuration by using options in the config file's [runtests] section which could provide paths to optional files for individual tests.

e.g. s1s2-blank-local.ini:

[runtests]
...
prefs = s1s2-blank-local-prefs.json
env = s1s2-blank-local-env.json

These external files would add to/override the values defined by the tests.

Comment 3

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

I like this idea best- it keeps us out of .py files for changing prefs.

I can think of a few scenarios:
1) adding a new pref/env to all tests
2) adding a new pref/env to 1 test
3) adding a pref/env to a series of tests (but not all)
4) removing a pref/env from 1 or more tests

I can't see how easy it would be to remove a pref- but adding a pref could be easy.  Maybe have a base set of prefs/env that all tests share, then a further set of prefs/env for each test?  How would something like that look like in .json/.ini format?

Comment 4

[Bob Clary [:bc] (inactive)]

Basically with this new idea, we would set the default preferences/environment variables for all tests in the dict in phonetest.py's PhoneTest. Then individual tests would customize by setting their own dicts which would add/override the parent class's dict values but not the entire dict. This would handle most cases. Customization of an individual invocation of a test would be done through the ini/json files in the config directory.

I do like ini over json btw.

I thought about how we would override/remove a pref and the best way I could think of was to just use the ini/json file to set the pref/env variable to the default. That could easily be done by using <name>= with no value. I haven't tested that but if it doesn't work then we could use <name>=<defaultvalue>.

Comment 5

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

ok, so we have a default set of preferences/envvars, then the configs/test.ini can define:
[preferences]
turbo.button = true
autophone.mode = clary

[environment]
NO_REMOTE = 1
...

Is this what you are thinking?

Comment 6

[Bob Clary [:bc] (inactive)]

yep. consistent and no need for other files. I think we have a winner.

gbrown, clearing your ni but feel free to chime in.

I'll get to this asap.

Comment 7

[Geoff Brown [:gbrown]]

This (comment 5) looks good to me.

Comment 8

[Bob Clary [:bc] (inactive)]

Attachment: bug-1177491-v1.patch

Comment 9

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

Comment on attachment 8628973 [details] [diff] [review]
bug-1177491-v1.patch

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

this is great stuff- much more simplified.

::: phonetest.py
@@ +333,5 @@
> +                        value = True
> +                    elif value.lower() == 'false':
> +                        value = False
> +                    elif re.match('\d+$', value):
> +                        value = int(value)

why do we need int here?  I would think a string would work.

@@ +357,5 @@
> +                        value = True
> +                    elif value.lower() == 'false':
> +                        value = False
> +                    elif re.match('\d+$', value):
> +                        value = int(value)

why do we need to specify int() here?

::: tests/webappstartup.py
@@ +22,5 @@
>          PerfTest.__init__(self, dm=dm, phone=phone, options=options,
>                            config_file=config_file, chunk=chunk, repos=repos)
>          self.webappstartup_name = None
> +        # Enable the consoleservice for logcat in release builds.
> +        self.preferences["consoleservice.logcat"] = True

it would be nice if this could be in the .ini files, but this simplifies things.

Comment 10

[Bob Clary [:bc] (inactive)]

(In reply to Joel Maher (:jmaher) from comment #9)
> Comment on attachment 8628973 [details] [diff] [review]
> bug-1177491-v1.patch
> 
> Review of attachment 8628973 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> this is great stuff- much more simplified.
> 
> ::: phonetest.py
> @@ +333,5 @@
> > +                        value = True
> > +                    elif value.lower() == 'false':
> > +                        value = False
> > +                    elif re.match('\d+$', value):
> > +                        value = int(value)
> 
> why do we need int here?  I would think a string would work.
> 

probably. I can test it and if it works I'll remove it.

> @@ +357,5 @@
> > +                        value = True
> > +                    elif value.lower() == 'false':
> > +                        value = False
> > +                    elif re.match('\d+$', value):
> > +                        value = int(value)
> 
> why do we need to specify int() here?
> 

ditto

> ::: tests/webappstartup.py
> @@ +22,5 @@
> >          PerfTest.__init__(self, dm=dm, phone=phone, options=options,
> >                            config_file=config_file, chunk=chunk, repos=repos)
> >          self.webappstartup_name = None
> > +        # Enable the consoleservice for logcat in release builds.
> > +        self.preferences["consoleservice.logcat"] = True
> 
> it would be nice if this could be in the .ini files, but this simplifies
> things.

Yeah, it could have been but would have been required. I think since it is required for Beta/Release I'd rather have it forced upon us.

Comment 11

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

ok, lets leave the pref in webappstartup.py and see if we can avoid the int types :)

Comment 12

[Bob Clary [:bc] (inactive)]

can't remove the int stuff: given

browser.webapps.checkForUpdates=1

end up with stuff like:

user_pref("browser.webapps.checkForUpdates", "1");

Comment 13

[Bob Clary [:bc] (inactive)]

https://github.com/mozilla/autophone/commit/2bcb211d42b5bff587a9e03263040e97934e1344