Bug 96453

Summary: run-perf-tests output cryptic error when the config file is missing.
Product: WebKit Reporter: Stephanie Lewis <slewis>
Component: Tools / TestsAssignee: 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 Flags
Improve error and help messages for source JSON files
none
Fixed one typo tony: review+

Description Stephanie Lewis 2012-09-11 17:55:20 PDT
The error occurred after all the test finished running and it was:

Failed to merge source JSON file ../../perf-test-config.json: [Errno 2] No such file or directory: '../../perf-test-config.json'

which  most people took to mean the tests were failing to run correctly.
Comment 1 Ryosuke Niwa 2012-09-13 14:29:45 PDT
Created attachment 163966 [details]
Improve error and help messages for source JSON files
Comment 2 Ryosuke Niwa 2012-09-13 14:32:51 PDT
Created attachment 163968 [details]
Fixed one typo
Comment 3 Tony Chang 2012-09-13 14:46:13 PDT
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 4 Ryosuke Niwa 2012-09-13 14:50:04 PDT
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...
Comment 5 Ryosuke Niwa 2012-09-13 14:51:26 PDT
Committed r128511: <http://trac.webkit.org/changeset/128511>
Comment 6 Tony Chang 2012-09-13 15:22:31 PDT
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).