Closed Bug 830430 Opened 12 years ago Closed 11 years ago

central m-c location for profile information for applicable tests

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla23

People

(Reporter: k0scist, Assigned: ahal)

References

Details

Attachments

(1 file)

Patch 1.0 - make automation.py read prefs from testing/profiles
(deleted), patch
k0scist
: review+
Details | Diff | Splinter Review 
Currently, our test harnesses mostly have preferences hard-coded in
python.  (There is also the issue of duplicating the profile writing
code, which will be done by centralizing on mozprofile, see e.g. bug
746243 . This is a separate issue.)  This means when a new pref is
needed in the test frameworks (or a pref is altered, dropped, etc.)
one has to scour for several different places that the pref is set.
In addition, we should be using the preferences that follow the tree,
save in a few cases.

Proposal:

Instead of having preferences live in source code, we should have e.g. JSON
files of preferences that live in the tree.  There are many ways that
this could be split up, but I would propose the following:

- have an e.g. testing/profiles directory that contains these (again
  e.g.) JSON files of preferences

- most harnesses will share a central set of preferences.  Then each
  harness may have its own preferences it needs additionally.  (And
  there could also be specific cases, etc where you want alternate
  preferences based on the run).  So by this system we would have:

  prefs.json
  prefs_mochitest.json
  prefs_talos.json
  <...>

- mozprofile will have to be improved such that setting a preference
  to None (in a chain of preferences) will be equivalent to not
  setting it.  This will allow setting a pref in prefs.json that
  e.g. nine out of ten harnesses want

- The default preferences in mozbase can and should then be placed in
  appropriate (e.g.) .json files.  Care should taken that doing this
  keeps mozbase-based harnesses as friendly as possible to the end
  user (without sacrificing power, of course)

- (There are probably other things that will need work in mozprofile
  too.)

- There is also the problem of how to get buildbot harnesses to use
  these preferences.  If you're a developer running tests in m-c, that
  is easy enough, but now we have to ship these files (e.g. in
  tests.zip) or otherwise make sure that this data is available to the
  harnesses. The big payoff: higher reliability and reduced
  maintenance costs
from the buildbotcustom repo (http://hg.mozilla.org/build/buildbotcustom/file/tip/process/factory.py#l5651), we could do something like this:
diff -r fe472c742fee process/factory.py
--- a/process/factory.py	Mon Dec 10 16:38:36 2012 -0800
+++ b/process/factory.py	Mon Jan 14 14:13:10 2013 -0500
@@ -5680,6 +5680,17 @@
              workdir=self.workdirBase,
              haltOnFailure=True,
             ))
