705809 - Talos should not depend on scripts being run from the talos directory

Status: RESOLVED FIXED

(Testing :: Talos, defect, P2)

Description

[Jeff Hammel]

Currently, Talos requires that run_test.py, etc, be run with the CWD
being the directory of the script.  This should be changed and instead
use paths relative to the script to use sibling resources.  As a
non-thorough examples list:

talos/bcontroller.py

+                       'xtalos\\start_xperf.py',
etc
=>
os.path.join(here, xtalos, start...)

+++ b/talos/ffprocess.py
+        return [sys.executable, 'bcontroller.py',
+                '--configFile', browser_config['bcontroller_config']]
=>
os.path.join(here, 'bcontrollery.py')

where `here = os.path.dirname(os.path.abspath(__file__))`

Comment 1

[Jeff Hammel]

The first step is to figure out what all needs this fix.

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: Bug 705809 - Talos should not depend on scripts run from the talks directory

Added abs_path method in utils.py, which converts a path to an absolute path based on the location of run_tests.py and normalizes to work cross-platform. It or something like it is used in the following locations:

In PerfConfigurator.py:
   Use re to add full path name to URL field of config file if -tp is found. (This depends on "-tp" being in the line)
   Change default output file path
   Change default logfile path

In ffprocess.py:
   Set bcontroller.py path

In remotePerfConfigurator.py:
   Set "files" list to full paths


In run_tests.py:
   Set path when using remoteapp.ini
   Set browser_path and bcontroller_conf paths
   Set path for each dir and each bundle in browser_config
   
In ttest.py:
   set profile_path in test_config

Comment 3

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

Comment on attachment 586980 [details] [diff] [review]
Bug 705809 - Talos should not depend on scripts run from the talks directory

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

wow, pretty complete patch here!  A few observations/questions below.  I am sure jhammel will have his own set of things that I didn't address.

::: talos/PerfConfigurator.py
@@ +108,5 @@
> +                    here = os.path.dirname(os.path.abspath(__file__))
> +                    rePath = re.compile('\S*/\S*\s')
> +                    reSearch = re.search(rePath, line)
> +                    reRes = here + '/' + reSearch.group(0)
> +                    line = re.sub(rePath, reRes, line)

this seems problematic.  The url line is based on relative to the webroot vs a filesystem.  Can you give an example of a before/after line that would be affected by this?

::: talos/remotePerfConfigurator.py
@@ +90,3 @@
>  
>          talosRoot = self.deviceRoot + '/talos/'
>          for file in files:

maybe we could just do a:
file = utils.abs_path(file)

this would save a lot of the changes above.

::: talos/ttest.py
@@ +140,5 @@
>  
>      def initializeProfile(self, profile_dir, browser_config):
>          if not self._ffsetup.InitializeNewProfile(profile_dir, browser_config):
>              raise talosError("failed to initialize browser")
> +        time.sleep(browser_config['browser_wait'])

this line was removed in bug 685183 which landed this past weekend...looks like it got in here on accident.

Comment 4

[Jeff Hammel]

Comment on attachment 586980 [details] [diff] [review]
Bug 705809 - Talos should not depend on scripts run from the talks directory

+                if 'url' in line and '-tp' in line and 'url_mod' not in line:
+                    here = os.path.dirname(os.path.abspath(__file__))
+                    rePath = re.compile('\S*/\S*\s')
+                    reSearch = re.search(rePath, line)
+                    reRes = here + '/' + reSearch.group(0)
+                    line = re.sub(rePath, reRes, line)

Not sure why this code is added.  Can you explain, please?

+            self.outputName = utils.abs_path(self.currentDate + "_config.yml")
This probably should output to the CWD and not to the talos directory

+        defaults["logFile"] = utils.abs_path("browser_output.txt")
Again, probably CWD is better for this

+        bcontroller = str(utils.abs_path("bcontroller.py"))
Why the cast to string?  Seems unnecessary

-                    lfile = os.path.join(os.getcwd(), 'browser_output.txt')
+                    lfile = utils.abs_path('browser_output.txt')
Again, I would think CWD is a better place for this.  The next line might also need to be changed:

                 elif (self.logFile != 'browser_output.txt'):

-        files = ['page_load_test/quit.js',
-                 'scripts/MozillaFileLogger.js',
-                 'startup_test/twinopen/winopen.xul',
-                 'startup_test/twinopen/winopen.js',
-                 'startup_test/twinopen/child-window.html']
+        page_load_test_quit = utils.abs_path('page_load_test/quit.js')
+        file_logger =  utils.abs_path('scripts/MozillaFileLogger.js')
+        twinopen_xul = utils.abs_path('startup_test/twinopen/winopen.xul')
+        twinopen_js = utils.abs_path('startup_test/twinopen/winopen.js')
+        twinopen_child_window = utils.abs_path('startup_test/twinopen/child-window.html')
+        
+        files = [page_load_test_quit,
+                 file_logger,
+                 twinopen_xul,
+                 twinopen_js,
+                 twinopen_child_window]
Its probably better to do this as a list comprehension:
files = [utils.abspath(i) for i in files]

