Bug 60869

Created attachment 93603 [details] Patch

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?

Created attachment 94024 [details] Patch

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.

Bump.

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.

Created attachment 94170 [details] Patch

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.

Created attachment 94371 [details] Patch

Created attachment 94372 [details] Patch

Created attachment 94374 [details] Patch

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 on attachment 94374 [details] Patch Clearing flags on attachment: 94374 Committed r87078: <http://trac.webkit.org/changeset/87078>

All reviewed patches have been landed. Closing bug.

Is this patch going to break https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/net/resultsjsonparser.py ?

I would have expected updates to these tests: https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/net/resultsjsonparser_unittest.py

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.

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.

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

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

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

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

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

Created attachment 94560 [details] Patch

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 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!

Created attachment 94571 [details] Patch

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

Created attachment 94717 [details] Patch

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

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 on attachment 94717 [details] Patch Clearing flags on attachment: 94717 Committed r89198: <http://trac.webkit.org/changeset/89198>

All reviewed patches have been landed. Closing bug.

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

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.

Created attachment 97749 [details] Patch

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.

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

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 on attachment 97749 [details] Patch Clearing flags on attachment: 97749 Committed r89888: <http://trac.webkit.org/changeset/89888>

All reviewed patches have been landed. Closing bug.

Created attachment 99794 [details] Patch

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

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

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

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

Reopening the bug so it will actually put the patch in the commit-queue.

Comment on attachment 99794 [details] Patch Clearing flags on attachment: 99794 Committed r90789: <http://trac.webkit.org/changeset/90789>

All reviewed patches have been landed. Closing bug.