+            self.addStep(ShellCommand(
+                name='download configuration data for talos json',
+                command=[self.pythonWithJson(self.OS), 'talos_from_code.py', \
+                        '--talos-pref-url', \
+                        WithProperties('%(repo_path)s/raw-file/%(revision)s/testing/talos/prefs.json')],
+                        '--talos-pref-url', \
+                        WithProperties('%(repo_path)s/raw-file/%(revision)s/testing/talos/talos_prefs.json')],
+                workdir=self.workdirBase,
+                haltOnFailure=True,
+                log_eval_func=lambda c,s: regex_log_evaluator(c, s, talos_hgweb_errors),
+            ))
         elif self.remoteTests:
             self.addStep(DownloadFile(
              url='http://build.mozilla.org/talos/zips/retry.zip',
the idea is that this would download .json files to the talos directory and we could then reference those as we do fennec_ids for robocop:
http://hg.mozilla.org/build/buildbotcustom/file/tip/process/factory.py#l5478
Depends on: 665662
(In reply to Jeff Hammel [:jhammel] from comment #0)
> Proposal:
> 
> Instead of having preferences live in source code, we should have e.g. JSON
> files of preferences that live in the tree.

I started working on bug 835930 and it's becoming clear to me that I'd basically have to go out of my way to avoid working on this at the same time. Therefore, I'm going to work on this at the same time ;)

> - have an e.g. testing/profiles directory that contains these (again
>   e.g.) JSON files of preferences

I tend to disagree with this point. For the same reason that we keep tests as close to the code that it is testing as possible, I think we should keep profile json data as close to the relevant harness files as possible. Namely, Joe Dev will say "What prefs are getting set in our reftests? I looked in layout/tools/reftest but couldn't find anything". I wouldn't object to having some kind of centralized manifest file that lists all the relevant profile json files for each harness however.

Everything else makes sense to me. The only additional thing I would propose is that this is also a problem with addons, permissions, locations etc (though not on as big a scale). What if instead of prefs.json we had e.g:

-- profile_data.json
{ "preferences": { "dom.window.dump.enabled": "true",
                   ...
                   "foo.bar": "false"},
  "addons": ["path_to_reftest_extension"]
  "permissions": { "AllowXULXBL": "http://reftest.server:80"}
}

Then we could do:

profile_data = json.load(open("profile_data.json", "r"))
profile = mozprofile.Profile(**profile_data)
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
Your data prosposal sounds sane. Anything we're setting in profiles would ideally be described here.

The "store in a central location" idea was more to cope with the "every harness sets the same prefs so you have to change them in N locations" problem. Ideally we'd fix that while we're at it.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> The "store in a central location" idea was more to cope with the "every
> harness sets the same prefs so you have to change them in N locations"
> problem. Ideally we'd fix that while we're at it.

Ah yeah, that's true. Alternatively we could run them through the preprocessor and "include" those common prefs, but that also seems yucky.
Also worth mentioning that one disadvantage of using json files is that we lose the ability to add comments.
Catlee said json is a subset of yaml and yaml can take comments.

I asked: does that mean yaml can take json with comments?

To be determined?
In [1]: import os
In [2]: os.system('cat test_no_comment.json')
{
  "foo": "a",
  "bar": "b"
}
In [3]: import json
In [4]: json.load(open('test_no_comment.json', 'r'))
Out[4]: {u'bar': u'b', u'foo': u'a'}

In [5]: os.system('cat test_comment.json')
{
  "foo": "a",
  // this is needed for all the bugs
  "bar": "b"
}
In [6]: json.load(open('test_comment.json', 'r'))
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-6-7f9cc711e552> in <module>()
----> 1 json.load(open('test_comment.json', 'r'))
...
ValueError: Expecting property name: line 3 column 3 (char 18)
What do we want to do with preferences that depend on command line or environment input (e.g http://mxr.mozilla.org/mozilla-central/source/build/automation.py.in#502).

We could either:

A) leave those preferences inside the harness files or
b) have a pre-defined set of dictionary keywords (like "server") that the test harness would be responsible for swapping in the proper values at runtime.
b) sounds reasonable. Presumably we'd have a method that parses these files and populates a profile, so the code could be consolidated there.
Depends on: 836741
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> In [1]: import os
> In [2]: os.system('cat test_no_comment.json')
> {
>   "foo": "a",
>   "bar": "b"
> }
> In [3]: import json
> In [4]: json.load(open('test_no_comment.json', 'r'))
> Out[4]: {u'bar': u'b', u'foo': u'a'}
> 
> In [5]: os.system('cat test_comment.json')
> {
>   "foo": "a",
>   // this is needed for all the bugs
>   "bar": "b"
> }
> In [6]: json.load(open('test_comment.json', 'r'))
> ---------------------------------------------------------------------------
> ValueError                                Traceback (most recent call last)
> <ipython-input-6-7f9cc711e552> in <module>()
> ----> 1 json.load(open('test_comment.json', 'r'))
> ...
> ValueError: Expecting property name: line 3 column 3 (char 18)

In casual experiment, PyYAML does work, though I had to replace the '//' with '#' (otherwise the files are identical):

>>> import yaml
>>> yaml.load(file('test_comment.json'))
{'foo': 'a', 'bar': 'b'}

That said, while I'm not against supporting YAML, I am somewhat against depending on it.  People aren't excited about YAML anymore and few use it, eveyrone knows JSON, if you treated these JSON-like files as if they were JSON then you still couldn't use JSON-specific tools on it (as Andrew shows in comment 8 ), etc.  In short, I don't see the advantage given that we're throwing away the huge advantage of JSON's interoperability.  If we really want use YAML...I don't really mind.  I just don't see the point

That said, I do agree that JSON has several short-comings for a human-editable/readable format. Were I alone in a vacuum, I would probably use a format like unix config files (that is, like .ini without sections) as it fits the data (the only caveat being long strings since typically config files are line delimited).  PKG-INFO files are in this format which python reads using the email module, I believe.  I'm not necessarily suggesting this, but...
Yeah, I don't like YAML as a language, but I would love a json-with-comments-and-trailing-commas parser.  Seems like YAML may be that.
Taking a step back, there is another dimension to this bug wrt where preferences live.

So we know why its bad to keep preferences in mozprofile (I'm going to focus on preferences, though you can read this to be "all profile data" for the purposes of this comment; I'm also talking about data with the mozprofile/mozbase repo, not necessarily in any particular format). It makes it hard to add preferences and obfuscates the process for devs, but as bad as that is, its not a show-stopper.  More importantly, to reiterate, since what we are testing lives with the tree, if a preference needs to be changed or add for a particular branch at a particular commit, you can change one file and all test harnesses on m-c get that picked up automatically.

That part is well discussed.

However, what do we do for harnesses that don't live in the tree? Imagine for a moment that you are making, as we do, a new test harness and that you want to use mozprofile.  Where do you get your preferences?

A few amongst probably infinite possibilities (and realizing this is as much of a human problem as a technical problem):

- your harness copies + pastes the preferences file from whatever version of m-c you're on and instructs mozprofile to use this.  The advantage of this is that it requires no thought at all ;)  The disadvantage is that it has essentially reversed the problem: harnesses in m-c (if hooked up properly, which with some clever `mach` magic could be made semi-automated) get the right set of preferences out of the box and are always in sync (yay!), but now if there is a preference change there are N places where that needs to propagate, where N is the number of harnesses out-of-tree.  Currently, there is only one: directly to mozprofile.  While this further highlights the need to keep track of mozbase consumers, we should have a strategy here. Note that things are slightly rosier than this, since while this does absolutely nothing for out-of-tree consumers of mozprofile wrt branches (its on them), we have solved that problem for m-c.  But while we've solved one(+) problem, we have created N problems.

- as a stab at fixing this, we could mirror what i call in comment 0 prefs.json *BACK* to mozproifile *FROM* m-c and either have a CLI/API flag to use these in mozprofile or use them as default (but probably not by default, as people may become confused). This puts a big-ole bandaid around one problem, the O(1) -> O(N) places to fix preferences for out-of-tree consumers (OOTC), but unless we do this e.g. via pulse messages, this won't be up-to-the-minute.  It also does absolutely nothing about the branch problem.  It introduces a new thing to think about: when to mirror (and the implied: when do we bump versions?)?  Ideally, as suggested, if we'd do this we'd use pulse or other push-based notification.  But that's another piece to wire up.  Could reverse-mirror these on generate_diff.py or similar or other half-way measure

- the same as the above but with a tip of the hat to branches: we mirror the prefs.js back to mozprofile but one per branch we care about, which we can then specify or introspect to gain access to which one we should use.

- for prefs.js (though not other), we could have this take a URL and then keep a short-hand for URLs of branches we actually care about

- ...

I have a meeting and need more coffee.  In short, its a hard problem and I don't like any of the above, or for that matter the four or more so that I don't have time to list right now.  I do think we should figure out what this is going to look like for OOTC.  For one, its the right thing to do vs. kicking the can.  For another, I think there is a lot of insight to be gained about the geometry of our testing ecosystem by considering what we're doing here.
I think we should optimize for the "things that live in the tree" scenario, but I think the scenario you posit is totally solveable. You can read the repository+changeset info out of application.ini for whatever version of Firefox you're running, so it would be simple enough to fetch http://hg.mozilla.org/whatever-repo/raw-file/path/to/preferences.json and use it. We could even write some helper code that did that.
While I disagree with your assertion that we should optimize for things that live in the tree (if we support both, we should support both), but I thought about it and came to the same conclusion.  We essentially have 3 scenarios:

- running in-tree (the dev method): this bug will fix everything ;) Well, conceptually anyway.  It would be nice (to be filed) to have some mach magic to hook things up automagically to the build system, but that's what one of my teachers referred to as "gravy"

