Bug 96453 – run-perf-tests output cryptic error when the config file is missing.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Stephanie Lewis]

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.

[Ryosuke Niwa]

Created attachment 163966 [details]
Improve error and help messages for source JSON files

[Ryosuke Niwa]

Created attachment 163968 [details]
Fixed one typo

[Tony Chang]

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.

[Ryosuke Niwa]

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...

[Ryosuke Niwa]

Committed r128511: <http://trac.webkit.org/changeset/128511>

[Tony Chang]

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).