[webkit-reviews] review denied: [Bug 67269] Provides a stand alone CSSWG reftest runner which can be used internally. : [Attachment 105885] Patch

[bugzilla-daemon at webkit.org]

Hayato Ito <hayato at chromium.org> has denied Ai Makabi <makabi at google.com>'s
request for review:
Bug 67269: Provides a stand alone CSSWG reftest runner which can be used
internally.
https://bugs.webkit.org/show_bug.cgi?id=67269

Attachment 105885: Patch
https://bugs.webkit.org/attachment.cgi?id=105885&action=review

------- Additional Comments from Hayato Ito <hayato at chromium.org>
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.