run_tests.py reparses the configuration from the YAML file that was generated from PerfConfigurator.py (or remotePerfConfigurator.py). Often there is a use case for a developer to want the original configuration file but to desire to override any of the attributes to `def test_file` on a per-run basis, such as the results server URL, the csv_dir or many of the required and optional parameters in browser_config. Using the same options and parser from PerfConfigurator.TalosOptions, run_tests.py should be able to do this in a fairly straight-forward way. --screen and --noisy already serve this general purpose and can be extended by run_tests.py's command line parser. In addition, --amo should be moved to PerfConfigurator.py

A few notes on this (will need some tweaking pending feedback): -d -s and -n are conflicts with names in TalosOptions - this patch lets d stand for debug, s stand for screen, and n for noisy, as they have been in the run_tests optparser (removes equivalents in TalosOptions). While this lets run_tests use TalosOptions, updating some of these options will not be meaningful at the time tests are run - for instance, TalosOptions stores --resultsURL in the name "resultsURL", the value of which is then written to results_url in the generated config file. "resultsURL" is not a name used by run_tests, so overriding it outside the context of PerfConfigurator has no effect. It seems simpler to me to get rid of using the name "resultsURL" at all, and use "results_url" throughout. This impacts a number of options in TalosOptions, including --executablePath (stored as exePath, later written to browser_path). If we think it's a good idea, I'll be happy to redo naming in TalosOptions to make it consistent with the actual keys in the config file. Related to this, specifying -a when running run_tests will not change the tests run. There is a similar name issue, but also some things in PerfConfigurator that need to happen to switch the tests to be run. I think this is sort of out of the scope of this bug, but something to keep in mind as we work towards https://bugzilla.mozilla.org/show_bug.cgi?id=704654, and with respect to https://bugzilla.mozilla.org/show_bug.cgi?id=717685

Comment on attachment 590127 [details] [diff] [review] Bug 716921 - run_tests.py should use PerfConfigurator.py options for command line overrides;r=jhammel Review of attachment 590127 [details] [diff] [review]: ----------------------------------------------------------------- This is simple and nice. At first glance I don't like removing the -d, -n, -s from PerfConfigurator, but I don't believe we are using them in tbpl production. Does this overwrite values in the config file? Will we be able to run tests without a config file now?

Thanks for the feedback - as it stands, this does not overwrite the config file at all, it only updates the dict (yaml_config) used in run_tests.py. So, we still can't run tests without a config file, but between this and https://bugzilla.mozilla.org/show_bug.cgi?id=717685, we are getting close.

FTR I think the goal of running without a config file is beyond the scope of this bug (but is part of bug 704654). When we do that, and PerfConfigurator (as we know it) goes away [*], we will still want a --dump <path> option so that we can dump the configuration for reproducibility and sharing.

Comment on attachment 590127 [details] [diff] [review] Bug 716921 - run_tests.py should use PerfConfigurator.py options for command line overrides;r=jhammel Like :jmaher, I am worried abut the collision of the command line options with what is done in production. We should probably scour the talos logs of http://tbpl.mozilla.org/ to ensure that '-d', '-n', and '-s' are not used via PerfConfigurator, and maybe give a general ping out before landing. It would be a good improvement to publicize anyway :) (And if I can get off my lazy keister, a good thing to blog about). + # allow run_tests.py to use TalosOptions without these checks + if __file__ is "PerfConfigurator.py": + if args: + raise Configuration("Configurator does not take command line arguments, only options (arguments were: %s)" % (",".join(args))) Please don't do this. The check is brittle and with this change we should move the check to the `main()` functions that consume this method vs keeping it here. Also, `__file__ is "PerfConfigurator.py"` is a check if the instance of the string __file__ *is* (read: has the same location in memory) as the instance of the literal string "PerfConfigurator.py". The python interpreter may cache things this way, or may not. You really want == for checks like this (this is the same as === in JS, AIUI). + if options[option] is not defaults[option]: As above, do not use `is` here, use `==`. Ideally we would have a pattern whereby the default value of the option == undefined if it isn't parsed; see http://k0s.org/mozilla/hg/bzconsole/file/85357b075211/bzconsole/command.py#l12 , but I am happy taking this as a follow up. (TL;DR: if someone specifies an option that happens to be the default using a .yml file where it is not the default they will get the .yml config value and not the default value that they verbosely specified, which is not what is implied.) + yaml_config.update({option:options.get(option)}) Please use yaml_config[option] = options[option] You could also use the more fancy python yaml_config.update(dict([(i,j) for i, j in options.items() if j != defaults[i]])) This looks really good. I have to r- it for the __file__ is "PerfConfigurator.py" check, but other than that I think it is ready. I can help with that if you want

