Bug 67269

Created attachment 105885 [details] Patch

Ai-san, Thank you for the patch. Let me review!

Comment on attachment 105885 [details] Patch Thank you for the great effort, Ai-san! That'd be useful for testing reftests. I think this is a nice first step for us. The patch looks good, but putting r- for some minor nitpicks. View in context: https://bugs.webkit.org/attachment.cgi?id=105885&action=review > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:27 > +"""This module is a stand alone CSSWG reftest runner""" That'd be great that if you include a sample usage of this script in this module level docstring. e.g. Usage: <path>/test_csswg_reftest.py directory_which_includes_reftests_under_layouttest_directory. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:33 > +from collections import defaultdict According to http://trac.webkit.org/wiki/PythonGuidelines, L33 should be before L30. from collections import defaultdict import os import sys > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:36 > +from webkitpy.layout_tests.port.base import Port Line36 should be before Line35 (alphabetically). > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:43 > + """This function is a structure of reftest file""" This docstring would be "A structure of reftest file", wouldn't it? Because this is a docstring for a class, not a fucntion. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:65 > + if reftests_dir_path in myport.test_dirs(): Could you exit early here if reftests_dir_path is not included in myport.test_dirs() so that we can reduce an indent level of the following block? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:69 > + absorute_path = os.path.join(root, file) That would be 'absolute_path', not 'absorute_path'. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:73 > + file_string = open(absorute_path).read() Could you close an opened file explicitly? e.g. file = open(absolute_path) file_string = file.read() file.close() Othewise, you might want to use 'with' statement. from __future__ import with_statement ... with open(absolute_path) as file: file_string = file.read() > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:84 > + This test compares match/mismatch for rendering results of That should be: """This test... (No new line at the beginning.) ... """ > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:90 > + input = DriverInput(test_file, 100000, None) 100000 milliseconds might be too long. Could you set it to 10000 ms? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:99 > + for test_file in self.test_files: Could you separate this 'def test(self)' function into two or three functions? For testability, it might be better to have a separate "# show results" function. This is my rough idea. 1. Calculating hash images of each test file (L87 - L97). 2. Testing matchness/mismatchess using image_hash (L99 - L106). 3. Print out results (L108 - L111). > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:119 > + print "%s is running: There is no %s file." % (test_file, match_file) This seems to be a warning/error message. So you might want to use logging module here like: _log = logging.getLogger(__name__) ... _log.error("There is no match file: %s, %s", test_file, match_file) > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:127 > + print "%s is running: There is no %s file." % (test_file, mismatch_file) Same to LIne119. logging module might be useful here. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:131 > + port = factory.get(options.platform, options) I think port object should be created only at once. You could create a port object in __init__(self, ...) method and reuse that in all methods.

Created attachment 105948 [details] Patch

Comment on attachment 105885 [details] Patch Thank you for the review. I've addressed your comment.

Comment on attachment 105948 [details] Patch Hi Ai-san. Thank you for addressing comments! Could you address some minor comments again? Sorry for the back and forth. I don't think we need a comprehensive unittest for this type of script because this test runner can be used only internally. So we could land this patch after you addressing minor comments and Hamaji-san reviews and give r+ to a next patch. View in context: https://bugs.webkit.org/attachment.cgi?id=105948&action=review > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:72 > + return Could you remove this line? We don't need 'return' here. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:101 > + self.test_files[test_file].crash = True You might want to replace L100-101 with one line as: self.test_files[test_file].crash = output.crash > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:137 > + if self.test_files[test_file].crash: Could you print a message to notify a crash? like: print "test_file: %s is crashed." % test_file

Created attachment 106077 [details] Patch

Thank you for the review again. I've addressed your comment.

Looks good. Thank you! Hamaji-san, could you take a look at the patch? If that's okay, your r+ is highly welcome! (In reply to comment #8) > Thank you for the review again. > I've addressed your comment.

Comment on attachment 106077 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106077&action=review Sorry for the big latency... I somehow missed this patch. I think this patch needs some works for readability. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:47 > + def __init__(self): We might need documentation for each fields. Especially, we might need to explain the key and values of result_* fields. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:56 > +class TestCSSWGReftest(): We might need a docstring which describes how to use this class. BTW, is this the best name for this class? I'd call this CSSWGReftestRunner or something like this, but I'm not sure. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:62 > + def make_reftest_file_data(self, reftests_dir_path): Cannot we move the code from this function to __init__ ? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:63 > + """Given a reftests directory, makes a dictionary I think the summary in a docstring should be a single line. Maybe something like: """Initializes this object with the given reftests directory. More descriptions about self.test_files... > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:71 > + raise AssertionError("Put %s in the %s" % (reftests_dir_path, myport.layout_tests_dir())) I think raising AssertionError in this way isn't a good idea... But I've found there are some code which manually raise AssertionError. Hmm... > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:76 > + absolute_path = os.path.join(root, file) I think this path isn't an absolute path, right? Maybe path_from_root and path_from_testdir (or test_name?) or something like them? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:90 > + """This test compares match/mismatch for rendering results of Runs tests and prints the results? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:107 > + def _compare_match_mismatch(self): How about check_results? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:111 > + test_file_hash = self.test_files[test_file].image_hash How about actual_hash? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:112 > + if self.test_files[test_file].matches: Do we need this if? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:113 > + self._is_matching(test_file, test_file_hash) How about passing test_files[test_file] instead of test_file. There are many "test_files[test_file]" in is_matching. > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:117 > + def _is_matching(self, test_file, test_file_hash): "is_matching" sounds like a boolean function and this function should not have a side-effect. How about check_matching? > Tools/Scripts/webkitpy/layout_tests/reftests/test_csswg_reftest.py:123 > + _log.error("There is no match file: %s, %s", test_file, match_file) It is unclear which file doesn't exist. How about ("Match file %s not found for %s", match_file, test_file) ?

I'm confused. What is the goal of this tool?

Because there seems to be no significant reason to land this, I marked it WONTFIX.