+        time.sleep(browser_config['browser_wait'])
Why?

+def abs_path(path_):
+    """
+    Takes a path, returns that path joined to the absolute path of run_tests.py,
+    so that talos is not dependent on running from a particular cwd
+    also normalizes path to work cross platform 
+    """
+    here = os.path.dirname(os.path.abspath(__file__))
+    path_ = os.path.normpath(path_)
+    path_ = os.path.join(here, path_)
+    return path_

`here` should be calculated once at the module level as it will not change.  Also, with many of the changes, if an absolute path is given (say, path to a configuration) you will want to return without futzing with the path, or you can check this in every place you have a configurable path.

A really good start!  Fix these things and I'll gladly re-review

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Thanks for the feedback! I'm working on a new patch that will be ready to submit shortly. Before I do, let me explain about the regex, so I can improve that for the new patch.

For tests like tsvg and tp, the url field in sample.config is something like this: 

"-tp page_load_test/svg/svg.manifest -tpchrome -tpnoisy -tpformat tinderbox"

The code I added finds the part with "/", and replaces it with that the path to the talos directory followed by the original path:

"-tp <path>/page_load_test/svg/svg.manifest -tpchrome -tpnoisy -tpformat tinderbox"

Without this, the test crashes when run form outside the talos directory (I can provide the output if that would be helpful). 

The pageloader readme provides this as page loader usage: ./firefox -tp file:///path/to/manifest.txt [-tpargs...]

I'm thinking there's something I'm not taking into account here (and likely a better way to do this). Let me know what you all think and I will change accordingly.

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Attachment: Changes per review

I have left out the section that changes the URL field of the config file. According to jmaher, this will not be compatible outside of --develop mode. The web server set up by --develop is already being set up with the absolute path to the talos directory as its root.

Comment 7

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

Comment on attachment 587393 [details] [diff] [review]
Changes per review

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

do we need to adjust our --develop code that uses mozhttpd so it will point to the proper location?

::: talos/run_tests.py
@@ +462,5 @@
>                      'bcontroller_config' : yaml_config.get('bcontroller_config', 'bcontroller.yml'),
>                      'xperf_path'         : yaml_config.get('xperf_path', None),
>                      'develop'            : yaml_config.get('develop', False)}
>  
> +  #normalize paths to work accross platforms 

this is accidentally added in and removed from below.  Make sure to do a 'hg pull -u' before applying this patch so we can get the latest bits.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

The server set up in run_tests.py already uses the directory of runt_tests.py rather than cwd:

http://hg.mozilla.org/build/talos/file/4887e3aebe37/talos/run_tests.py#l513

Comment 9

[Jeff Hammel]

(In reply to Chris Manchester from comment #5)
> Thanks for the feedback! I'm working on a new patch that will be ready to
> submit shortly. Before I do, let me explain about the regex, so I can
> improve that for the new patch.
> 
> For tests like tsvg and tp, the url field in sample.config is something like
> this: 
> 
> "-tp page_load_test/svg/svg.manifest -tpchrome -tpnoisy -tpformat tinderbox"
> 
> The code I added finds the part with "/", and replaces it with that the path
> to the talos directory followed by the original path:
> 
> "-tp <path>/page_load_test/svg/svg.manifest -tpchrome -tpnoisy -tpformat
> tinderbox"
> 
> Without this, the test crashes when run form outside the talos directory (I
> can provide the output if that would be helpful). 
> 
> The pageloader readme provides this as page loader usage: ./firefox -tp
> file:///path/to/manifest.txt [-tpargs...]
> 
> I'm thinking there's something I'm not taking into account here (and likely
> a better way to do this). Let me know what you all think and I will change
> accordingly.

Hmmmm....yeah this is a tricky one.  Let me give it some though

Comment 10

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

also for remote testing we generate a new manifest file and copy it to the remote device in a new location.  This requires us to have the path to the existing manifest.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Attachment: Added: provide path to manifest for remote testing.

Thanks, I've updated the patch accordingly.

Comment 12

[Jeff Hammel]

Currently bitrotted: 

(talos)│curl https://bug705809.bugzilla.mozilla.org/attachment.cgi?id=587741 | patch -p1 --dry-run
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 11517  100 11517    0     0  12736      0 --:--:-- --:--:-- --:--:-- 18726
patching file talos/PerfConfigurator.py
Hunk #1 succeeded at 15 with fuzz 2.
Hunk #2 succeeded at 249 (offset -2 lines).
Hunk #3 succeeded at 316 (offset -2 lines).
patching file talos/ffprocess.py
patching file talos/remotePerfConfigurator.py
patching file talos/run_tests.py
Hunk #1 succeeded at 365 (offset 11 lines).
Hunk #2 FAILED at 466.
1 out of 2 hunks FAILED -- saving rejects to file talos/run_tests.py.rej
patching file talos/ttest.py
patching file talos/utils.py

