Bug 59239 – move times into their own json file

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

[Ojan Vafai]

move times into their own json file

[Ojan Vafai]

Created attachment 90770 [details]
Patch

[Eric Seidel (no email)]

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

[Eric Seidel (no email)]

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.

[Ojan Vafai]

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.

[Dirk Pranke]

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?

[Adam Roben (:aroben)]

Comment on attachment 90770 [details]
Patch

r+ing on Dirk's behalf.

[Ojan Vafai]

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.

[Dirk Pranke]

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

[Ojan Vafai]

Committed r85087: <http://trac.webkit.org/changeset/85087>