719270 - PerfConfigurator should accept multiple --extension arguments

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

For no particular reason, PerfConfigurator uses --extension to really
mean one single extension.  The yml actually specifies a list, which
is good, but the front-end only allows one.  Instead,
`action='append'` should be used to allow OptionParser to put these in
a list, and
http://hg.mozilla.org/build/talos/file/d93f2b21985c/talos/PerfConfigurator.py#l162
should be altered to support multiple extensions

Comment 1

[Jeff Hammel]

Attachment: allow multiple extensions

Comment 2

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

Comment on attachment 593580 [details] [diff] [review]
allow multiple extensions

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

r=me if we can verify there is nothing using the --extension parameter.  

The singular name looks fine:
python Perfconfigurator.py --extension k0s.xpi --extension k1s.xpi

The plural name doesn't look very good:
python Perfconfigurator.py --extensions k0s.xpi --extensions k1s.xpi


What is the intended behavior here?

::: talos/PerfConfigurator.py
@@ +31,5 @@
>  class PerfConfigurator(object):
>      attributes = ['browser_path', 'configPath', 'sampleConfig', 'outputName', 'title',
>                    'branch', 'branchName', 'buildid', 'currentDate', 'browserWait',
>                    'verbose', 'testDate', 'useId', 'results_url',
> +                  'activeTests', 'noChrome', 'fast', 'testPrefix', 'extensions',

hmm, this might break existing scripts, particularly the AMO addon testing.

@@ +173,5 @@
>  
>              if self.symbolsPath:
>                  newline += '\nsymbols_path: %s\n' % self.symbolsPath
> +        if self.extensions and ('extensions : {}' in line):
> +            newline = 'extensions:\n' + '\n'.join([(' - %s' % extension) for extension in self.extensions])

this line just isn't very readable.  Mostly the first newline.

Comment 3

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #2)
> Comment on attachment 593580 [details] [diff] [review]
> allow multiple extensions
> 
> Review of attachment 593580 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me if we can verify there is nothing using the --extension parameter.  

Not sure I know how to do that. 

> The singular name looks fine:
> python Perfconfigurator.py --extension k0s.xpi --extension k1s.xpi
> 
> The plural name doesn't look very good:
> python Perfconfigurator.py --extensions k0s.xpi --extensions k1s.xpi
> 
> 
> What is the intended behavior here?

The former is how the patch works.

> ::: talos/PerfConfigurator.py
> @@ +31,5 @@
> >  class PerfConfigurator(object):
> >      attributes = ['browser_path', 'configPath', 'sampleConfig', 'outputName', 'title',
> >                    'branch', 'branchName', 'buildid', 'currentDate', 'browserWait',
> >                    'verbose', 'testDate', 'useId', 'results_url',
> > +                  'activeTests', 'noChrome', 'fast', 'testPrefix', 'extensions',
> 
> hmm, this might break existing scripts, particularly the AMO addon testing.

:(  Anywhere I can look to figure this out?

> @@ +173,5 @@
> >  
> >              if self.symbolsPath:
> >                  newline += '\nsymbols_path: %s\n' % self.symbolsPath
> > +        if self.extensions and ('extensions : {}' in line):
> > +            newline = 'extensions:\n' + '\n'.join([(' - %s' % extension) for extension in self.extensions])
> 
> this line just isn't very readable.  Mostly the first newline.

IMHO its no worse than how it was before.  The entire function is lousy.  I can file a blocker to clean it up if you want but I consider that beyond the scope of the patch.

Comment 4

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

Comment on attachment 593580 [details] [diff] [review]
allow multiple extensions

I misread this patch and we are keeping the commandline argument the same!

Comment 5

[Jeff Hammel]

reference: http://mxr.mozilla.org/build/source/buildbotcustom/steps/talos.py#66

Comment 6

[Jeff Hammel]

As far as I can tell, this change should not break the code from http://mxr.mozilla.org/build/source/buildbotcustom/steps/talos.py#66

Comment 7

[Mozilla RelEng Bot]

Try run for b2eb6f3f4272 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b2eb6f3f4272
Results (out of 80 total builds):
    success: 79
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-b2eb6f3f4272

Comment 8

[Jeff Hammel]

The try results look good.  Also tried on mobile successfully with ts and tsvg.  Going to push

Comment 9

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/4f9e229d378e