Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7693018:
Intial checkin of layout test analyzer. (Closed)
|
Created:
9 years, 4 months ago by imasaki1 Modified:
9 years, 3 months ago Reviewers:
dennis_jeffrey CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, Paweł Hajdan Jr., acolwell+watch_chromium.org, annacc+watch_chromium.org, pam+watch_chromium.org, ajwong+watch_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing) Visibility:
Public. |
DescriptionIntial checkin of layout test analyzer. This is continuation of http://codereview.chromium.org/7671049/. The goal of this tool is to provide
updated information about layout test status to help developers. The tool goes to Webkit SVN to get layout test cases and the test expectation file (chromium) and do some analysis and send out periodical emails.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98756
Patch Set 1 #
Total comments: 2
Patch Set 2 : Adding trend graph related classes and some minor bug fixes. #Patch Set 3 : Minor modifications. #
Total comments: 330
Patch Set 4 : Modification based on CR comments. #Patch Set 5 : Minor modification after doublecheck. #
Total comments: 65
Patch Set 6 : Modification based on CR comments. #
Total comments: 15
Patch Set 7 : Modification based on CR comments. #
Total comments: 2
Patch Set 8 : Modified based on CR comments. #Messages
Total messages: 12 (0 generated)
Thanks.
I'm going to defer to Dennis for reviewing this. http://codereview.chromium.org/7693018/diff/1/media/tools/layout_tests/bug.py File media/tools/layout_tests/bug.py (right): http://codereview.chromium.org/7693018/diff/1/media/tools/layout_tests/bug.py... media/tools/layout_tests/bug.py:16: WEBKIT_BUG_URL = 'http://bugs.webkit.org/show_bug.cgi?id=' FWIW there's a redirect from http://webkit.org/b/<NNN> to the above URL.
Fixed and did some manual double check. Thanks. http://codereview.chromium.org/7693018/diff/1/media/tools/layout_tests/bug.py File media/tools/layout_tests/bug.py (right): http://codereview.chromium.org/7693018/diff/1/media/tools/layout_tests/bug.py... media/tools/layout_tests/bug.py:16: WEBKIT_BUG_URL = 'http://bugs.webkit.org/show_bug.cgi?id=' On 2011/08/23 14:49:14, Ami Fischman wrote: > FWIW there's a redirect from http://webkit.org/b/%3CNNN> to the above URL. Done.
I'm only partially through this CL, but wanted to send the comments I have so far. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/bug.py File media/tools/layout_tests/bug.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/bug... media/tools/layout_tests/bug.py:13: For example, you can get the name bug owner. 'name bug' --> 'name of a bug' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/bug... media/tools/layout_tests/bug.py:27: BUGWK12345, BUGCR12345, BUGV8_12345, BUGDPRANKE are possible. It looks like the "raw bug text" and the "modifiers" are two different things (the BUGWK12345... items are bug identifier modifiers that may exist in the raw bug text, right?) I think we should make the description a little more clear here, because it sounds like you're referring only to the raw bug text when you mention the BUGWK12345... items. Also, add an "Args:" section to this docstring where you can describe the raw bug text. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/bug... media/tools/layout_tests/bug.py:50: def ToString(self): Instead of calling this function ToString, how about the special name "__str__"? I think that makes it a little cleaner when trying to access the string representation of an object, because you can use the str() built-in function on the object. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:5: google.load('visualization', '1', {'packages':['annotatedtimeline']}); nit: put a space after the ':' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:20: // insert 1 what do you mean by this comment? Same at line 30 below. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:24: data2.addColumn('number', 'Passing Rate '+ nit: put a space right before the '+' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:25: '(100-(#Non-skipped tests)/'+ nit: put a space right before the '+' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:33: var chart = new google.visualization.AnnotatedTimeLine(document.getElementById('chart_div')); nit: have this statement span 2 lines to prevent the line from being longer than 80 characters. Same at lines 35 and 36 below. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:34: chart.draw(data, {displayAnnotations: true, allowHtml:true}); nit: add a space right after the second ':' in this line. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:36: chart2.draw(data2, {displayAnnotations: true, allowHtml:true, scaleType:'maximized'}); nit: add a space right after the second ':' in this line. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:45: <h3 id="numbers">Numbers</h3> nit: change the double quotes to single quotes here http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttest_analyzer.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:6: """A Module for LayoutTestAnalyzer main functions.""" 'Main functions for the Layout Test Analyzer module.' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:18: from layouttest_analyzer_helpers import AnalyzerResultMap We probably don't need this if you already import all of layouttest_analyzer_helpers in line 17 above. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:30: options which contains all command-line option information. 'options which contains' --> 'an object containing' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:36: help="reciever's email adddress", might want to mention the default in this help string http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:46: help='trend graph location', metavar='FILE', might want to mention this default in this help string http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:50: help='bug annotation file location in CSV format', might want to mention this default in this help string http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:53: return options maybe combine the above 2 lines into this? return option_parser.parse_args()[0] http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:59: t = datetime.now() maybe use a more descriptive variable name than 't'. Something like 'start_time' perhaps? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:66: test_info_map = layouttests.JoinWithTestExpectation(test_expectations) Maybe combine the above 2 lines? test_info_map = layouttests.JoinWithTestExpectation(TestExpectations()) In fact, you might also want to combine it with line 67 below too (as long as you think it's still readable) http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:73: prev_time = '2011-08-19-11' you might want to consider making these date folder names constants at the top of this file. That way, if you ever want to change them later for debugging mode purposes, it's easy to find and change them in a single place. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:75: '2011-08-19-11')) use "prev_time" instead here. Also fix the indentation: this parameter should line up underneath the start of the first parameter in the previous line; if it doesn't fit, maybe you can indent like this instead: prev_analyzer_result_map = AnalyzerResultMap.Load( os.path.join(RESULT_DIR, prev_time)) http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:77: # Read bug annotations and generate a annotation map used for the status nit: 'a' --> 'an' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:108: 'undefined', 'undefined')}) nit: indent this line by 1 more space to make it consistent with the indentation used in the lines above. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:113: main() nit: only indent this line 2 spaces, not 4. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttest_analyzer_helpers.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:6: """A module for helper functions for the layouttest analyzer.""" 'Helper functions for the layout test analyzer.' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:32: no skipped. The information is exactly same as the one parsed by the 'no skipped' --> 'not skipped' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:37: """Initialize the Result based on test_info_map. 'Result' --> 'AnalyzerResultMap' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:40: classify them to 'whole', 'skip' or 'nonskip' based on that information. 'them to' --> 'them as' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:48: test expectation keywords such as "SKIP". You first mention that the map contains keys "desc" and "te_info". But in the next sentence, you say the key of the map is keywords such as "SKIP". This is a little confusing; could you clarify the description here? Also, isn't it lower-case "skip" instead of capital "SKIP"? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:54: if test_info_map is not None: I think we can just use "if test_info_map:" http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:62: break I think we can shorten lines 58-62 above using the built-in function any() and python's list comprehensions, but I leave that as an optional exercise for you if you want to practice it :-) http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:70: """Get difference string out of diff map element What do you mean by a "difference string"? Also, add a period at the end of the line. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:72: This is used for generating email message. 'email message' --> 'email messages' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:75: diff_map_element: the compared map generated by |CompareResultMaps()| Add a period or comma at the end of this line. If period, capitalize the first letter in the next line below. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:76: also, this is for each test group ('whole', 'skip', 'nonskip') Add period at end of this sentence. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:77: What about the "type_str" arg? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:78: Return: 'Returns:' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:79: a string including diff information. what do you mean by "diff information"? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:81: diff = len(diff_map_element[0]) - len(diff_map_element[1]) I thought diff_map_element is a dictionary with keys such as "whole", "skip", etc. Is that correct? If so, then would it be a syntax error to access it as diff_map_element[0]? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:89: str = '<font color="%s">+%d</font>' % (color, diff) was it intentional to have a '+' within the string here, given that "diff" may be either a positive or negative value at this point? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:92: str1 += name + "," use single quotes in the string http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:112: anno_map: a annotation map where keys are bug names and values are 'anno_map' --> 'bug_anno_map' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:113: annotation for the bug. 'annotation' --> 'annotations' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:120: # Passing rate is calculated like the followings. I think this comment is not necessary; we can see the passing_rate calculated on the next line below. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:140: str += '<ul>%s (%s)' % (Bug(bug_txt).ToString(), bug_anno_map[bug_txt]) Do you use a Bug object anywhere else? If so, that's ok. If not, you should consider removing Bug as a class if its only purpose it to have its ToString() function called. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:148: '%s' % (gpu_link, test_name)) nit: indent the above 2 lines some more so they line up underneath the start of the string in line 146 above. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:154: def CompareResultMaps(self, result_map2): How about 'other_result_map' instead of 'result_map2'? Having the '2' in the name suggests there may be another variable called 'result_map1', which is not exactly the case here. Also, this function is meant to compare the result_map of the current object to the other, specified result map, so how about changing the function name to "CompareToOtherResultMap"? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:155: """Compare this result map with the other to see if any difference. 'any difference' --> 'there are any differences' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:161: result_map2: another result map to be compared againt this result. 'this result' --> 'the result map of the current object' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:164: a comp_result_map, which contains 'whole', 'skip' and 'nonskip' as keys. We shouldn't say 'comp_result_map' in the description here, since that just happens to be the name of the variable used by this function. The description here should be understandable even if someone cannot look into the code itself. Is there a way to describe 'comp_result_map' more generally, rather than referring to the variable name here? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:166: tuples, where one is for a list of current tests diff and the other 'tests diff' --> 'test diffs' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:167: one is for a list of previous test diff. 'diff' --> 'diffs' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:173: current one. Should we add a comment somewhere explicitly saying that index 0 of the tuple corresponds to the "current" result, and index 1 of the tuple corresponds to the "previous" result? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:179: lookintoTestExpectaionInfo = True 'lookintoTestExpectaionInfo' --> 'lookIntoTestExpectationInfo' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:193: file_path: the file path to be read the result from. 'the string path to the file from which to read the result' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:207: file_path: the file path to be read the result from. 'the string path to the file in which to store the result' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:209: file_object = open(file_path, "wb") use single quotes in strings http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:214: """Get a lit of bugs for non-skipped layout tessts. 'lit' --> 'list' 'tessts' --> 'tests' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:216: This is used for generating email content. Add a "Returns:" section to this docstring http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:220: for te_info in v['te_info']: only indent lines 220-228 by 2 spaces underneath line 219, not 4. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:244: receiver_email_address: reciever's email address. reciever's --> receiver's http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:249: prev_time = datetime.strptime(prev_time, "%Y-%m-%d-%H") use single quotes in strings http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:259: testname_map[k] = True How about something like this? for type in ['skip', 'nonskip']: for i in range(2): for (k, _) in diff_map[type][i]: testname_map[k] = True http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:264: if len(rev_infos) > 0: if rev_infos: http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:272: link = l % (new_rev, old_rev) Maybe combine the above 2 statements so that we don't need to define the 'l' variable? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:286: def SendEmail(sender_email_address, sender_name, receivers_email_addresses, This seems to only be called at line 281 above. Do we need this as a separate function, or can we put it all in the function above? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:305: html_top = """ only indent all the lines within the 'try' by 2 spaces, not 4. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:324: msg.as_string()) nit: indent the above 2 lines by 1 fewer space each http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:327: print 'Error: unable to send email' only indent line by 2 spaces, not 4 http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:327: print 'Error: unable to send email' Maybe we should also capture the exception message and print that out here too http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:340: a string representing latest time among the time_list. add this: 'or None if |time_list| is empty.' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:347: return latest_date.strftime("%Y-%m-%d-%H") Be careful: if time_list is empty, lastest_date will be None here, leading to a problem at this line. We should probably add a check to prevent against this case. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:363: file_name = FindLatestTime(dirList) Why not remove FindLatestTime and just put that code inside this function, since it's only used here anyway? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:376: a list of the difference between test expectation information. 'difference between test' --> 'differences between the test' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:387: return result_list This function seems to only identify the elements in list1 that are also not in list2. If there are any elements in list2 that are not in list1, I think we'll miss those. Is that intentional? If so, we should make sure to describe this in the docstring, and even possibly change the name of the function to make this clear. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:390: def GetDiffBetweenMaps(map1, map2, lookintoTestExpectaionInfo=False): 'lookintoTestExpectaionInfo' --> 'lookIntoTestExpectationInfo' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:396: lookintoTestExpectaionInfo: a boolean to indicate whether you compare 'a boolean to indicate whether to compare test expectation information in addition to just the test case names' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:428: name2_list.append((name, v2)) There's a big chunk of code in this function that's almost duplicated twice: is there a way you can write this code just once and use it twice? Lines 405-415 and 417-428 are very similar to each other.
Comments on the rest of the files. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:20: // insert 1 On 2011/08/24 17:40:47, dennis_jeffrey wrote: > what do you mean by this comment? Same at line 30 below. Wait a sec, I just realized why you need this in trend_graph.py. You actually parse the HTML file to find this, then replace it with actual data, right? Are there any alternative approaches to doing this that we could use? This seems a little too hacky. If the current file should contain dynamic data, maybe the file itself should be created dynamically and should not be checked in as a permanent file(?) http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py:12: from layouttest_analyzer_helpers import AnalyzerResultMap Do we need this, given that we've already imported the full layouttest_analyzer_helpers above? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py:22: def GenerateTestDataWholeAndSkip(self): Is this function meant to be invoked manually? I don't see it called anywhere. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py:44: def GenerateTestDataNonSkip(self): Is this function meant to be invoked manually? I don't see it called anywhere. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttests.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:6: """A Module for LayoutTests. We can see this from the file name; is there any other more helpful description we could give here? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:25: # When parsing the test HTML file and find test description, 'find' --> 'finding the' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:26: # this script tries to find test description using sentences 'find test' --> 'find the test' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:29: Remove this blank line; the comment above is associated with the statement below http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:31: ['This tests', 'This test', 'Test that', 'Tests that', 'Test ']) I think this list can be shortened. 'This test' implies 'This tests' 'Test' implies 'Tests that' and 'Test that' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:36: ['title', 'p', 'div']) Can fit on the previous line? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:53: """Initialize LayoutTests Add period at end of line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:59: in CSV format. Describe what happens when this parameter is None (the default) http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:64: filter_names = [] for the above 4 lines: filter_names = [] if csv_file_path: filter_names = LayoutTests.GetLayoutTestNamesFromCSV(csv_file_path) http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:68: layouttest_root_path) indent this line by 5 fewer spaces. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:91: example can be fallen into rule 1): (This example falls into rule 1): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:96: </p> Indent lines 92-96 a bit since this is an example (to help separate the example from the text above and below it) http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:107: # (1) Try to find test description contains keywords such as 'contains' --> 'that contains' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:109: # We found this is the most common case. 'This is the most common case'. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:111: # Try to find <p> and </p>. Only indent by 2 spaces, not 4, within this function, to keep it consistent with the indentation used in the rest of this CL. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:127: # (3) Try to find it by using THML tag such as title. 'THML' --> 'HTML' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:145: # In this case, just returns 'UNKNOWN' Remove this line. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:154: parent_location_list: a list of locations of parents directory. This is 'parents directory' --> 'parent directories' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:199: names.append(row[0]) For the above 3 lines: names = [row[0] for row in reader] http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:208: names : a list of test names. The test name also have path information as remove the space before the ':' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:208: names : a list of test names. The test name also have path information as 'test name also' --> 'test names also' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:217: def JoinWithTestExpectation(self, test_expections): 'test_expections' --> 'test_expectations' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:227: not care about values). If you don't care about values, why not use a list instead of a map? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:237: # Only 1 match (if exist) # Only keep the first match when found. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:244: layouttest_root_path=DEFAULT_LAYOUTTEST_LOCATION): indent this line so it lines up underneath the first parameter in the previous line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:247: Using urllib2.urlopen(), this method gets the entire HTML and extract its 'extract' --> 'extracts' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:251: test_location: the location of the layout test add period at end of line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:257: raises: 'raises:' --> 'Raises:' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:264: raise URLError Maybe add an informative message to go along with the raised exception? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttests_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:14: """A class for unittest for layouttests class.""" 'Unit tests for the LayoutTests class.' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:20: test_info_map = layouttests.JoinWithTestExpectation(test_expectations) any verification that should be done in this test? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:28: 'Test description is wrong') nit: indent the above 3 lines by 1 more space each. Also add msg='...' around the message in this line. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:30: self.assertEquals(desc2, 'UNKNOWN', 'Test description is wrong') msg='Test description is wrong' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:33: l = ['hoge1/hoge2/hoge3.html', 'hoge1/x.html', 'hoge1/hoge2/hoge4.html'] recommend a more descriptive variable name than 'l' (also, 'l' looks like the number '1' when used at line 35 below). http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_data/graph.html (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_data/graph.html:1: <html> Is this the same as the other file in this CL media/tools/layout_tests/graph/graph.html? If so, do we need to have the file twice? If we need to keep the current file, please make the same changes to this file as you did in the other one. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_data/more (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_data/more:1: (ilayouttest_analyzer_helpers Is this the same as the other file media/tools/layout_tests/result/2011-08-19-21? If so, do we need the file twice? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_data/more_te_info (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_data/more_te_info:1: (ilayouttest_analyzer_helpers Is this the same as the other file media/tools/layout_tests/result/2011-08-19-11? If so, do we need the file twice? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations_history.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:8: import pysvn If this is not a standard python library but is instead a third-party library, it should come down below, after the standard library imports. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:16: # TODO(imasaki): support multiple test expectations files. 'expectations' --> 'expectation' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:34: """Get difference between time perios for the specified test names. 'perios' --> 'periods' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:43: end: A float time object specifying end of the time period to be is it really a "float time object", or just a floating-point value? http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:45: testname_list : A list of the test name string that you are interested 'A list of strings representing test names of interest.' Also remove the space right before the ':' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:51: |lines| contains the diff of the tests that I am interested in. 'that I am interested in' --> 'of interest' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:53: # Get directory name which is necesary to call PySVN.checout(). 'checout' --> 'checkout' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:65: # I am using 1 day back as |start2| (called start2) from original |start|. 'I am using' --> 'This uses'. Also, you may want to explain why you're using start2 as being 1 day back. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:72: pysvn.opt_revision_kind.date, start)) nit: indent the above 4 lines by 1 more space each http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:73: if len(logs2) > 0: if logs2: http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:74: logs.append(logs2[len(logs2)-2]) only indent this line by 2 spaces, not 4 http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:74: logs.append(logs2[len(logs2)-2]) put spaces around the '-' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:75: for i in xrange(len(logs)-1): put spaces around the '-' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:76: # PySVN.log() returns logs in reverse chronological order. Indent this line and the lines below by only 2 spaces, not 4 http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:79: # Getting information about new revision. '# Get information about the new revision.' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:82: message = logs[i].message I think the above 3 variables probably don't need to be defined; we could just inline the code where needed below. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:97: if len(target_lines) > 0: if target_lines: http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:100: # Needs to convert to normal date string for presentation. I think we probably don't need this comment. If you'd prefer to keep it, I recommend moving it right before the current statement (function call), rather than putting it in the middle of the parameter list of the function call. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations_history_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:9: from datetime import datetime Re-arrange the above 4 lines to put them in order by package name. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:14: """A Unittest class for test_expectation_history module.""" 'Unit tests for the TestExpectationsHistory class.' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:17: """Assert test name in the result_list add period at end of line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:19: This method is to chek the test name is in the result. I think this line is unnecessary; the first line of the docstring above already talks about this. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:24: |lines| are the entries in the test expectaton 'expectaton' --> 'expectation'. Also add a period at the end of the line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:28: True if the result contains the testname. add this: ', and False otherwise.' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:30: for (old_rev, new_rev, author, date, message, lines) in result_list: We only actually need to use variable "lines", so we can probably name all the other variables as "_" since they're not used. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:34: return False I think we can probably reduce this function down to one or two lines through creative use of list comprehensions, but will leave that up to you as an optional exercise if you'd like to try it. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:43: [testname]) Can fit on the previous line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:50: ptime = datetime.strptime(ptime, "%Y-%m-%d-%H") Combine the above 2 lines into 1 line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:53: ctime = datetime.strptime(ctime, "%Y-%m-%d-%H") Combine the above 2 lines into 1 line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:57: [testname]) can fit on the previous line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... File media/tools/layout_tests/trend_graph.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:15: # The following is necesasry to decide the point to insert add period at end of line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:39: other is for passing rates) add period at end of sentence http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:42: date_string: a timedate string (e.g., '2008,1,1,13,45,00)' 'date_string' --> 'datetime_string' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:42: date_string: a timedate string (e.g., '2008,1,1,13,45,00)' 'timedate' --> 'datetime' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:47: has the following tuples (numbers, title, text) Remove this line; already stated on line 44 above. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:53: joined_str) indent this line by 1 more space http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:66: new_line_for_passingrate) indent this line by 1 more space http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:69: """Replace line which has |search_exp|, with |replace_line| remove the comma, and add a period at the end of the line http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:72: search_exp: search expression to find a line to be replaced. 'to find a line' --> 'to find in a line' http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:80: if search_exp in line: Indent this line and the lines below by only 2 spaces, not 4 http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:83: sys.stdout.write(line) Should it really be written to stdout? Also, we're missing a return in this function. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... File media/tools/layout_tests/trend_graph_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph_unittest.py:8: import os Reverse the order of the above 3 lines to put them in alphabetical order http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph_unittest.py:37: shutil.copyfile(test_graph_file_backup_path, test_graph_file_path) It's hacky to save the original graph file and then revert back to the original file again here. It's also problematic: if the test fails at line 35 above, we won't get to the current line, and so we'll never revert back to the original graph file. I think that not checking in the graph file may be a better solution here: we only dynamically generate the file when we need to. There's no original graph file checked in that we need to maintain.
Thank for following up all the issues. I hope most of the issues are resolved. Thanks. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/bug.py File media/tools/layout_tests/bug.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/bug... media/tools/layout_tests/bug.py:13: For example, you can get the name bug owner. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'name bug' --> 'name of a bug' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/bug... media/tools/layout_tests/bug.py:27: BUGWK12345, BUGCR12345, BUGV8_12345, BUGDPRANKE are possible. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > It looks like the "raw bug text" and the "modifiers" are two different things > (the BUGWK12345... items are bug identifier modifiers that may exist in the raw > bug text, right?) > > I think we should make the description a little more clear here, because it > sounds like you're referring only to the raw bug text when you mention the > BUGWK12345... items. Also, add an "Args:" section to this docstring where you > can describe the raw bug text. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/bug... media/tools/layout_tests/bug.py:50: def ToString(self): On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Instead of calling this function ToString, how about the special name "__str__"? > I think that makes it a little cleaner when trying to access the string > representation of an object, because you can use the str() built-in function on > the object. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:5: google.load('visualization', '1', {'packages':['annotatedtimeline']}); On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: put a space after the ':' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:20: // insert 1 On 2011/08/24 17:40:47, dennis_jeffrey wrote: > what do you mean by this comment? Same at line 30 below. This is the insert point of the data. The result of the analyzer will be put here and used for graph. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:20: // insert 1 On 2011/08/25 00:36:46, dennis_jeffrey wrote: > On 2011/08/24 17:40:47, dennis_jeffrey wrote: > > what do you mean by this comment? Same at line 30 below. > > Wait a sec, I just realized why you need this in trend_graph.py. You actually > parse the HTML file to find this, then replace it with actual data, right? Are > there any alternative approaches to doing this that we could use? This seems a > little too hacky. > > If the current file should contain dynamic data, maybe the file itself should be > created dynamically and should not be checked in as a permanent file(?) Well, I am not sure about it. Embedding these HTML in Python code is a good idea or not..... But, I am open for any suggestion. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:24: data2.addColumn('number', 'Passing Rate '+ On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: put a space right before the '+' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:25: '(100-(#Non-skipped tests)/'+ On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: put a space right before the '+' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:33: var chart = new google.visualization.AnnotatedTimeLine(document.getElementById('chart_div')); On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: have this statement span 2 lines to prevent the line from being longer than > 80 characters. Same at lines 35 and 36 below. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:34: chart.draw(data, {displayAnnotations: true, allowHtml:true}); On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: add a space right after the second ':' in this line. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:36: chart2.draw(data2, {displayAnnotations: true, allowHtml:true, scaleType:'maximized'}); On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: add a space right after the second ':' in this line. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:45: <h3 id="numbers">Numbers</h3> On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: change the double quotes to single quotes here Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttest_analyzer.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:6: """A Module for LayoutTestAnalyzer main functions.""" On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'Main functions for the Layout Test Analyzer module.' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:18: from layouttest_analyzer_helpers import AnalyzerResultMap On 2011/08/24 17:40:47, dennis_jeffrey wrote: > We probably don't need this if you already import all of > layouttest_analyzer_helpers in line 17 above. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:30: options which contains all command-line option information. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'options which contains' --> > 'an object containing' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:36: help="reciever's email adddress", On 2011/08/24 17:40:47, dennis_jeffrey wrote: > might want to mention the default in this help string Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:46: help='trend graph location', metavar='FILE', On 2011/08/24 17:40:47, dennis_jeffrey wrote: > might want to mention this default in this help string Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:50: help='bug annotation file location in CSV format', On 2011/08/24 17:40:47, dennis_jeffrey wrote: > might want to mention this default in this help string Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:53: return options On 2011/08/24 17:40:47, dennis_jeffrey wrote: > maybe combine the above 2 lines into this? > > return option_parser.parse_args()[0] Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:59: t = datetime.now() On 2011/08/24 17:40:47, dennis_jeffrey wrote: > maybe use a more descriptive variable name than 't'. Something like > 'start_time' perhaps? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:66: test_info_map = layouttests.JoinWithTestExpectation(test_expectations) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Maybe combine the above 2 lines? > > test_info_map = layouttests.JoinWithTestExpectation(TestExpectations()) > > In fact, you might also want to combine it with line 67 below too (as long as > you think it's still readable) Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:73: prev_time = '2011-08-19-11' On 2011/08/24 17:40:47, dennis_jeffrey wrote: > you might want to consider making these date folder names constants at the top > of this file. That way, if you ever want to change them later for debugging > mode purposes, it's easy to find and change them in a single place. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:75: '2011-08-19-11')) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > use "prev_time" instead here. Also fix the indentation: this parameter should > line up underneath the start of the first parameter in the previous line; if it > doesn't fit, maybe you can indent like this instead: > > prev_analyzer_result_map = AnalyzerResultMap.Load( > os.path.join(RESULT_DIR, prev_time)) Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:77: # Read bug annotations and generate a annotation map used for the status On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: 'a' --> 'an' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:108: 'undefined', 'undefined')}) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: indent this line by 1 more space to make it consistent with the indentation > used in the lines above. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer.py:113: main() On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: only indent this line 2 spaces, not 4. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttest_analyzer_helpers.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:6: """A module for helper functions for the layouttest analyzer.""" On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'Helper functions for the layout test analyzer.' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:32: no skipped. The information is exactly same as the one parsed by the On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'no skipped' --> 'not skipped' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:37: """Initialize the Result based on test_info_map. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'Result' --> 'AnalyzerResultMap' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:40: classify them to 'whole', 'skip' or 'nonskip' based on that information. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'them to' --> 'them as' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:48: test expectation keywords such as "SKIP". On 2011/08/24 17:40:47, dennis_jeffrey wrote: > You first mention that the map contains keys "desc" and "te_info". But in the > next sentence, you say the key of the map is keywords such as "SKIP". This is a > little confusing; could you clarify the description here? Also, isn't it > lower-case "skip" instead of capital "SKIP"? Added more comments, here. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:54: if test_info_map is not None: On 2011/08/24 17:40:47, dennis_jeffrey wrote: > I think we can just use "if test_info_map:" Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:62: break On 2011/08/24 17:40:47, dennis_jeffrey wrote: > I think we can shorten lines 58-62 above using the built-in function any() and > python's list comprehensions, but I leave that as an optional exercise for you > if you want to practice it :-) Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:70: """Get difference string out of diff map element On 2011/08/24 17:40:47, dennis_jeffrey wrote: > What do you mean by a "difference string"? Also, add a period at the end of the > line. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:72: This is used for generating email message. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'email message' --> > 'email messages' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:75: diff_map_element: the compared map generated by |CompareResultMaps()| On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Add a period or comma at the end of this line. If period, capitalize the first > letter in the next line below. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:76: also, this is for each test group ('whole', 'skip', 'nonskip') On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Add period at end of this sentence. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:77: On 2011/08/24 17:40:47, dennis_jeffrey wrote: > What about the "type_str" arg? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:78: Return: On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'Returns:' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:79: a string including diff information. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > what do you mean by "diff information"? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:81: diff = len(diff_map_element[0]) - len(diff_map_element[1]) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > I thought diff_map_element is a dictionary with keys such as "whole", "skip", > etc. Is that correct? If so, then would it be a syntax error to access it as > diff_map_element[0]? no. It is a tuple with two lists. I will modify the comments to avoid the confusion. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:89: str = '<font color="%s">+%d</font>' % (color, diff) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > was it intentional to have a '+' within the string here, given that "diff" may > be either a positive or negative value at this point? Good catch I will add only when diff is negative. Modify the code. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:92: str1 += name + "," On 2011/08/24 17:40:47, dennis_jeffrey wrote: > use single quotes in the string Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:112: anno_map: a annotation map where keys are bug names and values are On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'anno_map' --> 'bug_anno_map' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:113: annotation for the bug. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'annotation' --> 'annotations' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:120: # Passing rate is calculated like the followings. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > I think this comment is not necessary; we can see the passing_rate calculated on > the next line below. Removed. Also, this part is replaced by function call since it is used when generating graph as well. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:140: str += '<ul>%s (%s)' % (Bug(bug_txt).ToString(), bug_anno_map[bug_txt]) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Do you use a Bug object anywhere else? If so, that's ok. If not, you should > consider removing Bug as a class if its only purpose it to have its ToString() > function called. I am planning to implement several more functions in Bug class (you can see my TODO in the class). So, I prefer to keep as it is. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:148: '%s' % (gpu_link, test_name)) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: indent the above 2 lines some more so they line up underneath the start of > the string in line 146 above. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:154: def CompareResultMaps(self, result_map2): On 2011/08/24 17:40:47, dennis_jeffrey wrote: > How about 'other_result_map' instead of 'result_map2'? Having the '2' in the > name suggests there may be another variable called 'result_map1', which is not > exactly the case here. > > Also, this function is meant to compare the result_map of the current object to > the other, specified result map, so how about changing the function name to > "CompareToOtherResultMap"? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:155: """Compare this result map with the other to see if any difference. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'any difference' --> 'there are any differences' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:161: result_map2: another result map to be compared againt this result. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'this result' --> 'the result map of the current object' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:164: a comp_result_map, which contains 'whole', 'skip' and 'nonskip' as keys. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > We shouldn't say 'comp_result_map' in the description here, since that just > happens to be the name of the variable used by this function. The description > here should be understandable even if someone cannot look into the code itself. > Is there a way to describe 'comp_result_map' more generally, rather than > referring to the variable name here? Modified comment here. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:166: tuples, where one is for a list of current tests diff and the other On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'tests diff' --> 'test diffs' Deleted. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:167: one is for a list of previous test diff. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'diff' --> 'diffs' Deleted http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:173: current one. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Should we add a comment somewhere explicitly saying that index 0 of the tuple > corresponds to the "current" result, and index 1 of the tuple corresponds to the > "previous" result? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:179: lookintoTestExpectaionInfo = True On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'lookintoTestExpectaionInfo' --> > 'lookIntoTestExpectationInfo' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:193: file_path: the file path to be read the result from. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'the string path to the file from which to read the result' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:207: file_path: the file path to be read the result from. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'the string path to the file in which to store the result' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:209: file_object = open(file_path, "wb") On 2011/08/24 17:40:47, dennis_jeffrey wrote: > use single quotes in strings Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:214: """Get a lit of bugs for non-skipped layout tessts. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'lit' --> 'list' > > 'tessts' --> 'tests' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:216: This is used for generating email content. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Add a "Returns:" section to this docstring Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:220: for te_info in v['te_info']: On 2011/08/24 17:40:47, dennis_jeffrey wrote: > only indent lines 220-228 by 2 spaces underneath line 219, not 4. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:244: receiver_email_address: reciever's email address. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > reciever's --> > receiver's Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:249: prev_time = datetime.strptime(prev_time, "%Y-%m-%d-%H") On 2011/08/24 17:40:47, dennis_jeffrey wrote: > use single quotes in strings Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:259: testname_map[k] = True On 2011/08/24 17:40:47, dennis_jeffrey wrote: > How about something like this? > > for type in ['skip', 'nonskip']: > for i in range(2): > for (k, _) in diff_map[type][i]: > testname_map[k] = True Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:264: if len(rev_infos) > 0: On 2011/08/24 17:40:47, dennis_jeffrey wrote: > if rev_infos: Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:272: link = l % (new_rev, old_rev) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Maybe combine the above 2 statements so that we don't need to define the 'l' > variable? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:286: def SendEmail(sender_email_address, sender_name, receivers_email_addresses, On 2011/08/24 17:40:47, dennis_jeffrey wrote: > This seems to only be called at line 281 above. Do we need this as a separate > function, or can we put it all in the function above? I prefer to separate pure email function and status email function. So, later on, we can use just SendEmail function only (if we want to). http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:305: html_top = """ On 2011/08/24 17:40:47, dennis_jeffrey wrote: > only indent all the lines within the 'try' by 2 spaces, not 4. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:324: msg.as_string()) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > nit: indent the above 2 lines by 1 fewer space each Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:327: print 'Error: unable to send email' On 2011/08/24 17:40:47, dennis_jeffrey wrote: > only indent line by 2 spaces, not 4 Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:327: print 'Error: unable to send email' On 2011/08/24 17:40:47, dennis_jeffrey wrote: > only indent line by 2 spaces, not 4 Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:340: a string representing latest time among the time_list. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > add this: 'or None if |time_list| is empty.' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:347: return latest_date.strftime("%Y-%m-%d-%H") On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Be careful: if time_list is empty, lastest_date will be None here, leading to a > problem at this line. We should probably add a check to prevent against this > case. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:363: file_name = FindLatestTime(dirList) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > Why not remove FindLatestTime and just put that code inside this function, since > it's only used here anyway? I used this in the unit test. This needs to be seperate function to be unit tested. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:376: a list of the difference between test expectation information. On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'difference between test' --> > 'differences between the test' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:387: return result_list On 2011/08/24 17:40:47, dennis_jeffrey wrote: > This function seems to only identify the elements in list1 that are also not in > list2. If there are any elements in list2 that are not in list1, I think we'll > miss those. Is that intentional? If so, we should make sure to describe this > in the docstring, and even possibly change the name of the function to make this > clear. Deleted. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:390: def GetDiffBetweenMaps(map1, map2, lookintoTestExpectaionInfo=False): On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'lookintoTestExpectaionInfo' --> > 'lookIntoTestExpectationInfo' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:396: lookintoTestExpectaionInfo: a boolean to indicate whether you compare On 2011/08/24 17:40:47, dennis_jeffrey wrote: > 'a boolean to indicate whether to compare test expectation information in > addition to just the test case names' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:428: name2_list.append((name, v2)) On 2011/08/24 17:40:47, dennis_jeffrey wrote: > There's a big chunk of code in this function that's almost duplicated twice: is > there a way you can write this code just once and use it twice? Lines 405-415 > and 417-428 are very similar to each other. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py:12: from layouttest_analyzer_helpers import AnalyzerResultMap On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Do we need this, given that we've already imported the full > layouttest_analyzer_helpers above? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py:22: def GenerateTestDataWholeAndSkip(self): On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Is this function meant to be invoked manually? I don't see it called anywhere. Yes. Only when you want to generate the test data. I could put this into set up if you want me to do it. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py:44: def GenerateTestDataNonSkip(self): On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Is this function meant to be invoked manually? I don't see it called anywhere. Same comments as above. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttests.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:6: """A Module for LayoutTests. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > We can see this from the file name; is there any other more helpful description > we could give here? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:25: # When parsing the test HTML file and find test description, On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'find' --> 'finding the' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:26: # this script tries to find test description using sentences On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'find test' --> 'find the test' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:29: On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Remove this blank line; the comment above is associated with the statement below Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:31: ['This tests', 'This test', 'Test that', 'Tests that', 'Test ']) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > I think this list can be shortened. > > 'This test' implies 'This tests' > 'Test' implies 'Tests that' and 'Test that' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:36: ['title', 'p', 'div']) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Can fit on the previous line? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:53: """Initialize LayoutTests On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Add period at end of line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:59: in CSV format. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Describe what happens when this parameter is None (the default) Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:64: filter_names = [] On 2011/08/25 00:36:46, dennis_jeffrey wrote: > for the above 4 lines: > > filter_names = [] > if csv_file_path: > filter_names = LayoutTests.GetLayoutTestNamesFromCSV(csv_file_path) Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:68: layouttest_root_path) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > indent this line by 5 fewer spaces. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:91: example can be fallen into rule 1): On 2011/08/25 00:36:46, dennis_jeffrey wrote: > (This example falls into rule 1): Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:96: </p> On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Indent lines 92-96 a bit since this is an example (to help separate the example > from the text above and below it) Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:107: # (1) Try to find test description contains keywords such as On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'contains' --> 'that contains' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:109: # We found this is the most common case. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'This is the most common case'. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:111: # Try to find <p> and </p>. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Only indent by 2 spaces, not 4, within this function, to keep it consistent with > the indentation used in the rest of this CL. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:127: # (3) Try to find it by using THML tag such as title. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'THML' --> 'HTML' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:145: # In this case, just returns 'UNKNOWN' On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Remove this line. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:154: parent_location_list: a list of locations of parents directory. This is On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'parents directory' --> 'parent directories' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:199: names.append(row[0]) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > For the above 3 lines: > > names = [row[0] for row in reader] Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:208: names : a list of test names. The test name also have path information as On 2011/08/25 00:36:46, dennis_jeffrey wrote: > remove the space before the ':' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:208: names : a list of test names. The test name also have path information as On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'test name also' --> > 'test names also' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:217: def JoinWithTestExpectation(self, test_expections): On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'test_expections' --> > 'test_expectations' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:227: not care about values). On 2011/08/25 00:36:46, dennis_jeffrey wrote: > If you don't care about values, why not use a list instead of a map? But, I have to look the value up several times so I think map is the better option. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:237: # Only 1 match (if exist) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > # Only keep the first match when found. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:244: layouttest_root_path=DEFAULT_LAYOUTTEST_LOCATION): On 2011/08/25 00:36:46, dennis_jeffrey wrote: > indent this line so it lines up underneath the first parameter in the previous > line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:247: Using urllib2.urlopen(), this method gets the entire HTML and extract its On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'extract' --> 'extracts' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:251: test_location: the location of the layout test On 2011/08/25 00:36:46, dennis_jeffrey wrote: > add period at end of line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:257: raises: On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'raises:' --> 'Raises:' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests.py:264: raise URLError On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Maybe add an informative message to go along with the raised exception? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttests_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:14: """A class for unittest for layouttests class.""" On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'Unit tests for the LayoutTests class.' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:20: test_info_map = layouttests.JoinWithTestExpectation(test_expectations) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > any verification that should be done in this test? Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:28: 'Test description is wrong') On 2011/08/25 00:36:46, dennis_jeffrey wrote: > nit: indent the above 3 lines by 1 more space each. Also add msg='...' around > the message in this line. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:30: self.assertEquals(desc2, 'UNKNOWN', 'Test description is wrong') On 2011/08/25 00:36:46, dennis_jeffrey wrote: > msg='Test description is wrong' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttests_unittest.py:33: l = ['hoge1/hoge2/hoge3.html', 'hoge1/x.html', 'hoge1/hoge2/hoge4.html'] On 2011/08/25 00:36:46, dennis_jeffrey wrote: > recommend a more descriptive variable name than 'l' (also, 'l' looks like the > number '1' when used at line 35 below). Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_data/graph.html (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_data/graph.html:1: <html> On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Is this the same as the other file in this CL > media/tools/layout_tests/graph/graph.html? > > If so, do we need to have the file twice? > > If we need to keep the current file, please make the same changes to this file > as you did in the other one. I have to keep this file since this is for unit testing. The other is for real analyzer processing. I will make the same change here. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_data/more (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_data/more:1: (ilayouttest_analyzer_helpers On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Is this the same as the other file > media/tools/layout_tests/result/2011-08-19-21? If so, do we need the file > twice? Yes. This is for unit test. the other file is for actual results. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_data/more_te_info (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_data/more_te_info:1: (ilayouttest_analyzer_helpers On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Is this the same as the other file > media/tools/layout_tests/result/2011-08-19-11? If so, do we need the file > twice? Same as other comments. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations_history.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:8: import pysvn On 2011/08/25 00:36:46, dennis_jeffrey wrote: > If this is not a standard python library but is instead a third-party library, > it should come down below, after the standard library imports. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:16: # TODO(imasaki): support multiple test expectations files. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'expectations' --> 'expectation' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:34: """Get difference between time perios for the specified test names. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'perios' --> 'periods' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:43: end: A float time object specifying end of the time period to be On 2011/08/25 00:36:46, dennis_jeffrey wrote: > is it really a "float time object", or just a floating-point value? Actually it is timestamp. change here. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:45: testname_list : A list of the test name string that you are interested On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'A list of strings representing test names of interest.' > > Also remove the space right before the ':' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:51: |lines| contains the diff of the tests that I am interested in. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'that I am interested in' --> 'of interest' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:53: # Get directory name which is necesary to call PySVN.checout(). On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'checout' --> 'checkout' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:65: # I am using 1 day back as |start2| (called start2) from original |start|. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'I am using' --> 'This uses'. > > Also, you may want to explain why you're using start2 as being 1 day back. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:72: pysvn.opt_revision_kind.date, start)) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > nit: indent the above 4 lines by 1 more space each Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:73: if len(logs2) > 0: On 2011/08/25 00:36:46, dennis_jeffrey wrote: > if logs2: Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:74: logs.append(logs2[len(logs2)-2]) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > only indent this line by 2 spaces, not 4 Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:74: logs.append(logs2[len(logs2)-2]) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > put spaces around the '-' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:75: for i in xrange(len(logs)-1): On 2011/08/25 00:36:46, dennis_jeffrey wrote: > put spaces around the '-' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:76: # PySVN.log() returns logs in reverse chronological order. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Indent this line and the lines below by only 2 spaces, not 4 Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:79: # Getting information about new revision. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > '# Get information about the new revision.' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:82: message = logs[i].message On 2011/08/25 00:36:46, dennis_jeffrey wrote: > I think the above 3 variables probably don't need to be defined; we could just > inline the code where needed below. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:97: if len(target_lines) > 0: On 2011/08/25 00:36:46, dennis_jeffrey wrote: > if target_lines: Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history.py:100: # Needs to convert to normal date string for presentation. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > I think we probably don't need this comment. If you'd prefer to keep it, I > recommend moving it right before the current statement (function call), rather > than putting it in the middle of the parameter list of the function call. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... File media/tools/layout_tests/test_expectations_history_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:9: from datetime import datetime On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Re-arrange the above 4 lines to put them in order by package name. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:14: """A Unittest class for test_expectation_history module.""" On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'Unit tests for the TestExpectationsHistory class.' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:17: """Assert test name in the result_list On 2011/08/25 00:36:46, dennis_jeffrey wrote: > add period at end of line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:19: This method is to chek the test name is in the result. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > I think this line is unnecessary; the first line of the docstring above already > talks about this. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:24: |lines| are the entries in the test expectaton On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'expectaton' --> 'expectation'. Also add a period at the end of the line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:28: True if the result contains the testname. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > add this: ', and False otherwise.' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:30: for (old_rev, new_rev, author, date, message, lines) in result_list: On 2011/08/25 00:36:46, dennis_jeffrey wrote: > We only actually need to use variable "lines", so we can probably name all the > other variables as "_" since they're not used. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:34: return False On 2011/08/25 00:36:46, dennis_jeffrey wrote: > I think we can probably reduce this function down to one or two lines through > creative use of list comprehensions, but will leave that up to you as an > optional exercise if you'd like to try it. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:43: [testname]) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Can fit on the previous line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:50: ptime = datetime.strptime(ptime, "%Y-%m-%d-%H") On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Combine the above 2 lines into 1 line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:53: ctime = datetime.strptime(ctime, "%Y-%m-%d-%H") On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Combine the above 2 lines into 1 line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tes... media/tools/layout_tests/test_expectations_history_unittest.py:57: [testname]) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > can fit on the previous line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... File media/tools/layout_tests/trend_graph.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:15: # The following is necesasry to decide the point to insert On 2011/08/25 00:36:46, dennis_jeffrey wrote: > add period at end of line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:39: other is for passing rates) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > add period at end of sentence Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:42: date_string: a timedate string (e.g., '2008,1,1,13,45,00)' On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'date_string' --> 'datetime_string' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:42: date_string: a timedate string (e.g., '2008,1,1,13,45,00)' On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'timedate' --> 'datetime' Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:47: has the following tuples (numbers, title, text) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Remove this line; already stated on line 44 above. Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:53: joined_str) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > indent this line by 1 more space Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:66: new_line_for_passingrate) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > indent this line by 1 more space Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:69: """Replace line which has |search_exp|, with |replace_line| On 2011/08/25 00:36:46, dennis_jeffrey wrote: > remove the comma, and add a period at the end of the line Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:72: search_exp: search expression to find a line to be replaced. On 2011/08/25 00:36:46, dennis_jeffrey wrote: > 'to find a line' --> 'to find in a line' ? I think this is correct... http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:80: if search_exp in line: On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Indent this line and the lines below by only 2 spaces, not 4 Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:83: sys.stdout.write(line) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Should it really be written to stdout? Also, we're missing a return in this > function. Yes. Also removed "Returns" from comments. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... File media/tools/layout_tests/trend_graph_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph_unittest.py:8: import os On 2011/08/25 00:36:46, dennis_jeffrey wrote: > Reverse the order of the above 3 lines to put them in alphabetical order Done. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph_unittest.py:37: shutil.copyfile(test_graph_file_backup_path, test_graph_file_path) On 2011/08/25 00:36:46, dennis_jeffrey wrote: > It's hacky to save the original graph file and then revert back to the original > file again here. It's also problematic: if the test fails at line 35 above, we > won't get to the current line, and so we'll never revert back to the original > graph file. I think that not checking in the graph file may be a better > solution here: we only dynamically generate the file when we need to. There's > no original graph file checked in that we need to maintain. Created back up file and changed the code to copy from back up from at the beginning of the tests.
Thank you for addressing the last big batch of comments! This batch of comments is a lot smaller ;-) http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/gra... media/tools/layout_tests/graph/graph.html:20: // insert 1 On 2011/08/25 23:57:03, imasaki1 wrote: > On 2011/08/25 00:36:46, dennis_jeffrey wrote: > > On 2011/08/24 17:40:47, dennis_jeffrey wrote: > > > what do you mean by this comment? Same at line 30 below. > > > > Wait a sec, I just realized why you need this in trend_graph.py. You actually > > parse the HTML file to find this, then replace it with actual data, right? > Are > > there any alternative approaches to doing this that we could use? This seems > a > > little too hacky. > > > > If the current file should contain dynamic data, maybe the file itself should > be > > created dynamically and should not be checked in as a permanent file(?) > > Well, I am not sure about it. Embedding these HTML in Python code is a good idea > or not..... But, I am open for any suggestion. Maybe we could check this file in as something like a "template" file, which is only meant to be read in by the current layout test analyzer tool. Instead of having "// inert 1" and "// insert 2", we could replace each of those with "%s". Then the analyzer tool reads in the file as a string, and just replaces the two "%s" with the necessary dynamic data. Something like this maybe: (Assume "data1" belongs where "// insert 1" used to be, and "data2" belongs where "// insert 2" used to be (both of those should be replaced by "%s" now in the template file). Also assume that the current file "graph.html" has its name changed to "graph_template.txt") template_file = open('graph_template.txt', 'r') html_text = template_file.read() % (data1, data2) template_file.close() # At this point, "html_text" contains the complete HTML code, and we can use it however we wish. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... File media/tools/layout_tests/layouttest_analyzer_helpers.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/lay... media/tools/layout_tests/layouttest_analyzer_helpers.py:81: diff = len(diff_map_element[0]) - len(diff_map_element[1]) On 2011/08/25 23:57:03, imasaki1 wrote: > On 2011/08/24 17:40:47, dennis_jeffrey wrote: > > I thought diff_map_element is a dictionary with keys such as "whole", "skip", > > etc. Is that correct? If so, then would it be a syntax error to access it as > > diff_map_element[0]? > > no. It is a tuple with two lists. I will modify the comments to avoid the > confusion. Got it. Thanks. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... File media/tools/layout_tests/trend_graph.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph.py:72: search_exp: search expression to find a line to be replaced. On 2011/08/25 23:57:03, imasaki1 wrote: > On 2011/08/25 00:36:46, dennis_jeffrey wrote: > > 'to find a line' --> 'to find in a line' > > ? I think this is correct... Yes, you're right. I was originally misinterpreting how "search_exp" was used. Sorry about that. http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... File media/tools/layout_tests/trend_graph_unittest.py (right): http://codereview.chromium.org/7693018/diff/8001/media/tools/layout_tests/tre... media/tools/layout_tests/trend_graph_unittest.py:37: shutil.copyfile(test_graph_file_backup_path, test_graph_file_path) On 2011/08/25 23:57:03, imasaki1 wrote: > On 2011/08/25 00:36:46, dennis_jeffrey wrote: > > It's hacky to save the original graph file and then revert back to the > original > > file again here. It's also problematic: if the test fails at line 35 above, > we > > won't get to the current line, and so we'll never revert back to the original > > graph file. I think that not checking in the graph file may be a better > > solution here: we only dynamically generate the file when we need to. There's > > no original graph file checked in that we need to maintain. > > Created back up file and changed the code to copy from back up from at the > beginning of the tests. This is better, but the approach of having a backup file that we have to copy over still seems a little awkward. If there's no other clear alternative, maybe it's ok as is. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/bu... File media/tools/layout_tests/bug.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/bu... media/tools/layout_tests/bug.py:28: bug_modifier: a string representing a bug modifer According to 'modifer' --> 'modifier'. Also add a period right after that word (before "According", since that's a new sentence) http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:33: var chart = new google.visualization.AnnotatedTimeLine(div1); Remove this blank line http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:34: chart.draw(data, {displayAnnotations: true, allowHtml: true}); Add a blank line right below this http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:35: var div2 = document.getElementById('chart_div2') remove this blank line http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:36: var chart2 = new google.visualization.AnnotatedTimeLine(div2); remove this blank line. Now, we have the 3 lines related to "chart" grouped together, followed by a blank line, followed by the 3 lines related to "chart2" grouped together. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:41: '%s)') % 'imasaki@chromium.org', (optional) One nice thing about the optparse module is that when defining your help string, if you want to mention the default value, you don't need to hard-code it: you can use "%default" instead, and then the optparse module will fill that in with whatever you specify in the "default" argument. For example, you could change this to: help='Receiver's e-mail address (defaults to %default)' If you decide to make this change, you might want to make a similar change at lines 52 and 57 below. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:47: 'the debugging time (default to False)'), 'default to False' --> 'off by default' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:57: 'CSV format (default to %s)') % ( nit: the phrase 'bug annotation file location in CSV format' makes it sound like the file's location is in CSV format, but what you actually mean is that the file itself is in CSV format. This might be more clear: 'Location of the bug annotation file; file is expected to be in CSV format' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer_helpers.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:44: test_info_map: the result map of |layouttests.JoinWithTestExpectation|, nit: change the comma at the end of this line into a period http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:49: map is test expectation keywords such as "SKIP" and other keywords Ok, I was confused here because I thought you were only talking about 2 different maps. Actually, you're talking about 3 maps. The first map maps test names to maps. The second map maps strings to a list of maps. Each of those maps (within the list) map test expectation keyword to ...(something you don't mention). I think that if you add the words "test expectation information" at the front of this line, it clears things up for me, because it makes it clear that you're talking about a third type of map. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:83: determination. I recommend a slight re-wording here: type_str: a string indicating the test group to which |diff_map_element| belongs; used for color determination. Must be 'whole', 'skip', or 'nonskip'. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:100: diff_sign = '' We could slightly shorten the above 4 lines like this: diff_sign = '' if diff > 0: diff_sign = '+' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:126: return 100 - len(self.result_map['nonskip'].keys()) * 100 / d Do we have to worry about the possibility of 'd' being 0, leading to a divide-by-zero issue here? http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:196: but it is NOT the current result. This is used for comparions We can probably remove the 'This is used for comparions' phrase; it should be left up to the caller to decide how to use the result of this function. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:202: lookIntoTestExpectaionInfo = True Oops, you did capitalize the 'I' but forgot to add the 't': 'lookIntoTestExpectaionInfo' --> 'lookIntoTestExpectationInfo' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:242: a mapping from bug modifer text (e.g., BUGCR1111) to a test name and 'modifer' --> 'modifier' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:297: '%2Fplatform%2Fchromium%2Ftest_expectations.txt') % ( nit: indent the above 3 lines by 3 more spaces each http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:352: print "Authentication failed:", e use single quotes to define this string http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:400: def GetTEInfoListDiff(list1, list2): I'm curious why you removed the docstring here? I think the docstring should still be here, but it should just make sure to state clearly that this function only identifies the elements in |list1| that are also not in |list2| (after all, computing the "difference" between two lists can be done in other ways too). http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:413: def GetDiffBetweenMapsHelper(map1, map2, lookIntoTestExpectaionInfo): 'lookIntoTestExpectaionInfo' --> 'lookIntoTestExpectationInfo' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:413: def GetDiffBetweenMapsHelper(map1, map2, lookIntoTestExpectaionInfo): Since this function is only needed within the GetDiffBetweenMaps() function below, we can probably define this function as an inner function inside of the function below. Then I think we won't need to have a docstring for this function. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:431: def GetDiffBetweenMaps(map1, map2, lookIntoTestExpectaionInfo=False): 'lookIntoTestExpectaionInfo' --> 'lookIntoTestExpectationInfo' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:437: lookIntoTestExpectaionInfo: aa boolean to indicate whether to compare 'aa' --> 'a' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py:11: import layouttest_analyzer_helper I think we need an 's' at the end of this line, else the import would fail. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttests.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttests.py:149: parent_location_list: a list of locations of parent directory. This is nit: 'directory' --> 'directories' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttests_unittest.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttests_unittest.py:22: mediaTestInResult = False Remove this line; this variable is unused. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttests_unittest.py:25: 'media test cases since we only retrieved media ' Rather than asserting we find at least one media test in the result, can't we have a stronger check and make sure the total number of media tests in the result is correct? I am assuming the input data file is known to us, so we should be able to verify precisely the result. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... File media/tools/layout_tests/test_data/graph.html (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... media/tools/layout_tests/test_data/graph.html:2: <head> Ok, if we have to keep both this test file as well as the original file, please make the same changes here as you did for the original file (media/tools/layout_tests/graph/graph.html) in the current patch set. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... File media/tools/layout_tests/test_data/graph.html.bak (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... media/tools/layout_tests/test_data/graph.html.bak:1: <html> I think this is a temporary file not meant to be a part of this CL, right? http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_history.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_history.py:68: # obtained. Will anything bad happen if at least 1 revision does not exist? Suppose we set up a worldwide "software engineer vacation day" such that some day occurs when nobody checks in a revision. If that situation would break this code, we should probably have a check for that to handle this potential situation gracefully. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_history_unittest.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_history_unittest.py:15: class TestTestExpectaionsHistory(unittest.TestCase): 'TestTestExpectaionsHistory' --> 'TestTestExpectationsHistory' http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/tr... File media/tools/layout_tests/trend_graph.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/tr... media/tools/layout_tests/trend_graph.py:42: datetime_string: a dateime string (e.g., '2008,1,1,13,45,00)' 'dateime' --> 'datetime'
Thank you. I put "?" in the comments that I do not understand. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/bu... File media/tools/layout_tests/bug.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/bu... media/tools/layout_tests/bug.py:28: bug_modifier: a string representing a bug modifer According to On 2011/08/26 19:01:26, dennis_jeffrey wrote: > 'modifer' --> 'modifier'. Also add a period right after that word (before > "According", since that's a new sentence) Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:33: var chart = new google.visualization.AnnotatedTimeLine(div1); On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Remove this blank line ? http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:34: chart.draw(data, {displayAnnotations: true, allowHtml: true}); On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Add a blank line right below this Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:35: var div2 = document.getElementById('chart_div2') On 2011/08/26 19:01:26, dennis_jeffrey wrote: > remove this blank line ? http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:36: var chart2 = new google.visualization.AnnotatedTimeLine(div2); On 2011/08/26 19:01:26, dennis_jeffrey wrote: > remove this blank line. Now, we have the 3 lines related to "chart" grouped > together, followed by a blank line, followed by the 3 lines related to "chart2" > grouped together. Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:41: '%s)') % 'imasaki@chromium.org', On 2011/08/26 19:01:26, dennis_jeffrey wrote: > (optional) One nice thing about the optparse module is that when defining your > help string, if you want to mention the default value, you don't need to > hard-code it: you can use "%default" instead, and then the optparse module will > fill that in with whatever you specify in the "default" argument. For example, > you could change this to: > > help='Receiver's e-mail address (defaults to %default)' > > If you decide to make this change, you might want to make a similar change at > lines 52 and 57 below. Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:47: 'the debugging time (default to False)'), On 2011/08/26 19:01:26, dennis_jeffrey wrote: > 'default to False' --> 'off by default' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:57: 'CSV format (default to %s)') % ( On 2011/08/26 19:01:26, dennis_jeffrey wrote: > nit: the phrase 'bug annotation file location in CSV format' makes it sound like > the file's location is in CSV format, but what you actually mean is that the > file itself is in CSV format. This might be more clear: 'Location of the bug > annotation file; file is expected to be in CSV format' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer_helpers.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:44: test_info_map: the result map of |layouttests.JoinWithTestExpectation|, On 2011/08/26 19:01:26, dennis_jeffrey wrote: > nit: change the comma at the end of this line into a period Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:49: map is test expectation keywords such as "SKIP" and other keywords On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Ok, I was confused here because I thought you were only talking about 2 > different maps. Actually, you're talking about 3 maps. The first map maps test > names to maps. The second map maps strings to a list of maps. Each of those > maps (within the list) map test expectation keyword to ...(something you don't > mention). > > I think that if you add the words "test expectation information" at the front of > this line, it clears things up for me, because it makes it clear that you're > talking about a third type of map. I understand it is confusing. I hope this is clear now. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:83: determination. On 2011/08/26 19:01:26, dennis_jeffrey wrote: > I recommend a slight re-wording here: > > type_str: a string indicating the test group to which |diff_map_element| > belongs; used for color determination. Must be 'whole', 'skip', or > 'nonskip'. Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:100: diff_sign = '' On 2011/08/26 19:01:26, dennis_jeffrey wrote: > We could slightly shorten the above 4 lines like this: > > diff_sign = '' > if diff > 0: > diff_sign = '+' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:126: return 100 - len(self.result_map['nonskip'].keys()) * 100 / d On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Do we have to worry about the possibility of 'd' being 0, leading to a > divide-by-zero issue here? Added exception here in that case. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:196: but it is NOT the current result. This is used for comparions On 2011/08/26 19:01:26, dennis_jeffrey wrote: > We can probably remove the 'This is used for comparions' phrase; it should be > left up to the caller to decide how to use the result of this function. Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:202: lookIntoTestExpectaionInfo = True On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Oops, you did capitalize the 'I' but forgot to add the 't': > > 'lookIntoTestExpectaionInfo' --> > 'lookIntoTestExpectationInfo' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:242: a mapping from bug modifer text (e.g., BUGCR1111) to a test name and On 2011/08/26 19:01:26, dennis_jeffrey wrote: > 'modifer' --> 'modifier' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:297: '%2Fplatform%2Fchromium%2Ftest_expectations.txt') % ( On 2011/08/26 19:01:26, dennis_jeffrey wrote: > nit: indent the above 3 lines by 3 more spaces each Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:352: print "Authentication failed:", e On 2011/08/26 19:01:26, dennis_jeffrey wrote: > use single quotes to define this string Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:400: def GetTEInfoListDiff(list1, list2): On 2011/08/26 19:01:26, dennis_jeffrey wrote: > I'm curious why you removed the docstring here? I think the docstring should > still be here, but it should just make sure to state clearly that this function > only identifies the elements in |list1| that are also not in |list2| (after all, > computing the "difference" between two lists can be done in other ways too). Removed this function. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:413: def GetDiffBetweenMapsHelper(map1, map2, lookIntoTestExpectaionInfo): On 2011/08/26 19:01:26, dennis_jeffrey wrote: > 'lookIntoTestExpectaionInfo' --> > 'lookIntoTestExpectationInfo' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:413: def GetDiffBetweenMapsHelper(map1, map2, lookIntoTestExpectaionInfo): On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Since this function is only needed within the GetDiffBetweenMaps() function > below, we can probably define this function as an inner function inside of the > function below. Then I think we won't need to have a docstring for this > function. Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:431: def GetDiffBetweenMaps(map1, map2, lookIntoTestExpectaionInfo=False): On 2011/08/26 19:01:26, dennis_jeffrey wrote: > 'lookIntoTestExpectaionInfo' --> > 'lookIntoTestExpectationInfo' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:437: lookIntoTestExpectaionInfo: aa boolean to indicate whether to compare On 2011/08/26 19:01:26, dennis_jeffrey wrote: > 'aa' --> 'a' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers_unittest.py:11: import layouttest_analyzer_helper On 2011/08/26 19:01:26, dennis_jeffrey wrote: > I think we need an 's' at the end of this line, else the import would fail. Done. Also, I also forget to run this unit test. I have to make changes to make this unittest pass. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttests.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttests.py:149: parent_location_list: a list of locations of parent directory. This is On 2011/08/26 19:01:26, dennis_jeffrey wrote: > nit: 'directory' --> 'directories' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... File media/tools/layout_tests/layouttests_unittest.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttests_unittest.py:22: mediaTestInResult = False On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Remove this line; this variable is unused. Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/la... media/tools/layout_tests/layouttests_unittest.py:25: 'media test cases since we only retrieved media ' On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Rather than asserting we find at least one media test in the result, can't we > have a stronger check and make sure the total number of media tests in the > result is correct? I am assuming the input data file is known to us, so we > should be able to verify precisely the result. I agree but, Currently, test expectation file is dynamically obtained. So, the result changes dynamically. I will put TODO here to reflect your comments. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... File media/tools/layout_tests/test_data/graph.html (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... media/tools/layout_tests/test_data/graph.html:2: <head> On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Ok, if we have to keep both this test file as well as the original file, please > make the same changes here as you did for the original file > (media/tools/layout_tests/graph/graph.html) in the current patch set. Delete this file. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... File media/tools/layout_tests/test_data/graph.html.bak (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... media/tools/layout_tests/test_data/graph.html.bak:1: <html> On 2011/08/26 19:01:26, dennis_jeffrey wrote: > I think this is a temporary file not meant to be a part of this CL, right? No this is backup file. This needs to include this. The other can be deleted. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_history.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_history.py:68: # obtained. On 2011/08/26 19:01:26, dennis_jeffrey wrote: > Will anything bad happen if at least 1 revision does not exist? Suppose we set > up a worldwide "software engineer vacation day" such that some day occurs when > nobody checks in a revision. If that situation would break this code, we should > probably have a check for that to handle this potential situation gracefully. Modified the code to handle this case. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_history_unittest.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_history_unittest.py:15: class TestTestExpectaionsHistory(unittest.TestCase): On 2011/08/26 19:01:26, dennis_jeffrey wrote: > 'TestTestExpectaionsHistory' --> > 'TestTestExpectationsHistory' Done. http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/tr... File media/tools/layout_tests/trend_graph.py (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/tr... media/tools/layout_tests/trend_graph.py:42: datetime_string: a dateime string (e.g., '2008,1,1,13,45,00)' On 2011/08/26 19:01:26, dennis_jeffrey wrote: > 'dateime' --> > 'datetime' Done.
A handful more comments. Thank you! http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/12005/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:33: var chart = new google.visualization.AnnotatedTimeLine(div1); On 2011/08/26 22:28:44, imasaki1 wrote: > On 2011/08/26 19:01:26, dennis_jeffrey wrote: > > Remove this blank line > > ? Sorry, in the previous patch set I thought there was a blank line here - must have been a mistake. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:20: [new Date(2011,08,26,14,48,00),220,undefined,undefined,51,undefined,undefined,11,undefined,undefined,], nit: split this onto 2 lines so no line exceeds 80 characters http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:20: [new Date(2011,08,26,14,48,00),220,undefined,undefined,51,undefined,undefined,11,undefined,undefined,], Did you intend to add this hard-coded data here? http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:21: [new Date(2011,08,26,15,01,59),220,undefined,undefined,51,undefined,undefined,11,undefined,undefined,], nit: split this onto 2 lines so no line exceeds 80 characters http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:32: [new Date(2011,08,26,14,48,00),94,undefined,undefined], Did you intend to add this hard-coded data here? http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer.py (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:40: help=("reciever's email adddress (defaults to " nit: use single quotes instead of double quotes here. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer_helpers.py (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:436: return (GetDiffBetweenMapsHelper(map1, map2, lookIntoTestExpectationInfo), I think there's a code structure problem here. This return is actually outside of any function. The function defined at line 406 above seems to be empty. Could you double-check lines 406-437 and make sure it's correct? http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_history.py (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_history.py:78: gobackdays *= 2 (optional) Maybe have some maximum value for 'gobackdays', and if the value exceeds that maximum threshold, then you can issue an error and terminate the script? That would prevent some kind of "infinite" loop here.
Thank you. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... File media/tools/layout_tests/graph/graph.html (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:20: [new Date(2011,08,26,14,48,00),220,undefined,undefined,51,undefined,undefined,11,undefined,undefined,], On 2011/08/27 00:04:50, dennis_jeffrey wrote: > Did you intend to add this hard-coded data here? This one was added by mistake. I will remmove those. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:20: [new Date(2011,08,26,14,48,00),220,undefined,undefined,51,undefined,undefined,11,undefined,undefined,], On 2011/08/27 00:04:50, dennis_jeffrey wrote: > nit: split this onto 2 lines so no line exceeds 80 characters This one was added by mistake. I will remmove those. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:21: [new Date(2011,08,26,15,01,59),220,undefined,undefined,51,undefined,undefined,11,undefined,undefined,], On 2011/08/27 00:04:50, dennis_jeffrey wrote: > nit: split this onto 2 lines so no line exceeds 80 characters This one was added by mistake. I will remmove those. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/gr... media/tools/layout_tests/graph/graph.html:32: [new Date(2011,08,26,14,48,00),94,undefined,undefined], On 2011/08/27 00:04:50, dennis_jeffrey wrote: > Did you intend to add this hard-coded data here? This one was added by mistake. I will remmove those. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer.py (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:40: help=("reciever's email adddress (defaults to " On 2011/08/27 00:04:50, dennis_jeffrey wrote: > nit: use single quotes instead of double quotes here. Done. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer_helpers.py (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:436: return (GetDiffBetweenMapsHelper(map1, map2, lookIntoTestExpectationInfo), On 2011/08/27 00:04:50, dennis_jeffrey wrote: > I think there's a code structure problem here. This return is actually outside > of any function. The function defined at line 406 above seems to be empty. > Could you double-check lines 406-437 and make sure it's correct? I thought this is return value of GetDiffBetweenMaps. It is called in unit tests function and has been tested on my desktop.. Are there anything I miss here. http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/te... File media/tools/layout_tests/test_expectations_history.py (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/te... media/tools/layout_tests/test_expectations_history.py:78: gobackdays *= 2 On 2011/08/27 00:04:50, dennis_jeffrey wrote: > (optional) Maybe have some maximum value for 'gobackdays', and if the value > exceeds that maximum threshold, then you can issue an error and terminate the > script? That would prevent some kind of "infinite" loop here. Done.
LGTM Just 1 nit to consider before submitting. Thank you very much for addressing all my comments! http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer_helpers.py (right): http://codereview.chromium.org/7693018/diff/26001/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer_helpers.py:436: return (GetDiffBetweenMapsHelper(map1, map2, lookIntoTestExpectationInfo), On 2011/08/29 21:32:49, imasaki1 wrote: > On 2011/08/27 00:04:50, dennis_jeffrey wrote: > > I think there's a code structure problem here. This return is actually > outside > > of any function. The function defined at line 406 above seems to be empty. > > Could you double-check lines 406-437 and make sure it's correct? > > I thought this is return value of GetDiffBetweenMaps. > > It is called in unit tests function and has been tested on my desktop.. Are > there anything I miss here. > Yes, I mis-read the code here. I see now that the function defined at line 422 is an inner function, inside the function defined at line 406. And the return at line 436 is the implementation of the outer function (which just calls the inner function). So it does look fine as is. Sorry about that! http://codereview.chromium.org/7693018/diff/33001/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer.py (right): http://codereview.chromium.org/7693018/diff/33001/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:40: help=('reciever\'s email adddress (defaults to ' nit: 'reciever' --> 'receiver'
Thank you! http://codereview.chromium.org/7693018/diff/33001/media/tools/layout_tests/la... File media/tools/layout_tests/layouttest_analyzer.py (right): http://codereview.chromium.org/7693018/diff/33001/media/tools/layout_tests/la... media/tools/layout_tests/layouttest_analyzer.py:40: help=('reciever\'s email adddress (defaults to ' On 2011/08/29 23:52:26, dennis_jeffrey wrote: > nit: 'reciever' --> 'receiver' Done. |