Comment 13

[Jeff Hammel]

Comment on attachment 587741 [details] [diff] [review]
Added: provide path to manifest for remote testing.

+        browser_config['bcontroller_config'] = utils.abs_path(browser_config['bcontroller_config'])

I think(?) we want bcontroller_config to be based off of CWD as it is a generated file, but I could go either way.  See also from run_tests.py:

+  browser_config['bcontroller_config'] = utils.abs_path(browser_config['bcontroller_config'])

+            localfilename = utils.abs_path("remoteapp.ini")
Again, not sure if we want this based on CWD or the talos directory.  Quite frankly, I find the way remoteapp.ini is used to be a bit magic.  I would prefer a CLI switch to specify it vs looking for remoteapp.ini in some magical location.  It is worth noting that if you have different devices to test, having only one remoteapp.ini is an inconvenience.

-      if (not os.path.isfile('remoteapp.ini')):
-        devicemanager.getFile(remoteAppIni, 'remoteapp.ini')
-      appIni = open('remoteapp.ini')
+      if (not os.path.isfile(utils.abs_path('remoteapp.ini'))):
+        devicemanager.getFile(remoteAppIni, utils.abs_path('remoteapp.ini'))
+      appIni = open(utils.abs_path('remoteapp.ini'))

Whatever decision is made for the above should apply here

+  browser_config['browser_path'] = utils.abs_path(browser_config['browser_path'])
Fairly sure the browser path should not be based on the talos directory

:jmaher, could you weigh in on these?  

I'm not sure if this covers all the use-cases.  We should have a test for this (or more than one) but that can be done as a follow-up.

r+, but I would like jmaher or wlach to weigh in on a few of these things

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Attachment: Updated to apply to most recent revision. Reverted "browser_path"

Thanks for reviewing. This update applies changes to most current revision and reverts "browser_path" (I agree completely this should not be changed to the talos directory).

Comment 15

[Jeff Hammel]

Attachment: attempt at a test

I've attempted to write a unittest for this, at least for the PerfConfigurator part.  However, I don't know what to do about -e for the executable switch which is required by PerfConfigurator.

Comment 16

[Jeff Hammel]

I'm trying to figure out a way that paths should work for .config
files once bug 705809 (Talos should not depend on scripts being run
from the talos directory) and bug 705811 (run_tests.py and
PerfConfigurator.py should become console_scripts) land.  In general
the worklow will be:

1. generate a .yml file:
   PerfConfigurator [options] -o myconfig.yml

2. run tests with this .yml file
   talos -n -d myconfig.yml

Note that myconfig.yml will be generated in the CWD which will not
necessarily be `os.dirname(os.abspath(talos))`.

So what should we do about relative paths in .yml files and .config files?

A few examples:

"""
browser_log: browser_output.txt
"""

In this case, browser_output.txt should go to $PWD/browser_output.txt
(IMHO).  Since it is involved in the specific run, the CWD makes
sense.

Contrast with:

"""
bundles:
  talos_pageloader : page_load_test/pageloader.xpi
"""

In this case you want
`os.path.join(os.dirname(os.abspath(__file__)), 'page_load_test/pageloader.xpi')`
.

So how do we make this work with both cases?

The paths we want based on CWD are pretty easy: they should just work,
since, IIRC, we do not chdir during the execution of Talos.

So what do we do for the cases where you want to refer to the `talos`
python directory?  A few options:

0. We just don't have them.  Not really an option right now, but going
forward we could do this.   There are actually only a few keys that we
care about being this way:
   - bundle paths
   - profile_path (for each test)
   - tpmanifest (for each test)
   - head, and tail.....maybe (be careful here)
If we could make these notions more programmatic, we could actually
eliminate them from configuration entirely (that is, if we had a test
class with these notions wired in).  But this isn't a (practical)
option for now.

1. Have PerfConfigurator fill these in using absolute paths.
Currently we don't interpolate any of these values in
PerfConfigurator.

2. Have run_tests.py interpolate paths for these and these alone.  I
dislike this option since it leaves ambiguity wrt relative to what in
the configuration files.  There is no way of looking at the .yml and
telling which paths are relative to what.  Because of this, I'm less
in favor of this option.

3. Have a macro, e.g. `%(here)s`,
that == `import talos; os.path.dirname(os.path.abspath(talos.__file__))`.
The you have, e.g.:

"""
bundles:
  talos_pageloader : %(here)s/page_load_test/pageloader.xpi
"""

