[Webkit-unassigned] [Bug 67269] Provides a stand alone CSSWG reftest runner which can be used internally.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67269


Hayato Ito <hayato at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #105885|review?                     |review-
               Flag|                            |




--- Comment #3 from Hayato Ito <hayato at chromium.org>  2011-08-31 23:54:01 PST ---
(From update of attachment 105885)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.