[webkit-reviews] review granted: [Bug 192030] Layout test should generate performance metrics : [Attachment 356194] Patch

[bugzilla-daemon at webkit.org]

Aakash Jain <aakash_jain at apple.com> has granted Zhifei Fang
<zhifei_fang at apple.com>'s request for review:
Bug 192030: Layout test should generate performance metrics
https://bugs.webkit.org/show_bug.cgi?id=192030

Attachment 356194: Patch

https://bugs.webkit.org/attachment.cgi?id=356194&action=review




--- Comment #15 from Aakash Jain <aakash_jain at apple.com> ---
Comment on attachment 356194
  --> https://bugs.webkit.org/attachment.cgi?id=356194
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356194&action=review

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:411
> +	   perf_metrics_path = self._filesystem.join(self._results_directory,
"layout_test_perf_metrics.json")

It would be a good idea to make the json file name a variable and define it in
the beginning of the file. e.g.: LAYOUT_TEST_PERF_METRIC_FILENAME

That way if we want to use this file name in other portion of code, we can
simply use the variable instead of hard-coding it at multiple places.

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:12
3
> +def test_perf_metrics(run_time, individual_test_timings):

run_time seems confusing, it can be interpreted as a runtime (environment).

other alternatives might be:  total_execution_time, total_time

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:12
7
> +    2. run time, which is how much time consumed by the layout tests script

This naming is confusing. Total time is not really the total time. Run time is
actually the total time.

We should rename it to something like:

1) Test time  / Total test time
2) Total time /  Total run/execution time

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:13
7
> +		   "Time": {"current": [total_run_time]},

curious, why "current"?

>
Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:14
0
> +	   "layout_tests_run_time": {

Ditto.