<rdar://problem/32779561>
Wrong number <rdar://problem/32779516>
Created attachment 355780 [details] Patch
Created attachment 355784 [details] Patch
Comment on attachment 355784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355784&action=review > Tools/ChangeLog:1 > +2018-11-27 Zhifei FANG <zhifei_fang@apple.com> Duplicate changelogs here. Also, should your last name be capitalized like that? Seems like a bug, perhaps a tooling one. > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:273 > + } Maybe collapse these?
Created attachment 355795 [details] Patch
Created attachment 355797 [details] Patch
Discussed this with Zhifei and Ryan. We need to be tracking total test-time here, not individual test time. Tracking individual test time will be a nightmare for the service consuming these results.
Created attachment 355890 [details] Patch
Created attachment 355892 [details] Patch
Comment on attachment 355892 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=355892&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:130 > + total_run_time += int(1000 * test_result.test_run_time) ...This will be an aggregate of of all test times, not the time it took to run the tests. Not sure which metric we want to track, I can see merit in both. As an example, suppose we ran 100 tests in 10 processes with each test taking 30 seconds. This metric would report a total test run time of 50 minutes, when in reality, it actually took 5 minutes for the tests to run.
Created attachment 356101 [details] Patch
Comment on attachment 356101 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356101&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:412 > + initial_results.results_by_name.values()) This seems like it would be fine on 1 line. Side note, the one part of PEP-8 we ignore is the line size limit > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:414 > + self._results_directory, "layout_test_perf_metrics.json") Why a new-line here? > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:125 > + Will need to generate the total runtime of layout tests as a perfromance metric This seems like a description of what needs to be done as opposed to what has been done.
Created attachment 356194 [details] Patch
Unofficial r=me.
Comment on attachment 356194 [details] 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:123 > +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:127 > + 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:137 > + "Time": {"current": [total_run_time]}, curious, why "current"? > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:140 > + "layout_tests_run_time": { Ditto.
Created attachment 356444 [details] Patch
As discussed change to run time of second level categories and total run time.
Ooop. I marked this as r+ accidentally. Sorry.
Created attachment 356513 [details] Patch
hi, could you guys approve this ?
Comment on attachment 356513 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=356513&action=review > Tools/ChangeLog:8 > + Can you add a high-level comment about what's being done here? As a side note, add_test_perf_metric and test_perf_metrics probably deserve some commend as well. > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:122 > +def add_test_perf_metric(path, time, tests, depth, target_depth): Can we append this function with an underscore since it's defiantly a private function?
Created attachment 358550 [details] Patch
Comment on attachment 358550 [details] Patch Attachment 358550 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/10665230 New failing tests: media/video-zoom.html
Created attachment 358564 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 358550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358550&action=review r=me assuming Dean Johnson's in-person feedback has been incorporated. > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:122 > +def _add_test_perf_metric(path, time, tests, depth, target_depth): Ditto. _add_perf_metric_for_test() might read better. > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:159 > +def test_perf_metrics(run_time, individual_test_timings): 'test_perf_metrics' name feels like this is a test for perf_metrics. perf_metrics_for_test might read better.
Comment on attachment 358550 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=358550&action=review Looks good overall, just a few nits on style / naming. > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:122 > +def _add_test_perf_metric(path, time, tests, depth, target_depth): Talked with Jonathan about target_depth as I was a bit confused by it. Could you rename it to depth_limit, which implies it is okay to exit before hitting target_depth? > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:124 > + Breaks a test name into chunks by directory, and keep track the depth. the depth -> of the depth. Both lines may be able to be simplified to: "Aggregate test time to result for a given test at a specified target_depth." > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:144 > + }}} Is there a reason you expanded this block, but not the one below? We should try to be consistent in style here. > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:183 > + # for now, we only sent two level of categories sent -> send level -> levels
Created attachment 358613 [details] Patch
Comment on attachment 358613 [details] Patch Clearing flags on attachment: 358613 Committed r239739: <https://trac.webkit.org/changeset/239739>
All reviewed patches have been landed. Closing bug.