Bug 55116 – generate a times.json file when running layout tests

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: New Bugs

[Ojan Vafai]

generate a times.json file when running layout tests

[Ojan Vafai]

Created attachment 83613 [details]
Patch

[Kinuko Yasuda]

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

[Tony Chang]

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

[Ojan Vafai]

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

[Dirk Pranke]

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.

[Tony Chang]

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

[Mihai Parparita]

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

[Ojan Vafai]

(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 ***