Summary: | run-perf-tests output cryptic error when the config file is missing. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Stephanie Lewis <slewis> | ||||||
Component: | Tools / Tests | Assignee: | Ryosuke Niwa <rniwa> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, dpranke, ojan, rniwa, tony, webkit.review.bot | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 77037 | ||||||||
Attachments: |
|
Description
Stephanie Lewis
2012-09-11 17:55:20 PDT
Created attachment 163966 [details]
Improve error and help messages for source JSON files
Created attachment 163968 [details]
Fixed one typo
Comment on attachment 163968 [details] Fixed one typo View in context: https://bugs.webkit.org/attachment.cgi?id=163968&action=review > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:224 > + slave_config_json = self._host.filesystem.open_text_file_for_reading(slave_config_json_path) Nit: Should we move this line out of the try block now? > Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:226 > + return dict(slave_config.items() + output.items()) Nit: I would probably also move this line out of the try as well and make the Exception clear that it's a JSON error. Comment on attachment 163968 [details] Fixed one typo View in context: https://bugs.webkit.org/attachment.cgi?id=163968&action=review >> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:224 >> + slave_config_json = self._host.filesystem.open_text_file_for_reading(slave_config_json_path) > > Nit: Should we move this line out of the try block now? I think codecs.open can still throw exceptions. e.g. slave_config_json_path can be a file that's not readable by the current user. >> Tools/Scripts/webkitpy/performance_tests/perftestsrunner.py:226 >> + return dict(slave_config.items() + output.items()) > > Nit: I would probably also move this line out of the try as well and make the Exception clear that it's a JSON error. No. One of the errors that could happen is that slave_config is an array, string, etc... Committed r128511: <http://trac.webkit.org/changeset/128511> In general, we should strive to scope try/except blocks as tightly as possible to help identify errors. E.g., if you had a separate try/except for each line, it would be clear what failed (bad file permissions, bad json file (does this even raise?), or bad slave config format). |