727711 - interpolate files referencing the talos directory with the ${talos} pattern

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Jeff Hammel]

run_tests.py has a way of interpolating file paths wrt the talos
directory:

http://hg.mozilla.org/build/talos/file/56020a3a3bba/talos/run_tests.py#l506

This is currently only used for bundles and extensions, but should be
done for other items in .config/.yml files, including:
- profile paths (for each test)
- tpmanifest (for each test)
- head and tail (for each test)

See https://bugzilla.mozilla.org/show_bug.cgi?id=705809#c16

I may have missed something but at least these are required.

Also, and more painfully, the .config files will have to be edited to
add the '${talos}/' string in to each occurance of these, unless
bug 725097 is fixed first

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern. Patch 1 of 2, changes to run_tests.py and ffprocess;r=jhammel

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern. Patch 2 of 2, changes to config files;r=jhammel

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern. Patch 1 of 2, changes to run_tests.py and ffprocess;r=jhammel

Removed extraneous print statement. This also includes this fix: https://bug722476.bugzilla.mozilla.org/attachment.cgi?id=597129

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern;r=jhammel

Combine patches, take out part that changes path to 'remoteapp.ini'

Comment 5

[Jeff Hammel]

Comment on attachment 598411 [details] [diff] [review]
Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern;r=jhammel

looks good!

Comment 6

[Jeff Hammel]

I pushed this to try, https://tbpl.mozilla.org/?tree=Try&rev=8b51dd89b5f7

Windows, at least WinXP, seems to be having some problems, e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=9430189&tree=Try&full=1 :

Failed tsvg: 
		Stopped Fri, 17 Feb 2012 18:02:42
FAIL: Busted: tsvg
FAIL: unrecognized output format
Traceback (most recent call last):
  File "run_tests.py", line 668, in ?
    main()
  File "run_tests.py", line 665, in main
    test_file(arg, options, parser.parsed)
  File "run_tests.py", line 606, in test_file
    raise e
utils.talosError: 'unrecognized output format'
program finished with exit code 1
elapsedTime=24.484000
TinderboxPrint:<a href = "http://hg.mozilla.org/try/rev/8b51dd89b5f7">rev:8b51dd89b5f7</a>

I don't know why this is happening, but it is clearly bad :(

Comment 7

[Mozilla RelEng Bot]

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

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern;r=jhammel

I was able to track down a windows XP machine to debug - it looks like pageloader wasn't finding the manifest, perhaps due to spaces in the path or something else about file uris on windows.

Comment 9

[Jeff Hammel]

Comment on attachment 599282 [details] [diff] [review]
Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern;r=jhammel

-                        self.buildRemoteManifest(line.split(":")[1].strip())
+                        self.buildRemoteManifest(line.split("${talos}/")[1].strip())

Not sure what this line is but....I'm pretty sure we need to do something different

Comment 10

[Jeff Hammel]

