722794 - TalosOptions should store options consistent with config keys

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

With https://bugzilla.mozilla.org/show_bug.cgi?id=716921 we are now able to override talos options in a config file while running run_tests.py. However, due to the names in TalosOptions, this will not have the desired result for some options. This is because TalosOptions associates these options with names that are not known to the config used by tests, for instance:

http://hg.mozilla.org/build/talos/file/9c1b3addb9ee/talos/PerfConfigurator.py#l382

When this is written to the config file, it is associated with 'browser_wait', not 'browserWait'. If overridden while running run_tests, this will add 'browserWait' as a key in the config object but have no impact on tests run.

Instead, the names in TalosOptions should match whatever key is in the config so that overrides will have the desired result. This will impact the way PerfConfigurator as well as remotePerfConfigurator write out config files based on options.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 722794 - TalosOptions should store options consistent with config keys; r=jmaher, jhammel

...and here's a patch.

I've left the switches themselves alone	for now. I've gone through all of the names in PerfConfigurator and remotePerfConfigurator that are written directly to the config file.

Changed	names:

(PerfConfigurator)

branchName: branch_name

testDate: testdate

browserWait: browser_wait

extension: extensions

symbolsPath: symbols_path

addonId: addon_id

webServer: webserver

(remotePerfConfigurator)

remoteDevice: deviceip

remotePort: deviceport

deviceRoot: deviceroot

Comment 2

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

Comment on attachment 593722 [details] [diff] [review]
Bug 722794 - TalosOptions should store options consistent with config keys; r=jmaher, jhammel

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

we send some values to bcontroller via bcontroller.yml.  I don't think this fiddles with it much, but something to double check.  A few things that look like we are changing the case on text that would come from the .config file vs the commandline.

::: talos/PerfConfigurator.py
@@ +187,5 @@
>                  lfile = os.path.abspath(lfile)
>  
>              lfile = lfile.replace('\\', '\\\\')
>              newline = '%s: %s\n' % (parts[0], lfile)
> +        if 'branch' in line:

hmm, this is searching for text in a .config file, I think this might be wrong or could match on >1 line?

@@ +423,2 @@
>                          help = "Extension to install while running")
> +        defaults["extensions"] = ''

jhammel recently fiddled with this name/value, could be bitrot or something similar.

::: talos/remotePerfConfigurator.py
@@ +108,5 @@
>                  if (part <> parts[-1]):
>                      newline += ' '
>  
>          #take care of tpan/tzoom tests
> +        newline = newline.replace('webserver=', 'webserver=' + self.webserver);

hmm, this might be invalid since we are matching webServer in the .config file.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Thanks for taking a look at this - I am unbitrotting now and will double check what gets matched from the .config file for each case.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: Unbitrot + fixes: TalosOptions should store options consistent with config keys

Names is bcontroller.yml look consistent with the changes here. 

I've reverted the change for 'testbranch' and fixed the 'webserver' case per feedback. Once unbitrotted the recent changes to 'extensions' vs. extension complement this well.

Comment 5

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

Comment on attachment 593895 [details] [diff] [review]
Unbitrot + fixes: TalosOptions should store options consistent with config keys

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

great, thanks

Comment 6

[Jeff Hammel]

Comment on attachment 593895 [details] [diff] [review]
Unbitrot + fixes: TalosOptions should store options consistent with config keys

Awesome :)

For future work we should ensure that these values stay the same.  https://bugzilla.mozilla.org/show_bug.cgi?id=717395 is a start to this.

I will push this to Try and hopefully be able to test locally on android (ts, tsvg, unless :jmaher thinks more extensive tests are needed).

Comment 7

[Mozilla RelEng Bot]

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

Comment 8

[Jeff Hammel]

Seems to work on android testing.  Pushing

Comment 9

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/6e8053f3c262