- running in production: we will have to do something here to get the profile sctuff -> the build slaves, but that's a problem we know how to solve: tests.zip (for now, if mozharness wants another solution that's cool too, and/or we could have a helper class, etc to give the path to sctuff in mozharness for easy consumption, but not an issue until we have or will have had duplicate code in the mozharness scripts)

- i am a person and i want to run some tests on my desktop from a harness not in m-c: which for many people is the usual case.  In this case, we do the above: in moztest (probably, though you can convince me otherwise; I would say *not* mozprofile as it seems more appropriate for an integration layer such as mozrunner or the stub moztest) we store the URLs for the various e.g. prefs.js files and code to read/use application.ini or allow the user to specify which branch (etc) they want and then fetch the profile information from the web.  Of course, you can still download and use the data yourself or what not; its just a "Do The Right Thing" default that will deal with non-exceptional use cases.  mozprofile should be modified to read e.g. prefs.js from the web.  If we're dealing more than preference files, this would have to be thought out more, though I'm guessing just doing the preference files would be a 95+% win.

No infrastructure is needed, which is always a plus. You should always have the correct version, so the problem will actually be solved for the case of sane input (and is correctable otherwise, though we should be verbose in e.g. logging about what's happening).  The only downside is having to hit net, but i think it is on the tester to make sure they can get the data (if there is a "sanity check" type step in the harness, we could check to ensure that "stuff" gets fetched then, and likewise we could cache it, but those are all nice-to-haves that can be iterated on).

TL;DR - move forward!  There is a clear build-on-able path here :)
We don't have access to application.ini all the time, specifically when it comes to fennec.  I am not sure about b2g.
I'm finding it very difficult to get a patch up for this bug that doesn't depend on bug 835930 being finished at the same time (which in turn is blocked by bug 837719).

