Bug 59239 - move times into their own json file
Summary: move times into their own json file
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Ojan Vafai
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-22 14:51 PDT by Ojan Vafai
Modified: 2011-04-27 14:04 PDT (History)
5 users (show)

See Also:


Attachments
Patch (15.97 KB, patch)
2011-04-22 14:55 PDT, Ojan Vafai
aroben: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ojan Vafai 2011-04-22 14:51:21 PDT
move times into their own json file
Comment 1 Ojan Vafai 2011-04-22 14:55:37 PDT
Created attachment 90770 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-04-22 14:59:19 PDT
@abarth: you should be aware that ojan is changing the API for creating LayoutTestResults.  You already need to make the ports aware of what sort of results they're parsing from, but now you'll need to call a separate method too (which is probably for the better).
Comment 3 Eric Seidel (no email) 2011-04-22 15:07:31 PDT
Comment on attachment 90770 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:202
> +            current_map = trie
> +            for i, part in enumerate(parts):
> +                if i == (len(parts) - 1):
> +                    current_map[part] = int(1000 * test_result.test_run_time)
> +                    break

I'm confused by this function.
Comment 4 Ojan Vafai 2011-04-22 15:12:58 PDT
Comment on attachment 90770 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:202
>> +                    break
> 
> I'm confused by this function.

It breaks a filename into chunks by directory and puts the test time as a value in the lowest part. So, it's basically just heirarchical.

foo/bar/baz.html: 1ms
foo/bar/baz1.html: 3ms

becomes
foo: {
    bar: {
        baz.html: 1,
        baz1.html: 3
    }
}

So, for foo/bar/baz.html we walk from foo, to bar, then to baz.html inserting dicts as appropriate. When we hit the last item in the list (baz.html), we know we're at a leaf and we insert the time instead of inserting a new dict.
Comment 5 Dirk Pranke 2011-04-26 16:01:44 PDT
Comment on attachment 90770 [details]
Patch

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

LGTM with the following comments.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:78
> +        self.filename = name

There is a convention I've tried to follow in other parts of the code where "name" refers to the test name (using unix-style forward directory separators) and "path" refers to the full/absolute path using host dir separators. Realizing that this wasn't what the code was doing before, it might be nice if we followed it here. Then again, given that the test name is broken up by path when it's stored, it seems less critical to me.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:208
> +

I think this particular function needs some unit tests. Pesonally, I'd make it a standalone function and not a classmethod as well. I'd be tempted to add a docstring with your explanation, too.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:220
> +        write_json(self._fs, times, file_path)

Maybe you should add the generate_times_ms_file() function now, update the code you can, and then provide a wrapper for compatibility that can be simply deleted later?
Comment 6 Adam Roben (:aroben) 2011-04-26 16:56:45 PDT
Comment on attachment 90770 [details]
Patch

r+ing on Dirk's behalf.
Comment 7 Ojan Vafai 2011-04-27 10:29:03 PDT
Comment on attachment 90770 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:78

> 
> There is a convention I've tried to follow in other parts of the code where "name" refers to the test name (using unix-style forward directory separators) and "path" refers to the full/absolute path using host dir separators. Realizing that this wasn't what the code was doing before, it might be nice if we followed it here. Then again, given that the test name is broken up by path when it's stored, it seems less critical to me.

I was just making the names match the names in test_results.TestResult so that test_timings_trie could take either one. I'm loath to change the other TestResult class due to the number of places I'd have to modify. I'll put in a FIXME in both for now?

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:208
>> +
> 
> I think this particular function needs some unit tests. Pesonally, I'd make it a standalone function and not a classmethod as well. I'd be tempted to add a docstring with your explanation, too.

Yeah. You're very right. Will do.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:220
>> +        write_json(self._fs, times, file_path)
> 
> Maybe you should add the generate_times_ms_file() function now, update the code you can, and then provide a wrapper for compatibility that can be simply deleted later?

Totally.
Comment 8 Dirk Pranke 2011-04-27 12:19:33 PDT
(In reply to comment #7)
> > There is a convention I've tried to follow in other parts of the code where "name" refers to the test name (using unix-style forward directory separators) and "path" refers to the full/absolute path using host dir separators. Realizing that this wasn't what the code was doing before, it might be nice if we followed it here. Then again, given that the test name is broken up by path when it's stored, it seems less critical to me.
> 
> I was just making the names match the names in test_results.TestResult so that test_timings_trie could take either one. I'm loath to change the other TestResult class due to the number of places I'd have to modify. I'll put in a FIXME in both for now?
>

Ah. Matching the names in TestResult is definitely more important. Do as you like.
Comment 9 Ojan Vafai 2011-04-27 14:04:35 PDT
Committed r85087: <http://trac.webkit.org/changeset/85087>