755374 - move browser_config creation to PerfConfigurator.py

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

browser_config has heavy overlap between PerfConfigurator config.
Ideally, one would just use PerfConfigurator config and deprecated
browser_config entirely.  More realistically, the part of
run_tests.run_test that generates browser_config should be made a
method on PerfConfigurator and passing the PerfConfigurator object, vs
just its config dictionary, to run_tests.run_tests

Comment 1

[Jeff Hammel]

Attachment: example

I've taken maximus's work from http://pastebin.mozilla.org/1642233 and made a diff out of it, also passing PerfConfigurator instance to the run_tests function.  This is pretty rough, but mostly a good first pass. It should be tested and confirmed to work.  In addition, there's a lot of streamlining that could happen here.  PerfConfigurator already has explicit knowledge of what variables its going to have and we replicate a bunch of these in a silly way

Comment 2

[jdev005]

Attachment: any changes required ?

Comment 3

[Jeff Hammel]

Comment on attachment 650242 [details] [diff] [review]
any changes required ?

+    def browser_config(self,config,title):

Since you're in the PerfConfigurator class you already have the config and don't need to pass it.  Its just self.config

We also don't need title.  We can move the line that gets it from run_tests.py to PerfConfigurator and have run_tests.py get it from browser_config (or just repeat the lines)

You also have two sections to patch run_tests.py.  I'm not sure how or why you did it, but it would be nice to have a single diff of the file.

Other than that, the basic idea is right :)  Thanks for the patch!

Nits:

Not important, but things I'd like:

+def run_tests(parser):

I'd rather this variable be called 'configurator' for consistency, but not a big deal

Comment 4

[jdev005]

Attachment: required changes made

Comment 5

[Jeff Hammel]

Comment on attachment 650591 [details] [diff] [review]
required changes made

This looks great!  Thanks for the patch!

Comment 6

[jdev005]

no problem :)

Comment 7

[Jeff Hammel]

pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1f5da62f9c95

Comment 8

[sam]

i'm looking to get involved with the mozilla community. anyone willing to help me get started on this bug, if not already assigned?

Comment 9

[Jeff Hammel]

looks green!  pushed: http://hg.mozilla.org/build/talos/rev/130ecfc6caeb

Comment 10

[Jeff Hammel]

(In reply to sam from comment #8)
> i'm looking to get involved with the mozilla community. anyone willing to
> help me get started on this bug, if not already assigned?

Sorry, this one's taken :)