Closed Bug 722476 Opened 12 years ago Closed 12 years ago

Talos tests should have a manifest section in the configuration file

Categories

(Testing :: Talos, defect)

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

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: k0scist, Unassigned)

Details

Attachments

(3 files, 5 obsolete files)

Bug 722476 - Talos tests should have a manifest section in the configuration file. Changes to PerfConfigurator and run_tests (Patch 2 of 2);r=jmaher
7.58 KB, patch
Details | Diff | Splinter Review
Bug 722476 - Talos tests should have a manifest section in the configuration file. Changes to config files (1 patch of 2);r=jmaher
32.57 KB, patch
Details | Diff | Splinter Review
Fix to bug722476 - missing newline after 'tpmanifest: %s' in PerfConfigurator.py;r=jhammel
1.15 KB, patch
k0scist
: review+
Details | Diff | Splinter Review 
Talos configuration is somewhat of a black and mystical art.  One of
the magical things is the intrinsic not explicit link to the test name
and the manifest.  For example, see several of the tests around here:

http://hg.mozilla.org/build/talos/file/9c1b3addb9ee/talos/sample.config#l169

- name: tp
  url :  '-tp page_load_test/tp3.manifest -tpchrome -tpnoisy -tpformat
  tinderbox -tpcycles 10'
  resolution : 20
  win_counters : ['Working Set', 'Private Bytes', '% Processor Time']
  w7_counters : ['Working Set', 'Private Bytes', '% Processor Time',
  'Modified Page List Bytes']
  linux_counters : ['Private Bytes', 'RSS', 'XRes']
  mac_counters : ['Private Bytes', 'RSS']
  shutdown : True

This defines the 'tp' test.  You see we have a manifest,
page_load_test/tp3.manifest but it is hidden in the poorly named url
option.  Really, probably all of these things should be more
configurable, but what you're really specifying is:

- what is the manifest?
- run in chrome mode or not?
- what counters to use?
- number of cycles?

While these are all separate issues, lets take the manifest one
first.  Instead we should have a `manifest` key:

 - manifest: page_load_test/tp3.manifest

Then "url" (ahem) should add the CLI option "-tp ${MANIFEST}" based on
this.

Really this is a part of an effort to become more explicit and
therefore having finer control.  If someone wanted to take demangling
more of this, I have some ideas there too;  for one, I would like the
manifest to ideally be a URI vs a local path for more canonical
results reporting.
I would like to have a pageloader section as we do basetest:

pageloader:
  manifest: 
  chrome: True
  noisy: True
  format: tinderbox
  cycles: 10

Then for tp3, we could have:
  manifest: page_load_test/tp3.manifest


What this would do is make url = '' and manifest != ''.  If that is the case we have a pageloader test.  

Alternatively we could have different types of tests (defined as we do basetest):
startuptests:
 option: default

pageloadertest:
 option: default

Then we could have tp3:

-tp3:
 base: pageloadertest
 manifest: page_load_test/tp3.manifest

I don't like this approach as much, but it is a bit less confusing and behind the scenes magic.
I also prefer the first method:  if a test section has a manifest, it is a pageloader test.  Otherwise it is a startup test.
This will allow override for override of manifest and pageloader cycles when running PerfConfigurator or run_tests. These seem like common things to override, although others can be included here.

This leaves the url name and field alone as the tests are run, but reconstructs it in run_tests from options.
Attachment #594642 - Flags: review?(jmaher)
Attachment #594642 - Flags: feedback?(jhammel)
Comment on attachment 594642 [details] [diff] [review]
Talos tests should have a manifest section in the configuration file;r=jmaher

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

can we make sure we have the latest version of sample.config before this patch.  We have refactored it to have a BaseTest section and it looks like you might be using and older version.