This is probably less work than case 1.

So what do we want to do?

I'm in favor of case 1 or case 3, in that order.

Comment 17

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

option 3 looks the best to me, but I would rather not use 'here' and something more along the lines of 'talosroot'

Comment 18

[Jeff Hammel]

So %(talosdir)s causes an error in yaml parsing:

(talos)│python talos/run_tests.py talos/20120214_0907_config.yml 
Traceback (most recent call last):
  File "talos/run_tests.py", line 655, in <module>
    main()
  File "talos/run_tests.py", line 652, in main
    test_file(arg, options, parser.parsed)
  File "talos/run_tests.py", line 428, in test_file
    yaml_config = yaml.load(config_file)
  File "build/bdist.linux-i686/egg/yaml/__init__.py", line 71, in load
    
  File "build/bdist.linux-i686/egg/yaml/constructor.py", line 37, in get_single_data
  File "build/bdist.linux-i686/egg/yaml/composer.py", line 36, in get_single_node
  File "build/bdist.linux-i686/egg/yaml/composer.py", line 55, in compose_document
  File "build/bdist.linux-i686/egg/yaml/composer.py", line 84, in compose_node
  File "build/bdist.linux-i686/egg/yaml/composer.py", line 133, in compose_mapping_node
  File "build/bdist.linux-i686/egg/yaml/composer.py", line 84, in compose_node
  File "build/bdist.linux-i686/egg/yaml/composer.py", line 133, in compose_mapping_node
  File "build/bdist.linux-i686/egg/yaml/composer.py", line 64, in compose_node
  File "build/bdist.linux-i686/egg/yaml/parser.py", line 98, in check_event
  File "build/bdist.linux-i686/egg/yaml/parser.py", line 449, in parse_block_mapping_value
  File "build/bdist.linux-i686/egg/yaml/scanner.py", line 116, in check_token
  File "build/bdist.linux-i686/egg/yaml/scanner.py", line 257, in fetch_more_tokens
yaml.scanner.ScannerError: while scanning for the next token
found character '%' that cannot start any token
  in "talos/20120214_0907_config.yml", line 73, column 22

Also, we decided to go with 'talos' vs 'talosroot'.

We'll use string.template and ${talos}

Comment 19

[Jeff Hammel]

This particular issue is dealt with in bug 727711

Comment 20

[Jeff Hammel]

Attachment: basic fix

tested locally with ts and tsvg and seems to work.  There may be more pieces that I don't know about but I'm for taking this and posting follow-ups as we find them

Comment 21

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

Comment on attachment 606335 [details] [diff] [review]
basic fix

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

simple.  I feel like we are forgetting something.  What about bcontroller and bcontroller.yml?

Comment 22

[Jeff Hammel]

(In reply to Joel Maher (:jmaher) from comment #21)
> Comment on attachment 606335 [details] [diff] [review]
> basic fix
> 
> Review of attachment 606335 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> simple.  I feel like we are forgetting something.  What about bcontroller
> and bcontroller.yml?

simple because most of the hard work has taken place in other bugs (thanks, chmanchester!)

bcontroller.py is already pegged to the right location: http://hg.mozilla.org/build/talos/file/b666faec6abe/talos/ffprocess.py#l127

bcontroller.yml is controlled by the PerfConfigurator output .yml file (oddly IMHO) which currently by default puts it in the talos directory: http://hg.mozilla.org/build/talos/file/b666faec6abe/talos/sample.config#l24 .  This should be fixed with https://bugzilla.mozilla.org/show_bug.cgi?id=735506 but doesn't really impact this bug IMHO

Comment 23

[Jeff Hammel]

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

This doesn't really test much since the tests there are run from the talos directory, but at least should ensure that it does mess things up.  Still need to test on android

Comment 24

[Jeff Hammel]

er, *doesn't* mess things up....comment 23 is an example of me messing things up ;)

Comment 25

[Jeff Hammel]

see also https://bugzilla.mozilla.org/show_bug.cgi?id=735506

Comment 26

[Mozilla RelEng Bot]

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

Comment 27

[Jeff Hammel]

(In reply to Mozilla RelEng Bot from comment #26)
> Try run for e03ec32a4a93 is complete.
> Detailed breakdown of the results available here:
>     https://tbpl.mozilla.org/?tree=Try&rev=e03ec32a4a93
> Results (out of 72 total builds):
>     success: 68
>     failure: 4
> Builds (or logs if builds failed) available at:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jhammel@mozilla.
> com-e03ec32a4a93

3 android failures which we're not actually testing; an Mac tp crash which seems unrelated, but rebuilding to make sure

Comment 28

[Jeff Hammel]

The tp redux worked fine

Comment 29

[Jeff Hammel]

pushed: http://hg.mozilla.org/build/talos/rev/a1f46a3bfd1c