Closed
Bug 705809
Opened 13 years ago
Closed 12 years ago
Talos should not depend on scripts being run from the talos directory
Categories
(Testing :: Talos, defect, P2)
Testing
Talos
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: k0scist, Unassigned)
References
Details
(Whiteboard: [mozbase][good first bug][mentor=jhammel][lang=py][mozharness+talos])
Attachments
(3 files, 3 obsolete files)
10.99 KB,
patch
|
Details | Diff | Splinter Review | |
1.65 KB,
text/plain
|
Details | |
4.90 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
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__))`
Reporter | ||
Updated•13 years ago
|
Whiteboard: [mozbase]
Reporter | ||
Comment 1•13 years ago
|
||
The first step is to figure out what all needs this fix.
Whiteboard: [mozbase] → [mozbase][good first bug][mentor=jhammel]
Reporter | ||
Updated•13 years ago
|
Priority: -- → P2
Whiteboard: [mozbase][good first bug][mentor=jhammel] → [mozbase][good first bug][mentor=jhammel][lang=py][mozharness+talos]
Comment 2•13 years ago
|
||
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
Attachment #586980 -
Flags: review?(jhammel)
Comment 3•13 years ago
|
||
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.
Reporter | ||
Comment 4•13 years ago
|
||
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
Attachment #586980 -
Flags: review?(jhammel) → review-
Comment 5•13 years ago
|
||
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•13 years ago
|
||
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.
Attachment #586980 -
Attachment is obsolete: true
Attachment #587393 -
Flags: review?(jhammel)
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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
Reporter | ||
Comment 9•13 years ago
|
||
(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•13 years ago
|
||
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•13 years ago
|
||
Thanks, I've updated the patch accordingly.
Attachment #587393 -
Attachment is obsolete: true
Attachment #587393 -
Flags: review?(jhammel)
Attachment #587741 -
Flags: review?(jhammel)
Reporter | ||
Comment 12•13 years ago
|
||
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
Reporter | ||
Comment 13•13 years ago
|
||
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
Attachment #587741 -
Flags: review?(jhammel) → review+
Comment 14•13 years ago
|
||
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).
Attachment #587741 -
Attachment is obsolete: true
Reporter | ||
Comment 15•13 years ago
|
||
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.
Reporter | ||
Comment 16•12 years ago
|
||
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•12 years ago
|
||
option 3 looks the best to me, but I would rather not use 'here' and something more along the lines of 'talosroot'
Reporter | ||
Comment 18•12 years ago
|
||
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}
Reporter | ||
Comment 19•12 years ago
|
||
This particular issue is dealt with in bug 727711
Reporter | ||
Comment 20•12 years ago
|
||
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
Attachment #606335 -
Flags: review?(jmaher)
Comment 21•12 years ago
|
||
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?
Attachment #606335 -
Flags: review?(jmaher) → review+
Reporter | ||
Comment 22•12 years ago
|
||
(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
Reporter | ||
Comment 23•12 years ago
|
||
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
Reporter | ||
Comment 24•12 years ago
|
||
er, *doesn't* mess things up....comment 23 is an example of me messing things up ;)
Reporter | ||
Comment 25•12 years ago
|
||
see also https://bugzilla.mozilla.org/show_bug.cgi?id=735506
See Also: → 735506
Comment 26•12 years ago
|
||
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
Reporter | ||
Comment 27•12 years ago
|
||
(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
Reporter | ||
Comment 28•12 years ago
|
||
The tp redux worked fine
Reporter | ||
Comment 29•12 years ago
|
||
pushed: http://hg.mozilla.org/build/talos/rev/a1f46a3bfd1c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•