In general we should make all pageloader options have a tp in front of them (yes I know responsiveness, mozafterpaint and rss don't), that would be a good bug to add to the system.

r- for the reuse of cycles and the test=buildcommandline() call as well.  This bug was not the best defined and I think you nailed it 95% on the head.

::: talos/PerfConfigurator.py
@@ +113,5 @@
> +                        if self.noChrome:
> +                            line += "  chrome: False\n"
> +
> +                        if self.cycles:
> +                            line += "  cycles: %s\n" % self.cycles

tpcycles please.

@@ +144,5 @@
> +                if "manifest:" in line:
> +                    if self.manifest:
> +                        line = "  manifest: %s" % self.manifest
> +                    if self.develop:
> +                        line = line.strip("\n") + ".develop\n"

what is the develop option doing here?

@@ +519,5 @@
> +        defaults["manifest"] = ""
> +        self.add_option('--cycles', type='int',
> +                        action='store', dest='cycles',
> +                        help="number of pageloader cycles to run")
> +        defaults["cycles"] = None

tpcycles and maybe we should use tpmanifest as well?

::: talos/run_tests.py
@@ +440,5 @@
> +  # Build command line from options
> +  for test in tests:
> +    if not test['url']:
> +      test = buildCommandLine(test, options)
> +

should we reassign test['url'] here instead of test?

::: talos/sample.config
@@ +223,3 @@
>  - name: tgfx
> +  manifest: page_load_test/gfx/gfx.manifest
> +  cycles: 5

we will have a conflict with talos cycles; pageloader options should be prepended with a tp or something else:
tpcycles: 5
Attachment #594642 - Flags: review?(jmaher) → review-
Thanks Joel. Changes per review: pageloader options start with "tp", assign to test['url'] rather than test.

In PerfCongfigurator, the extra check for self.develop adds .develop to the manifest name so that tests run locally (although I suppose if we can override from the command line this is not as important).
Attachment #594642 - Attachment is obsolete: true
Attachment #594642 - Flags: feedback?(jhammel)
Attachment #594721 - Flags: review?(jmaher)
Attachment #594721 - Flags: feedback?(jhammel)
Comment on attachment 594721 [details] [diff] [review]
Talos tests should have a manifest section in the configuration file;r=jmaher

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

r=me.  There are a lot of nits and questions here and I would appreciate an updated patch that resolves some of these.  Otherwise this is great stuff.

::: talos/PerfConfigurator.py
@@ +116,5 @@
> +                        if self.tpcycles:
> +                            line += "  tpcycles: %s\n" % self.tpcycles
> +
> +                        if self.mozAfterPaint:
> +                            line += "  tpmozafterpaint: True\n"

Is there a way we could do this smarter?  Maybe a list of tp options or basetest options?  My thoughts are if we add a new test or pageloader option we have to hack up too many places.

@@ +144,5 @@
> +                if "tpmanifest:" in line:
> +                    if self.tpmanifest:
> +                        line = "  tpmanifest: %s" % self.tpmanifest
> +                    if self.develop:
> +                        line = line.strip("\n") + ".develop\n"

I want to ensure that we do create the .develop version of the manifest before assigning this.  Also we should ensure this is the place to add it.  If it is, I would prefer a comment here as this is confusing at first glance.

::: talos/run_tests.py
@@ +398,5 @@
>          if test[item] is None:
>            test[item] = ''
>    return tests
> +  
> +def buildCommandLine(url, test, options):

we pass in url, but assign it right away.

@@ +401,5 @@
> +  
> +def buildCommandLine(url, test, options):
> +  for item in test:
> +    if options.get(item):
> +      test[item] = options[item]

do we do this to get all the options["responsiveness"] and other things which live in options?  Seems we could do a better job of say what goes where, although that could be a followup bug.

@@ +409,5 @@
> +  if test['tpmozafterpaint']:
> +    url += ' -tpmozafterpaint'
> +  if test['tpnoisy']:
> +    url += ' -tpnoisy'
> +  url += ' -tpformat {tpformat} -tpcycles {tpcycles}'.format(**test)

can we sanity check these values before assigning?  

tpcycles is an integer between 1 and 1000?
tpformat can be 'js', 'jsfull', 'text', 'tinderbox'

@@ +419,1 @@
>  

we need tpdelay and we should consider some of the other tp items.

@@ +439,5 @@
>  
> +  # Build command line from options
> +  for test in tests:
> +    if not test['url']:
> +      test['url'] = buildCommandLine(test['url'], test, options)

in this case we don't have a test['url'], do we need to pass it into buildCommandLine?

::: talos/sample.config
@@ +120,1 @@
>  

can we add tpdelay in here?  We use that for remote.config
Attachment #594721 - Flags: review?(jmaher) → review+
This addresses much of the review, and I maybe the scope of this bug, but let's keep the discussion going on further improvements.

Good call on the "develop" manifest, the previous version of this patch was preventing it from being generated at all.

I've combed through remote.config as well - this will allow overriding the manifest name from remotePerfConfigurator.
Attachment #594721 - Attachment is obsolete: true
Attachment #594721 - Flags: feedback?(jhammel)
Attachment #594925 - Flags: review?(jmaher)
Comment on attachment 594925 [details] [diff] [review]
Unbitrot + fixes: Talos tests should have a manifest section in the configuration file

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

I know this is a lot of feedback on this, but we can do this in a followup bug if you wish.  Overall I like this and it solves the purpose of the bug.

::: talos/PerfConfigurator.py
@@ +116,5 @@
> +                        if self.tpcycles:
> +                            line += "  tpcycles: %s\n" % self.tpcycles
> +
> +                        if self.mozAfterPaint:
> +                            line += "  tpmozafterpaint: True\n"

what about tpdelay?  I believe rss is taken care of already, but good to check.

@@ +139,5 @@
>                  if self.responsiveness and 'responsiveness' in line:
>                      line = ""
>  
>                  if self.noShutdown and 'shutdown :' in line:
>                      line = ""

these lines can be in 'replacements' as well

@@ +148,5 @@
> +                    # if --develop flag specified, generate .develop manifest
> +                    # and change manifest name in generated config file
> +                    if self.develop:
> +                        self.buildRemoteManifest(line.split(":")[1].strip())
> +                        line = line.strip("\n") + ".develop\n"

This solves the problem, but I think we can do it cleaner.

1) add tpmanifest to the replacements section
2) conditionally build the remote manifest in general.  We cal buildRemoteManifest from convertUrlToRemote() and convertUrlToRemote could be cleaned up to not have the check for .manifest now.