Just an example of what i am talking about for the undefined marker pattern for optparse options. This could use some work, but hopefully the basic pattern is evident

So I think the best thing to do is just take the patch as is with the `if args` checking moved to the PerfConfigurator.py:main function and change 'is' to '==' for the checks and take the Undefined/multiple configuration pattern as a follow-up

Sounds good. I've also updated: resultsURL, exePath and logFile from TalosOptions to correspond to the actual key names in the config file so that these have the desired impact when overriding from run_tests.py.

Comment on attachment 590374 [details] [diff] [review] Moves check for args and active tests from verifycommandline to main Looks great! Thanks for doing this!

I was running this on try server unsuccessfully. What I found is that we references options.to_screen and we changed it to options.screen. Also I need to access the variables options.to_screen and options.amo as options['to_screen'] and options['amo']. This is now running green on try server. There are a lot of changes from the previous patch due to bitrot and shifting of code. But the core changes are outlined above. If review is ok, I am ready to land.

Comment on attachment 590576 [details] [diff] [review] Moves check for args and active tests from verifycommandline to main (unbitrot + fixes) Review of attachment 590576 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me, thanks Joel. (r+ although I don't seem to have permission to set the flag)

Comment on attachment 590576 [details] [diff] [review] Moves check for args and active tests from verifycommandline to main (unbitrot + fixes) Good catches, Joel, and thanks for testing!

landed: http://hg.mozilla.org/build/talos/rev/757fb43d5eba

(In reply to Jeff Hammel [:jhammel] from comment #6) > Created attachment 590346 [details] > simple example of the Undefined for optparse + other configuration > > Just an example of what i am talking about for the undefined marker pattern > for optparse options. This could use some work, but hopefully the basic > pattern is evident I've tried to get this working, but currently TalosOptions stores defaults in a dict separate from kwargs passed to add_option: http://hg.mozilla.org/build/talos/file/ca816866e975/talos/PerfConfigurator.py#l325 This means the Undefined wont find kwargs.get('default') if it's being used as in the example. I've tried a few things but don't have it working. I can't tell at the moment why TalosOptions stores the defaults in a dict - would reversing this to store options in kwargs be an acceptable solution? (this might belong in it's own bug)

> I can't tell at the moment why TalosOptions stores the defaults in a dict - would reversing this to store options in kwargs be an acceptable solution? (this might belong in it's own bug) AFAIK, we do it this way in Talos because other test frameworks at Mozilla do it this way. In other words, probably less than a compelling reason. I'll file a follow-up bug for the rest of this

(In reply to Jeff Hammel [:jhammel] from comment #17) > > I can't tell at the moment why TalosOptions stores the defaults in a dict - would reversing this to store options in kwargs be an acceptable solution? (this might belong in it's own bug) > > AFAIK, we do it this way in Talos because other test frameworks at Mozilla > do it this way. In other words, probably less than a compelling reason. > > I'll file a follow-up bug for the rest of this https://bugzilla.mozilla.org/show_bug.cgi?id=720773

This breaks remotePerfConfigurator: │python remotePerfConfigurator.py -v -e org.mozilla.fennec --develop --activeTests tsvg --output tsvg.yml --remoteDevice 10.251.28.71 reconnecting socket Traceback (most recent call last): File "remotePerfConfigurator.py", line 220, in <module> sys.exit(main()) File "remotePerfConfigurator.py", line 205, in main configurator = remotePerfConfigurator(**options.__dict__) File "remotePerfConfigurator.py", line 16, in __init__ pc.PerfConfigurator.__init__(self, **options) File "/home/jhammel/mozilla/src/talos/src/talos/talos/PerfConfigurator.py", line 311, in __init__ self.buildid = self._getCurrentBuildId() File "/home/jhammel/mozilla/src/talos/src/talos/talos/PerfConfigurator.py", line 60, in _getCurrentBuildId masterContents = self._getMasterIniContents() File "remotePerfConfigurator.py", line 134, in _getMasterIniContents if (self.exePath.startswith('org.mozilla.f')): AttributeError: 'remotePerfConfigurator' object has no attribute 'exePath'

fixed along with http://hg.mozilla.org/build/talos/rev/05f01e049452