(In reply to Jeff Hammel [:jhammel] from comment #9)
> Comment on attachment 599282 [details] [diff] [review]
> Bug 727711 - interpolate files referencing the talos directory with the
> ${talos} pattern;r=jhammel
> 
> -                        self.buildRemoteManifest(line.split(":")[1].strip())
> +                       
> self.buildRemoteManifest(line.split("${talos}/")[1].strip())
> 
> Not sure what this line is but....I'm pretty sure we need to do something
> different

So I would recommend:

1. putting interpolatePath in utils.py and importing it to PerfConfigurator

2.
manifest = line.split(":")[1].strip()
if manifest:
    self.buildRemoteManifest(manifest)

See also bug 729361 about making buildRemoteManifest be a little smarter about errors

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern;r=jhammel

Move interpolatePath to utils and call on manifest name, only call buildRemoteManifest is the manifest is not empty.

Comment 12

[Jeff Hammel]

Comment on attachment 599420 [details] [diff] [review]
Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern;r=jhammel

+
+here = os.path.dirname(os.path.realpath(__file__))

No need for this in PerfConfigurator.py

+    if test.get('tpmanifest'):
+        test['tpmanifest'] = '%s%s' % ('file://', test['tpmanifest'].replace(" ", "%20"))

I would probably do something like

import urllib # at the top of the file

tpmanifest = test.get('tpmanifest')
if tpmanifest:
    test['tpmanifest'] = 'file://%s' % urlquote(tpmanifest)

r+ if you remove the unusedhere from PerfConfigurator.py

Comment 13

[Jeff Hammel]

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

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern;r=jhammel

This version makes use of urllib.quote() function and deletes unnecessary 'here' assignment in PerfConfigurator.py.

Comment 15

[Mozilla RelEng Bot]

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

Comment 16

[Jeff Hammel]

Comment on attachment 599468 [details] [diff] [review]
Bug 727711 - interpolate files referencing the talos directory with the ${talos} pattern;r=jhammel

Looks great! Thanks a bunch!

Comment 17

[Jeff Hammel]

Probably needs android testing. The try run from comment 15 is pretty friggin green (the one red on android isn't even testing the right talos) but the patch has slightly changed since then

Comment 18

[Chris Manchester (limited bugmail, email directly)]

In light of bug https://bugzilla.mozilla.org/show_bug.cgi?id=730342 I will take a closer look at this for android and modify the patch appropriately.

Comment 19

[Chris Manchester (limited bugmail, email directly)]

Attachment: Unbitrot + fix for android

Fix manifest path to work for Android. I have this working for tdhtml on Android, Windows XP, and Mac (develop mode). Tested by running from the directory above talos/talos, eg python talos/run_tests.py -d -n talos/mytest.yml.

Realpath, which we use to get "here," appears to work (in part) by calling abspath, which calls normpath, so this works without these few lines: http://hg.mozilla.org/build/talos/file/099171bc6da5/talos/run_tests.py#l530

Comment 20

[Jeff Hammel]

Comment on attachment 601755 [details] [diff] [review]
Unbitrot + fix for android

+        test['tpmanifest'] = os.path.normpath('file:/%s' % (urllib.quote(test['tpmanifest'], '/\\t:\\')))

should this be 

'file://%s' % os.path.normpath(urllib.quote(test['tpmanifest'], '/\\t:\\') ?

Comment 21

[Jeff Hammel]

pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=5fe02fed627b

Comment 22

[Jeff Hammel]

This seems to pass on tsvg and ts locally

Comment 23

[Jeff Hammel]

But massive failures on windows:

talos-r3-w7-041: 
		Started Fri, 02 Mar 2012 10:57:56
Running test ts_places_generated_max: 
		Started Fri, 02 Mar 2012 10:57:56
'..' is not recognized as an internal or external command,
operable program or batch file.
NOISE: 
NOISE: __FAILbrowser non-zero return code (1)__FAIL
Failed ts_places_generated_max: 
		Stopped Fri, 02 Mar 2012 10:58:07
FAIL: Busted: ts_places_generated_max
FAIL: failed to initialize browser
Traceback (most recent call last):
  File "run_tests.py", line 671, in ?
    main()
  File "run_tests.py", line 668, in main
    test_file(arg, options, parser.parsed)
  File "run_tests.py", line 609, in test_file
    raise e
utils.talosError: 'failed to initialize browser'
program finished with exit code 1
elapsedTime=14.187000

Comment 24

[Mozilla RelEng Bot]

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

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Attachment: Fix for Windows

This works for windows, testing with a directory structure similar to the automation machines (using ../firefox/firefox as the browser path). Needed to reinstate calling normpath on the browser path because it doesn't make use of ${talos} at the moment.

As we talked about briefly on irc, 

'file://%s' % os.path.normpath(urllib.quote(test['tpmanifest'], '/\\t:\\')

results in pageloader displaying the contents of the manifest in the browser rather than loading the pages (tested on Android 2.3.6). This may warrant further investigation.

Comment 26

[Jeff Hammel]

Comment on attachment 602800 [details] [diff] [review]
Fix for Windows

looks good!

I still would love to figure out the file:// issue on android

Comment 27

[Jeff Hammel]

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

needs testing on android

Comment 28

[Mozilla RelEng Bot]

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

Comment 29

[Jeff Hammel]

looks pretty green

pushed: http://hg.mozilla.org/build/talos/rev/1b66e67c9fa5

:chmanchester, can you file a follow-up bug about what you were seeing with the file:/ vs file:// issue?  Thanks, and thanks for the patch!  Glad it stuck this time