Because there are so many different dependent pieces that need to come together at the same time, I really don't want to get carried away on scope for this bug.

For now I'm going to:
A) only do mochitests
B) only focus on the "running in tree/production" use cases from comment 15

Because it is mochitest only I will put the patch up in bug 835930 and return to this bug to finish the other work at a later date.
(In reply to Joel Maher (:jmaher) from comment #16)
> We don't have access to application.ini all the time, specifically when it
> comes to fennec.  I am not sure about b2g.

Since this is for developer testing+debugging, I'm less worried about fennec/b2g.  For one, those are a PITA enough anyway ;) Since we'll allow specifying an arbitrary URL for e.g. prefs.js, I don't see this as a blocker
No longer depends on: 665662
Depends on: 838273
my only concern is developing with a known limitation we will end up hacking when it comes time for production and/or mobile.
(In reply to Joel Maher (:jmaher) from comment #20)
> my only concern is developing with a known limitation we will end up hacking
> when it comes time for production and/or mobile.

I'm happy to hear ideas here.  For production, the automation better damn know what file/URL they are pointing to.  While how that will work in practice isn't fleshed out, I am less worried about that piece since it should both be automated (no human error, at least wrt run time) and explicit.  It would be nice to look at this problem from the point of view of automation and figure out the pieces there.  I would assume that the profiles will be propagated to the test slave (and for mobile possibly to the device).

For the developer case, if you don't have an application.ini (e.g.), with this methodology you have a choice:
- specify a URL of a (e.g.) prefs.js file on the (e.g.) command line
- download/craft the same and point to that (in a way, the same thing)

This is all really bug 838273 stuff.  The first round may not even utilize application.ini: currently we have no code to read/use it.  There is https://bugzilla.mozilla.org/show_bug.cgi?id=769606 filed, but that is for mozrunner.  We may want to reconsider putting the code there; I'm kinda inclined to its own package.
> I'm kinda inclined to its own package.

And I've already changed my mind ;)

https://bugzilla.mozilla.org/show_bug.cgi?id=769606

The runner should probably do all of this, IMHO, and just invoke the profile how it wants.  There's so much work ahead of this that by the time we get to it hopefully we'll have a clear idea on the next steps.
See Also: → https://bugzilla.mozilla.org/show_bug.cgi?id=769606
Blocks: 835930
Depends on: 839527
Depends on: 841061
Bug 835930 got the ball rolling here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/947b9aa5e2d3

I'll use this bug to finish the job (e.g convert desktop/fennec mochitests as well as reftest and xpcshell tests).
I'm going to try and finish this up sooner rather than later. We don't want prefs living in >1 place in-tree.

These are the files that potentially use the prefs in automation.py:
http://mxr.mozilla.org/mozilla-central/search?string=import%20automation

For speed, I don't want to wait until all of those are converted to use mozprofile. Instead I'll modify automation.py itself to read the prefs from testing/profiles/prefs_general.js
In bug 835930 I created a testing/profiles/prefs_general.js file that contains the preferences that are hardcoded in automation.initializeProfile(). Unfortunately this means we now have the same set of prefs defined in two places in-tree which is bad.

Therefore this patch will remove the preferences out of automation.py. This patch is a temporary fix that will keep the prefs in one place until we can wean all of the other harnesses off of automation.py.

I did a diff of the user.js file generated from automation.initializeProfile both with and without this patch and they are identical. There is also a try run here (it turns out only mochitest, leaktest.py and profileserver.py use initializeProfile):
https://tbpl.mozilla.org/?tree=Try&rev=41ca4d32a78c

I'll make a post to dev.platform upon landing this. Let me know if this approach looks sane.
Attachment #733949 - Flags: review?(jhammel)
Attachment #733949 - Flags: feedback?
Review ping. Or if you'd rather I assign it to someone else, feel free to unflag yourself.
Flags: needinfo?(jhammel)
Attachment #733949 - Flags: review?(jhammel) → review?(jgriffin)
Flags: needinfo?(jhammel)
Comment on attachment 733949 [details] [diff] [review]
Patch 1.0 - make automation.py read prefs from testing/profiles

Jeff, can you please review this today?
Attachment #733949 - Flags: review?(jhammel)
Attachment #733949 - Flags: review?(jgriffin)
Attachment #733949 - Flags: feedback?
I'm looking at this now
Comment on attachment 733949 [details] [diff] [review]
Patch 1.0 - make automation.py read prefs from testing/profiles

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

LGTM. If tested and works, I say pushit

::: build/automation.py.in
@@ +43,5 @@
>      import mozcrash
>  # ---------------------------------------------------------------
>  
> +_DEFAULT_PREFERENCE_FILE = os.path.join(SCRIPT_DIR, 'prefs_general.js')
> +

extra whitespace?

@@ +436,5 @@
>        manifestFile.close()
>  
>    def initializeProfile(self, profileDir, extraPrefs=[],
>                          useServerLocations=False,
> +                        initialProfile=None, prefFile=None):

While you're here it'd be nice to....

- fix extraPrefs=[] mutable default footgun
- split this into one arg per line (IMHO, obviously a stylistic choice)

@@ +441,5 @@
>      " Sets up the standard testing profile."
>  
> +    if not prefFile and os.path.isfile(_DEFAULT_PREFERENCE_FILE):
> +      prefFile = _DEFAULT_PREFERENCE_FILE
> +

Why not just have this the default argument?

@@ +460,5 @@
> +    if prefFile:
> +        f = open(prefFile, 'r')
> +        part = f.read() % {"server" : self.webServer + ":" + str(self.httpPort)}
> +        f.close()
> +        prefs.append(part)

if we insist on 2.7 (or at least 2.5, i think?), we should probably start using `with` in places like this.

Here is where I would check for file existence, not above.

I'd also tend to do '%s: %s' % (self.webServer, self.httpPort)
Attachment #733949 - Flags: review?(jhammel) → review+
(In reply to Jeff Hammel [:jhammel] from comment #29)
> @@ +460,5 @@
> > +    if prefFile:
> > +        f = open(prefFile, 'r')
> > +        part = f.read() % {"server" : self.webServer + ":" + str(self.httpPort)}
> > +        f.close()
> > +        prefs.append(part)
> 
> if we insist on 2.7 (or at least 2.5, i think?), we should probably start
> using `with` in places like this.

(2.6 by default; 2.5 with a 'from __future__' import)
Addressed nits and pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=1639292414a4
Try run looked good (pretty sure the winxp xpcshell bustage is unrelated).

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e5ad7ec4b6e
https://hg.mozilla.org/mozilla-central/rev/9e5ad7ec4b6e
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Blocks: 1023483
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: