Bug 60869 - Convert json_results_generator.py to output version 4 JSON.
Summary: Convert json_results_generator.py to output version 4 JSON.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alice Boxhall
URL:
Keywords:
Depends on: 61294 62953 63609
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-15 21:57 PDT by Alice Boxhall
Modified: 2011-07-11 15:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.13 KB, patch)
2011-05-15 21:59 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.94 KB, patch)
2011-05-18 18:36 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.65 KB, patch)
2011-05-19 21:36 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.31 KB, patch)
2011-05-22 23:23 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.33 KB, patch)
2011-05-22 23:28 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.26 KB, patch)
2011-05-22 23:30 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.28 KB, patch)
2011-05-23 21:45 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.45 KB, patch)
2011-05-23 23:29 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.45 KB, patch)
2011-05-24 16:59 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (11.64 KB, patch)
2011-06-19 22:43 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff
Patch (10.19 KB, patch)
2011-07-05 23:19 PDT, Alice Boxhall
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alice Boxhall 2011-05-15 21:57:40 PDT
Convert json_results_generator.py to output version 4 JSON.
Comment 1 Alice Boxhall 2011-05-15 21:59:44 PDT
Created attachment 93603 [details]
Patch
Comment 2 Ojan Vafai 2011-05-18 18:01:44 PDT
Comment on attachment 93603 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:88
> +            if not isinstance(data, dict) or len(data) == 0 or "results" in data:

Do we ever hit cases in practice where data is not a dict or where len(data) == 0?

Also, !len(data) is preferred to checking if it's equal to zero in WebKit code.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:96
> +def convert_flat_path_to_trie(path, value, trie):

nit: Maybe just call this add_path_to_trie? It's not really converting anything to a trie.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:108
> +    directory, _, rest = path.partition("/")

Would be good to name the middle variable even if it's unused. Just to know what's going on.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:314
> +        all_failing_tests_trie = {}
> +        for test in all_failing_tests:
> +            convert_flat_path_to_trie(test, {}, all_failing_tests_trie)
> +
> +        all_failing_tests.update(convert_trie_to_flat_paths(tests))

I don't understand what this is doing. Sorry, I tried to figure it out.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:573
> +        if len(thisTest) == 0:

if !len(thisTest):

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:606
> +                try:
> +                    test_results = results[self.TESTS]
> +                except:
> +                    continue

Why do you need a try/except here?
Comment 3 Alice Boxhall 2011-05-18 18:36:46 PDT
Created attachment 94024 [details]
Patch
Comment 4 Alice Boxhall 2011-05-18 18:40:40 PDT
Comment on attachment 93603 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:88
>> +            if not isinstance(data, dict) or len(data) == 0 or "results" in data:
> 
> Do we ever hit cases in practice where data is not a dict or where len(data) == 0?
> 
> Also, !len(data) is preferred to checking if it's equal to zero in WebKit code.

By the time this code is called, it should always have a results dict for each test, but there are stages during building where it's an empty dict. I'm not sure where to draw the line with being cautious about what format the data will take.

Changed to not len(data).

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:96
>> +def convert_flat_path_to_trie(path, value, trie):
> 
> nit: Maybe just call this add_path_to_trie? It's not really converting anything to a trie.

Done.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:108
>> +    directory, _, rest = path.partition("/")
> 
> Would be good to name the middle variable even if it's unused. Just to know what's going on.

Done.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:314
>> +        all_failing_tests.update(convert_trie_to_flat_paths(tests))
> 
> I don't understand what this is doing. Sorry, I tried to figure it out.

Hm. That's because it has three completely unnecessary lines. Removed.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:573
>> +        if len(thisTest) == 0:
> 
> if !len(thisTest):

Done.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:606
>> +                    continue
> 
> Why do you need a try/except here?

Cargo cult copied from the old code (in json_layout_results_generator.py). I guess sometimes there is no tests data? A check would be a better way to handle that, though.
Comment 5 Alice Boxhall 2011-05-19 18:22:55 PDT
Bump.
Comment 6 Ojan Vafai 2011-05-19 18:42:37 PDT
Comment on attachment 94024 [details]
Patch

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

Please address the comments and then feel free to commit (or have someone commit it for you).

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:93
> +        """Flattens a trie directory structure into a flat structure.
> +
> +        Args:
> +            trie: trie structure.
> +            prefix: aleady-computed path to prepend to the eventual path, if any.
> +
> +        Returns:
> +            The flattened directory structure.
> +        """
> +        result = {}
> +        for name, data in trie.iteritems():
> +            if prefix:
> +                fullname = prefix + "/" + name
> +            else:
> +                fullname = name
> +
> +            if not isinstance(data, dict) or not len(data) or "results" in data:
> +                result[fullname] = data
> +            else:
> +                result.update(convert_trie_to_flat_paths(data, fullname))
> +
> +        return result

This block is indented too far.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:586
> +        archive_version = None

Don't think you need this line. Every line after either sets archive_version or early returns.

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

Remove the else clause since you're early returning.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:605
> +                    # Make all paths are hierarchical

I don't think this comment adds anything. It just explains what the code does, which is clear if you read the code. :)

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:613
> +        _log.info("converted to current version: \n" + str(results_json))

Yikes. I don't think we want to log the whole results json. That's a lot of spam output for the buildbots. How about just logging the new version number here? Also, usually we'd capitalize the first word (i.e. Converted).

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:159
> +                # go down the trie structure to find the test results if necessary

Ditto. This comment just describes what the code clearly does.
Comment 7 Alice Boxhall 2011-05-19 21:36:57 PDT
Created attachment 94170 [details]
Patch
Comment 8 Ojan Vafai 2011-05-19 22:15:39 PDT
Comment on attachment 94170 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:297
> +            _log.info(builder_name + " not in " + str(results_json))

Sorry, I didn't notice this before, but do we really want to log here? I prefer avoiding spewing too much to the log since this is always run when people run the tests. If you really think this is worthwhile, it should be _log.debug so it's only logged with --verbose.
Comment 9 Alice Boxhall 2011-05-22 23:23:36 PDT
Created attachment 94371 [details]
Patch
Comment 10 Alice Boxhall 2011-05-22 23:28:10 PDT
Created attachment 94372 [details]
Patch
Comment 11 Alice Boxhall 2011-05-22 23:30:38 PDT
Created attachment 94374 [details]
Patch
Comment 12 Alice Boxhall 2011-05-22 23:32:16 PDT
Comment on attachment 94170 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:297
>> +            _log.info(builder_name + " not in " + str(results_json))
> 
> Sorry, I didn't notice this before, but do we really want to log here? I prefer avoiding spewing too much to the log since this is always run when people run the tests. If you really think this is worthwhile, it should be _log.debug so it's only logged with --verbose.

Done (after a few Monday-brained failures).
Comment 13 WebKit Commit Bot 2011-05-23 10:28:45 PDT
Comment on attachment 94374 [details]
Patch

Clearing flags on attachment: 94374

Committed r87078: <http://trac.webkit.org/changeset/87078>
Comment 14 WebKit Commit Bot 2011-05-23 10:28:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Adam Barth 2011-05-23 10:35:07 PDT
Is this patch going to break https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/net/resultsjsonparser.py ?
Comment 16 Adam Barth 2011-05-23 10:35:42 PDT
I would have expected updates to these tests:
https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py
Comment 17 Eric Seidel (no email) 2011-05-23 10:47:57 PDT
Comment on attachment 94374 [details]
Patch

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

Ojan. :(  You've gone soft.  I don't think you read this very carefully.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:80
> +    """Flattens a trie directory structure into a flat structure.
> +
> +    Args:
> +        trie: trie structure.
> +        prefix: aleady-computed path to prepend to the eventual path, if any.
> +
> +    Returns:
> +        The flattened directory structure.
> +    """

These doc strings are mostly useless.  An overly verbose side-effect of Google-style python.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:86
> +        if prefix:
> +            fullname = prefix + "/" + name
> +        else:
> +            fullname = name

I would have written this as a ternary since we use Python 2.5+

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:88
> +        if not isinstance(data, dict) or not len(data) or "results" in data:

Why is this necessary?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:91
> +            result.update(convert_trie_to_flat_paths(data, fullname))

Seems these calls could have been a .update() in both cases, with a:

if data_is_not_what_we_want:
   data = convert_tree_to_flat_paths(data, fullname)

before if necessary.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:104
> +    if not "/" in path:

I assume we always use / and don't need to use os.path.sep?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:134
> +        add_path_to_trie(test, int(1000 * test_result.test_run_time), trie)

I wish our time variables would have their unit type in their name.  ms or second or whatever would make these types of multiplctaion much easier to read.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:562
> +        thisTest = tests

This is not python style.

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:593
> +        if archive_version == 3:

I would have broken this whole block off into a new helper function with a nice name telling what it does. :)

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:177
> +        sub_trie = trie

Huh?  Why not just name the incoming argument as sub_trie?

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:208
> +        # FIXME(aboxhall): re-work tests to be more comprehensible and comprehensive.

We don't generally name FIXME's in WebKit.  Also, all comments in webkit should be sentences, beginning with a capital and ending with period.
Comment 18 Adam Barth 2011-05-23 11:12:38 PDT
I rolled this patch out because there's some about how this patch interacts with the rest of the infrastructure and neither Alice nor Ojan seemed to be around to answer questions.
Comment 19 Ojan Vafai 2011-05-23 12:04:01 PDT
(In reply to comment #16)
> I would have expected updates to these tests:
> https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py

I don't see how the two are related. This only affects incremental_results.json, not full_results.json or unexpected_results.json.
Comment 20 Ojan Vafai 2011-05-23 12:04:48 PDT
(In reply to comment #18)
> I rolled this patch out because there's some about how this patch interacts with the rest of the infrastructure and neither Alice nor Ojan seemed to be around to answer questions.

Other than Eric's style/comment concerns, are there issues you see? Did something go wrong on the bots?
Comment 21 Adam Barth 2011-05-23 12:16:49 PDT
> Other than Eric's style/comment concerns, are there issues you see? Did something go wrong on the bots?

Nope.  I was just scared and couldn't find you on IRC.  If you think this is fine, we can land it again.
Comment 22 Ojan Vafai 2011-05-23 12:24:44 PDT
(In reply to comment #21)
> > Other than Eric's style/comment concerns, are there issues you see? Did something go wrong on the bots?
> 
> Nope.  I was just scared and couldn't find you on IRC.  If you think this is fine, we can land it again.

Cool. Sorry I wasn't around. I agree with most of Eric's comments, so I think it makes sense to wait for a new patch first.
Comment 23 Ojan Vafai 2011-05-23 12:28:51 PDT
(In reply to comment #17)
> (From update of attachment 94374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94374&action=review
> 
> Ojan. :(  You've gone soft.  I don't think you read this very carefully.

Sorry, was a bit rushed. I just focused on correctness.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:86
> > +        if prefix:
> > +            fullname = prefix + "/" + name
> > +        else:
> > +            fullname = name
> 
> I would have written this as a ternary since we use Python 2.5+

Meh. Personally I think python ternaries are less readable than an if/else.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:104
> > +    if not "/" in path:
> 
> I assume we always use / and don't need to use os.path.sep?

Correct. os.path.sep would be wrong.

> > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:134
> > +        add_path_to_trie(test, int(1000 * test_result.test_run_time), trie)
> 
> I wish our time variables would have their unit type in their name.  ms or second or whatever would make these types of multiplctaion much easier to read.

Agreed. Doesn't need to block this patch though, right?

> > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:562
> > +        thisTest = tests
> 
> This is not python style.
>
> > Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:208
> > +        # FIXME(aboxhall): re-work tests to be more comprehensible and comprehensive.
> 
> We don't generally name FIXME's in WebKit.  Also, all comments in webkit should be sentences, beginning with a capital and ending with period.

Whoops. Sorry. I'm so used to both syntaxes my eyes just glossed over these.
Comment 24 Alice Boxhall 2011-05-23 21:45:06 PDT
Created attachment 94560 [details]
Patch
Comment 25 Alice Boxhall 2011-05-23 21:45:58 PDT
Comment on attachment 94374 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:80
>> +    """
> 
> These doc strings are mostly useless.  An overly verbose side-effect of Google-style python.

Happy to change or remove; could you be more specific at all?

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:86
>>> +            fullname = name
>> 
>> I would have written this as a ternary since we use Python 2.5+
> 
> Meh. Personally I think python ternaries are less readable than an if/else.

I'm leaving this as-is unless there are particularly strong feelings about it. I've never used a Python ternary as I've only written Python in earnest at Google; I just looked at the syntax and it seems confusing to me.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:88
>> +        if not isinstance(data, dict) or not len(data) or "results" in data:
> 
> Why is this necessary?

It's the base case for the recursion: a dict containing "results" indicates that we have reached a leaf node. Simply using
if "results" in data:
does the job for all well-formed results; the rest is me being overly cautious.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:91
>> +            result.update(convert_trie_to_flat_paths(data, fullname))
> 
> Seems these calls could have been a .update() in both cases, with a:
> 
> if data_is_not_what_we_want:
>    data = convert_tree_to_flat_paths(data, fullname)
> 
> before if necessary.

I don't follow, sorry.
The base case is, we have reached the leaf node, so add the payload to the dict of full path to test data.
The recursive call is, traverse the part of the tree below this node and merge the results of that traversal in with the existing results.
I'm not sure what I would be passing to the update() call in the base case.

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:562
>>> +        thisTest = tests
>> 
>> This is not python style.
> 
> Whoops. Sorry. I'm so used to both syntaxes my eyes just glossed over these.

I'm not clear on what I need to change here.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:593
>> +        if archive_version == 3:
> 
> I would have broken this whole block off into a new helper function with a nice name telling what it does. :)

I pulled out the part in the loop.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:177
>> +        sub_trie = trie
> 
> Huh?  Why not just name the incoming argument as sub_trie?

FUD on my part about the way python passes arguments. I don't feel comfortable assigning to arguments (sub_trie is modified on line 181).

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:208
>> +        # FIXME(aboxhall): re-work tests to be more comprehensible and comprehensive.
> 
> We don't generally name FIXME's in WebKit.  Also, all comments in webkit should be sentences, beginning with a capital and ending with period.

Done.
Comment 26 Eric Seidel (no email) 2011-05-23 22:03:41 PDT
Comment on attachment 94374 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:80
>>> +    """
>> 
>> These doc strings are mostly useless.  An overly verbose side-effect of Google-style python.
> 
> Happy to change or remove; could you be more specific at all?

Generally we try to write methods which self-describe instead of writing desciptions for methods.  Commetns definitely have their place, but they're more useful for describing "why" rather than "what".  In this case, the docstring basically says the same thing 3 times over 9 lines. :)

You could probably just write:
"""Converts a directory trie into a list of absolute paths, appending "prefix" to the start of each.""" or similar to convey all 9 lines in one.

Or rename the method as:
absolute_paths_from_directory_trie(trie, path_root)
or similar, again trying to make the entire thing self-documetning.

Another way to look at it is that if the code of the method is short and clear enough then you don't need any documentation at all, since its just as easy to read the short method as it would be to read (likely out of date!) comments describing what the method does.

>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:86
>>>> +            fullname = name
>>> 
>>> I would have written this as a ternary since we use Python 2.5+
>> 
>> Meh. Personally I think python ternaries are less readable than an if/else.
> 
> I'm leaving this as-is unless there are particularly strong feelings about it. I've never used a Python ternary as I've only written Python in earnest at Google; I just looked at the syntax and it seems confusing to me.

Actually, i won't even bother with a ternary.  It looks like you just want:

if prefix:
   name = prefix + "/" + name

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:88
>>> +        if not isinstance(data, dict) or not len(data) or "results" in data:
>> 
>> Why is this necessary?
> 
> It's the base case for the recursion: a dict containing "results" indicates that we have reached a leaf node. Simply using
> if "results" in data:
> does the job for all well-formed results; the rest is me being overly cautious.

I prefer clarity/brevity over caution here.  The later is long-winded.  Besides, there are also many a fevered python programmer who might explain to you the evils of "isinstance" (i'm not one of them, but I generally try to avoid isinstance when possible).

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:91
>>> +            result.update(convert_trie_to_flat_paths(data, fullname))
>> 
>> Seems these calls could have been a .update() in both cases, with a:
>> 
>> if data_is_not_what_we_want:
>>    data = convert_tree_to_flat_paths(data, fullname)
>> 
>> before if necessary.
> 
> I don't follow, sorry.
> The base case is, we have reached the leaf node, so add the payload to the dict of full path to test data.
> The recursive call is, traverse the part of the tree below this node and merge the results of that traversal in with the existing results.
> I'm not sure what I would be passing to the update() call in the base case.

I see. I think I didn't understand what you were trying to do.  I would reverse the if, then I think it's more clear.

if "results" in data:
  // recurse
else:
   data[fullname] = data

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:134
>>> +        add_path_to_trie(test, int(1000 * test_result.test_run_time), trie)
>> 
>> I wish our time variables would have their unit type in their name.  ms or second or whatever would make these types of multiplctaion much easier to read.
> 
> Agreed. Doesn't need to block this patch though, right?

Yes, the comment had nothing to do with teh patch, just moaing about poor historical naming here.

>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:562
>>>> +        thisTest = tests
>>> 
>>> This is not python style.
>> 
>> Whoops. Sorry. I'm so used to both syntaxes my eyes just glossed over these.
> 
> I'm not clear on what I need to change here.

foo_bar, not fooBar for python in WebKit.  (We try to follow pep8.)

>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:177
>>> +        sub_trie = trie
>> 
>> Huh?  Why not just name the incoming argument as sub_trie?
> 
> FUD on my part about the way python passes arguments. I don't feel comfortable assigning to arguments (sub_trie is modified on line 181).

My understanding is that python is like javascript and does not deep-copy objects when passed.  Your sub_trie = tree also doesn't deep copy. :) so you're just modifying trie all the same!
Comment 27 Alice Boxhall 2011-05-23 23:29:32 PDT
Created attachment 94571 [details]
Patch
Comment 28 Alice Boxhall 2011-05-23 23:30:26 PDT
Comment on attachment 94374 [details]
Patch

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

>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:80
>>>> +    """
>>> 
>>> These doc strings are mostly useless.  An overly verbose side-effect of Google-style python.
>> 
>> Happy to change or remove; could you be more specific at all?
> 
> Generally we try to write methods which self-describe instead of writing desciptions for methods.  Commetns definitely have their place, but they're more useful for describing "why" rather than "what".  In this case, the docstring basically says the same thing 3 times over 9 lines. :)
> 
> You could probably just write:
> """Converts a directory trie into a list of absolute paths, appending "prefix" to the start of each.""" or similar to convey all 9 lines in one.
> 
> Or rename the method as:
> absolute_paths_from_directory_trie(trie, path_root)
> or similar, again trying to make the entire thing self-documetning.
> 
> Another way to look at it is that if the code of the method is short and clear enough then you don't need any documentation at all, since its just as easy to read the short method as it would be to read (likely out of date!) comments describing what the method does.

Converted to one-line documentation (and below).

>>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:86
>>>>> +            fullname = name
>>>> 
>>>> I would have written this as a ternary since we use Python 2.5+
>>> 
>>> Meh. Personally I think python ternaries are less readable than an if/else.
>> 
>> I'm leaving this as-is unless there are particularly strong feelings about it. I've never used a Python ternary as I've only written Python in earnest at Google; I just looked at the syntax and it seems confusing to me.
> 
> Actually, i won't even bother with a ternary.  It looks like you just want:
> 
> if prefix:
>    name = prefix + "/" + name

Done, despite my discomfort with assigning to arguments!

>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:88
>>>> +        if not isinstance(data, dict) or not len(data) or "results" in data:
>>> 
>>> Why is this necessary?
>> 
>> It's the base case for the recursion: a dict containing "results" indicates that we have reached a leaf node. Simply using
>> if "results" in data:
>> does the job for all well-formed results; the rest is me being overly cautious.
> 
> I prefer clarity/brevity over caution here.  The later is long-winded.  Besides, there are also many a fevered python programmer who might explain to you the evils of "isinstance" (i'm not one of them, but I generally try to avoid isinstance when possible).

Done.

>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:91
>>>> +            result.update(convert_trie_to_flat_paths(data, fullname))
>>> 
>>> Seems these calls could have been a .update() in both cases, with a:
>>> 
>>> if data_is_not_what_we_want:
>>>    data = convert_tree_to_flat_paths(data, fullname)
>>> 
>>> before if necessary.
>> 
>> I don't follow, sorry.
>> The base case is, we have reached the leaf node, so add the payload to the dict of full path to test data.
>> The recursive call is, traverse the part of the tree below this node and merge the results of that traversal in with the existing results.
>> I'm not sure what I would be passing to the update() call in the base case.
> 
> I see. I think I didn't understand what you were trying to do.  I would reverse the if, then I think it's more clear.
> 
> if "results" in data:
>   // recurse
> else:
>    data[fullname] = data

That makes sense; done.

>>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:562
>>>>> +        thisTest = tests
>>>> 
>>>> This is not python style.
>>> 
>>> Whoops. Sorry. I'm so used to both syntaxes my eyes just glossed over these.
>> 
>> I'm not clear on what I need to change here.
> 
> foo_bar, not fooBar for python in WebKit.  (We try to follow pep8.)

Ah, yes. Fixed.

>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:177
>>>> +        sub_trie = trie
>>> 
>>> Huh?  Why not just name the incoming argument as sub_trie?
>> 
>> FUD on my part about the way python passes arguments. I don't feel comfortable assigning to arguments (sub_trie is modified on line 181).
> 
> My understanding is that python is like javascript and does not deep-copy objects when passed.  Your sub_trie = tree also doesn't deep copy. :) so you're just modifying trie all the same!

Right, but the thing I can't cope with is having an argument which refers to a thing which was passed in, then changing what that argument refers to -- without changing the original object. I think it makes the code confusing to read.

In this case, I have a trie (more correctly, a reference to the top node of the trie) that I want to find something in, which I do by iteratively going down the trie, keeping a reference to the sub_trie I'm up to -- so the code reflects that. If I had code that took a sub_trie argument (which was really the top node of a trie, not a sub_trie at all), repeatedly assigned something else to that name and then returned something with the same name as the original argument, I think it would be very hard to work out what that did.
Comment 29 Ojan Vafai 2011-05-24 11:22:17 PDT
Comment on attachment 94374 [details]
Patch

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

>>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:177
>>>>> +        sub_trie = trie
>>>> 
>>>> Huh?  Why not just name the incoming argument as sub_trie?
>>> 
>>> FUD on my part about the way python passes arguments. I don't feel comfortable assigning to arguments (sub_trie is modified on line 181).
>> 
>> My understanding is that python is like javascript and does not deep-copy objects when passed.  Your sub_trie = tree also doesn't deep copy. :) so you're just modifying trie all the same!
> 
> Right, but the thing I can't cope with is having an argument which refers to a thing which was passed in, then changing what that argument refers to -- without changing the original object. I think it makes the code confusing to read.
> 
> In this case, I have a trie (more correctly, a reference to the top node of the trie) that I want to find something in, which I do by iteratively going down the trie, keeping a reference to the sub_trie I'm up to -- so the code reflects that. If I had code that took a sub_trie argument (which was really the top node of a trie, not a sub_trie at all), repeatedly assigned something else to that name and then returned something with the same name as the original argument, I think it would be very hard to work out what that did.

Meh. In my experience of python and javascript, it's common to assign to an argument. If I were writing this, I'd keep the variable named "trie" and just assign to it. It makes the code more terse and is just as readable IMO. I don't think it's worth blocking getting this checked in, but it would be better if you could change it.
Comment 30 Alice Boxhall 2011-05-24 16:59:41 PDT
Created attachment 94717 [details]
Patch
Comment 31 Alice Boxhall 2011-05-24 17:00:17 PDT
Comment on attachment 94374 [details]
Patch

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

>>>>>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator_unittest.py:177
>>>>>> +        sub_trie = trie
>>>>> 
>>>>> Huh?  Why not just name the incoming argument as sub_trie?
>>>> 
>>>> FUD on my part about the way python passes arguments. I don't feel comfortable assigning to arguments (sub_trie is modified on line 181).
>>> 
>>> My understanding is that python is like javascript and does not deep-copy objects when passed.  Your sub_trie = tree also doesn't deep copy. :) so you're just modifying trie all the same!
>> 
>> Right, but the thing I can't cope with is having an argument which refers to a thing which was passed in, then changing what that argument refers to -- without changing the original object. I think it makes the code confusing to read.
>> 
>> In this case, I have a trie (more correctly, a reference to the top node of the trie) that I want to find something in, which I do by iteratively going down the trie, keeping a reference to the sub_trie I'm up to -- so the code reflects that. If I had code that took a sub_trie argument (which was really the top node of a trie, not a sub_trie at all), repeatedly assigned something else to that name and then returned something with the same name as the original argument, I think it would be very hard to work out what that did.
> 
> Meh. In my experience of python and javascript, it's common to assign to an argument. If I were writing this, I'd keep the variable named "trie" and just assign to it. It makes the code more terse and is just as readable IMO. I don't think it's worth blocking getting this checked in, but it would be better if you could change it.

I honestly believe that it would be confusing to scan through this code without having seen it before and see that what appears to be the original argument is returned, given the whole point is to search through the trie structure and return a part of it. To me that looks more like I am modifying a part of the trie and then returning it in order to enable some kind of chaining. I have moved the initial assignment below the expansion of the path; hopefully that will emphasise what the code is actually doing a bit more.
Comment 32 Ojan Vafai 2011-05-25 10:33:34 PDT
Comment on attachment 94717 [details]
Patch

Still don't agree (e.g. we do this in many other places in the webkit python/javascript code), but this is such a minor point it's not worth blocking things on.
Comment 33 WebKit Review Bot 2011-06-18 12:44:28 PDT
The commit-queue encountered the following flaky tests while processing attachment 94717 [details]:

inspector/syntax-highlight-css.html bug 50863 (authors: keishi@webkit.org, pfeldman@chromium.org, and yurys@chromium.org)
The commit-queue is continuing to process your patch.
Comment 34 WebKit Review Bot 2011-06-18 12:45:55 PDT
Comment on attachment 94717 [details]
Patch

Clearing flags on attachment: 94717

Committed r89198: <http://trac.webkit.org/changeset/89198>
Comment 35 WebKit Review Bot 2011-06-18 12:46:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Stephen White 2011-06-19 20:04:46 PDT
Sorry, I had to revert this in http://trac.webkit.org/changeset/89218, since it was giving the WebKit canary Tests bots indigestion, e.g.,

http://build.chromium.org/p/chromium.webkit/builders/Linux%20Tests/builds/11135
Comment 37 Alice Boxhall 2011-06-19 20:06:09 PDT
No worries! I'm looking into the issue now; the problem is pretty obvious from the error message, I just need to figure out how to fix and test it.
Comment 38 Alice Boxhall 2011-06-19 22:43:22 PDT
Created attachment 97749 [details]
Patch
Comment 39 Alice Boxhall 2011-06-19 22:46:02 PDT
As it happens, it turned out the call to _get_path_relative_to_layout_test_root was unnecessary (it was previously necessary to convert the JSON from version 2 to 3), so I removed it. Tested by running new-run-webkit-tests and uploading test results to test-results-test.

I don't know how to test for the Chromium build, though; it would be nice to avoid breaking it again.
Comment 40 Stephen White 2011-06-20 08:05:33 PDT
(In reply to comment #39)
> As it happens, it turned out the call to _get_path_relative_to_layout_test_root was unnecessary (it was previously necessary to convert the JSON from version 2 to 3), so I removed it. Tested by running new-run-webkit-tests and uploading test results to test-results-test.
> 
> I don't know how to test for the Chromium build, though; it would be nice to avoid breaking it again.

If you have chromium commit access, you should be able to use the chromium try servers, even though it's a WebKit change.  In a chromium checkout:

gcl try --sub_rep third_party/WebKit/

(You may have to create a fake chromium CL with "gcl change" to satisfy the try command).  The results should be in the first three columns on this page:  http://build.chromium.org/p/tryserver.chromium/waterfall (and they'll be emailed to you too).

On the other hand, it could be that the try servers don't invoke the upload step you're trying to change, since they don't put their results on the flakiness dashboard.  Ojan would know the answer to that.
Comment 41 Ojan Vafai 2011-06-20 09:03:31 PDT
At a quick glance, only the "layout" try servers generate the JSON.

There's no simple way to test the Chromium bots. You would need to start a local buildbot and have it run the tests. http://dev.chromium.org/developers/testing/chromium-build-infrastructure/getting-the-buildbot-source and the links from that page should help you do it. Unless you're interested in doing other work on the buildbots, or just curious how to run one, I wouldn't bother with all this.

If I were you, I would just cq+ it at a time when you know you'll be around to watch the Chromium canaries to see that they don't turn red. The commit-queue is fast now, so it shouldn't take more than 20 minutes to get the commit in.
Comment 42 WebKit Review Bot 2011-06-27 21:20:01 PDT
Comment on attachment 97749 [details]
Patch

Clearing flags on attachment: 97749

Committed r89888: <http://trac.webkit.org/changeset/89888>
Comment 43 WebKit Review Bot 2011-06-27 21:20:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 44 Alice Boxhall 2011-07-05 23:19:01 PDT
Created attachment 99794 [details]
Patch
Comment 45 Alice Boxhall 2011-07-05 23:24:38 PDT
Comment on attachment 99794 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:78
> +        if len(data) and not "results" in data:

This ended up being the crucial problem: the archived results have {} as test results for passing tests, so this code was silently removing them.
Comment 46 Ojan Vafai 2011-07-05 23:56:37 PDT
Comment on attachment 99794 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:78
>> +        if len(data) and not "results" in data:
> 
> This ended up being the crucial problem: the archived results have {} as test results for passing tests, so this code was silently removing them.

Just for thoroughness sake, can you add a test that captures this case?
Comment 47 Alice Boxhall 2011-07-05 23:59:00 PDT
(In reply to comment #46)
> (From update of attachment 99794 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99794&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/layout_package/json_results_generator.py:78
> >> +        if len(data) and not "results" in data:
> > 
> > This ended up being the crucial problem: the archived results have {} as test results for passing tests, so this code was silently removing them.
> 
> Just for thoroughness sake, can you add a test that captures this case?

In theory, yes... in practice, the unit test still breaks my brain. I'll have a look at it tomorrow, though.
Comment 48 Ojan Vafai 2011-07-07 16:44:39 PDT
Comment on attachment 99794 [details]
Patch

Kinuko is about to fork some of this code over into the chromium tree, so I'd rather get this landed ASAP to avoid wasting everyone's time. I'll r+/cq+ for now. You already have a FIXME for improving the tests for this code. I'm fine putting off this specific test until you're able to do that.
Comment 49 Alice Boxhall 2011-07-07 16:49:16 PDT
(In reply to comment #48)
> (From update of attachment 99794 [details])
> Kinuko is about to fork some of this code over into the chromium tree, so I'd rather get this landed ASAP to avoid wasting everyone's time. I'll r+/cq+ for now. You already have a FIXME for improving the tests for this code. I'm fine putting off this specific test until you're able to do that.

Ok. I finally realised, also, that this actually requires a new test specifically for json_layout_result_generator.py, as the behaviour is subtly different. That should make it slightly easier to create the test; I'll create a separate bug for that once it's done.
Comment 50 Kinuko Yasuda 2011-07-11 02:06:15 PDT
(In reply to comment #49)
> (In reply to comment #48)
> > (From update of attachment 99794 [details] [details])
> > Kinuko is about to fork some of this code over into the chromium tree, so I'd rather get this landed ASAP to avoid wasting everyone's time. I'll r+/cq+ for now. You already have a FIXME for improving the tests for this code. I'm fine putting off this specific test until you're able to do that.
> 
> Ok. I finally realised, also, that this actually requires a new test specifically for json_layout_result_generator.py, as the behaviour is subtly different. That should make it slightly easier to create the test; I'll create a separate bug for that once it's done.

Ok I found this has not been submitted :)
I'm getting ready to upload my side of change in slave buildbot (it had been relying on this code) and have a question: should I fork the JSON generation part from this patch or it is not ready to be forked yet?

The code I'll need to fork is only json_results_generator.py (and _unittest).
Comment 51 Ojan Vafai 2011-07-11 15:39:54 PDT
Reopening the bug so it will actually put the patch in the commit-queue.
Comment 52 WebKit Review Bot 2011-07-11 15:48:27 PDT
Comment on attachment 99794 [details]
Patch

Clearing flags on attachment: 99794

Committed r90789: <http://trac.webkit.org/changeset/90789>
Comment 53 WebKit Review Bot 2011-07-11 15:48:35 PDT
All reviewed patches have been landed.  Closing bug.