generate a times.json file when running layout tests
Created attachment 83613 [details] Patch
Comment on attachment 83613 [details] Patch LGTM though I'm not a reviewer. Making it easier to find the bottlenecks in running tests sounds great! View in context: https://bugs.webkit.org/attachment.cgi?id=83613&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:84 > + TIMES_PREFIX = "ADD_TIMES(" nit: RESULTS_JSON_PREFIX, TIMES_JSON_PREFIX might be easier to read? (to make it match with 'JSON_SUFFIX')
Comment on attachment 83613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83613&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:184 > + # Convert floats from seconds into milliseconds. > + float_format = lambda o, allow_nan=True: format(o * 1000, '.0f') Why don't we just do this conversion when adding values to self._test_timings (or in get_times())? This avoids having to add an optional param to _generate_json_file and the hackery of setting encoder.floatstr. Also, the comment and the changelog disagree about the format of the map (is it seconds on ms?).
(In reply to comment #3) > (From update of attachment 83613 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=83613&action=review > > > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:184 > > + # Convert floats from seconds into milliseconds. > > + float_format = lambda o, allow_nan=True: format(o * 1000, '.0f') > > Why don't we just do this conversion when adding values to self._test_timings (or in get_times())? This avoids having to add an optional param to _generate_json_file and the hackery of setting encoder.floatstr. > > Also, the comment and the changelog disagree about the format of the map (is it seconds on ms?). We use the map values to create the incremental_results.json file as well. So, in one case, we want truncated to seconds and in another we want it converted to milliseconds. So, I don't see how we could set it when adding values to self._test_timings. I guess we could do the conversion in get_times, but we'd still need and argument to tell it whether to return seconds or milliseconds.
Comment on attachment 83613 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=83613&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/json_layout_results_generator.py:88 > + here you have "get_times" returning "_test_timings", which appears to be the whole dictionary; further down you have "get_test_timing" which refers to a single lookup. Maybe this should be "get_timings" to be more consistent? Or, if you don't like two functions that are different by only one character, "get_all_test_times" and "_get_test_time", and then make the member variables match the function names? On a more general note, a couple of comments ... 1) Are the file formats documented anywhere other than buried in the source code? 2) I feel like this is kind of an overly specialized file. It may work to generate a treemap, but I'm not sure if it is useful for much else. In particular, it would be nice to be able to see the time a test took next to the expectation for the test and the actual result. It's not clear to me if I can get that info from results.json (due to the arcane format of that file), and I don't know that it would make expectations.json much larger to add in the actual result and the time it took.
(In reply to comment #4) > > We use the map values to create the incremental_results.json file as well. So, in one case, we want truncated to seconds and in another we want it converted to milliseconds. So, I don't see how we could set it when adding values to self._test_timings. I guess we could do the conversion in get_times, but we'd still need and argument to tell it whether to return seconds or milliseconds. Alternately, you could just make a copy of the dict and convert the values before calling _generate_json_file. That seems better than reaching into simplejson and changing the formatter.
(In reply to comment #5) > 2) I feel like this is kind of an overly specialized file. It may work to generate a treemap, but I'm not sure if it is useful for much else. In particular, it would be nice to be able to see the time a test took next to the expectation for the test and the actual result. It's not clear to me if I can get that info from results.json (due to the arcane format of that file), and I don't know that it would make expectations.json much larger to add in the actual result and the time it took. +1 to this (this would effectively fix bug 52267, where I was hoping for a more sane results.json format).
(In reply to comment #7) > (In reply to comment #5) > > 2) I feel like this is kind of an overly specialized file. It may work to generate a treemap, but I'm not sure if it is useful for much else. In particular, it would be nice to be able to see the time a test took next to the expectation for the test and the actual result. It's not clear to me if I can get that info from results.json (due to the arcane format of that file), and I don't know that it would make expectations.json much larger to add in the actual result and the time it took. > > +1 to this (this would effectively fix bug 52267, where I was hoping for a more sane results.json format). I'll upload a patch that does this to bug 52267. I'm a bit torn. On the one hand, it's better to have less code to maintain. On the other, the file size going grows from ~750KB to ~2000KB. I'm a bit worried about this affecting test cycle time since these files are uploaded to the test-results-server on each run and need to be downloaded for the treemaps. We can try it and see. *** This bug has been marked as a duplicate of bug 52267 ***