@@ +151,5 @@
> +                        self.buildRemoteManifest(line.split(":")[1].strip())
> +                        line = line.strip("\n") + ".develop\n"
> +
> +                if self.tpcycles and 'tpcycles' in line:
> +                    line = ""                

it would be nice to add this to the 'replacements' section near the attributes.

::: talos/run_tests.py
@@ +402,5 @@
> +def buildCommandLine(test, options):
> +  
> +  # override with values from command line
> +  for item in test:
> +    print item

unnecessary print, this could be when debug is on though

@@ +411,5 @@
> +  if test['tpcycles'] not in range(1, 1000):
> +    raise talosError('pageloader cycles must be int 1 to 1,000')
> +  if test['tpformat'] not in ('js', 'jsfull', 'text', 'tinderbox'):
> +    raise talosError('pageloader format not recognized. valid ' +
> +                     'formats are: js, jsfull, text, and tinderbox')

can we add a check for tpdelay here as well?
Attachment #594925 - Flags: review?(jmaher) → review+
This was getting pretty hefty so I split the patch - these are changes to config files, adding pageloader options to each basetest section, and adding basetest to cycles.config.
Attachment #594925 - Attachment is obsolete: true
Attachment #595554 - Flags: review?(jmaher)
As discussed on IRC, adding options from basetest/tests section to replacements results in indentation conflict as it is right now. Cleaned up search and replace through 'url' field.
Attachment #595556 - Flags: review?(jmaher)
Comment on attachment 595554 [details] [diff] [review]
Bug 722476 - Talos tests should have a manifest section in the configuration file. Changes to config files (1 patch of 2);r=jmaher

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

looks great!
Attachment #595554 - Flags: review?(jmaher) → review+
Comment on attachment 595556 [details] [diff] [review]
Bug 722476 -  Talos tests should have a manifest section in the configuration file. Changes to PerfConfigurator and run_tests (Patch 2 of 2);r=jmaher

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

wow, great stuff.
Attachment #595556 - Flags: review?(jmaher) → review+
fix a bit of bitrot and pushed to try; will check in if all is well in a few hours!
Traceback (most recent call last):
  File "run_tests.py", line 647, in <module>
    main()
  File "run_tests.py", line 644, in main
    test_file(arg, options, parser.parsed)
  File "run_tests.py", line 457, in test_file
    test['url'] = buildCommandLine(test, options)
  File "run_tests.py", line 425, in buildCommandLine
    url += ' -tpformat {tpformat} -tpcycles {tpcycles}'.format(**test)
AttributeError: 'str' object has no attribute 'format'
Try run for 432cd854917f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=432cd854917f
Results (out of 12 total builds):
    exception: 4
    success: 2
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-432cd854917f
(In reply to Joel Maher (:jmaher) from comment #14)
> Traceback (most recent call last):
>   File "run_tests.py", line 647, in <module>
>     main()
>   File "run_tests.py", line 644, in main
>     test_file(arg, options, parser.parsed)
>   File "run_tests.py", line 457, in test_file
>     test['url'] = buildCommandLine(test, options)
>   File "run_tests.py", line 425, in buildCommandLine
>     url += ' -tpformat {tpformat} -tpcycles {tpcycles}'.format(**test)
> AttributeError: 'str' object has no attribute 'format'

should be 

url += ' -tpformat %(tpformat)s -tpcycles %(tpcycles)s' % test

or something more programmatic than that
Typo fix for two tests in cycles.config and sample.config. I wrote "tpmanifest" key as "tpanifest".
Try run for 606b25325c17 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=606b25325c17
Results (out of 56 total builds):
    success: 47
    failure: 9
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-606b25325c17
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
--tpmanifest doesn't quite do what you want:

python PerfConfigurator.py --tpmanifest foo.txt -a tsvg -e `which firefox` -o foo.yml

results in:

"""
...
tests :
- name: tsvg
  tpmanifest: foo.txt  tpcycles: 5
"""
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #597129 - Flags: review? → review?(jhammel)
Comment on attachment 597129 [details] [diff] [review]
Fix to bug722476 - missing newline after 'tpmanifest: %s' in PerfConfigurator.py;r=jhammel

looks good! thanks!
Attachment #597129 - Flags: review?(jhammel) → review+
Whiteboard: [talos-checkin-needed]
Try run for 455078615d8f is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=455078615d8f
Results (out of 52 total builds):
    exception: 1
    success: 51
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmaher@mozilla.com-455078615d8f
 Timed out after 12 hours without completing.
http://hg.mozilla.org/build/talos/rev/caf24bd3f566
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [talos-checkin-needed]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: