Bug 67269 – Provides a stand alone CSSWG reftest runner which can be used internally.

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Hayato Ito]

Before integrating CSSWG reftests runner with new-run-webkit-test.py, I'd like to have a simple CSSWG reftests runner, which just prints results of CSSWG reftests to standard output to make sure there is not a significant issue in supporting CSSWG reftests in Webkit.
That should be used internally and should not be integrated with new-run-webkit-tests yet.

After having such a CSSWG reftests runner, we'll try to integrate that to new-run-webkit-tests.

[Ai Makabi]

Created attachment 105885 [details]
Patch

[Hayato Ito]

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

[Hayato Ito]

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.

[Ai Makabi]

Created attachment 105948 [details]
Patch

[Ai Makabi]

Comment on attachment 105885 [details]
Patch

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

[Hayato Ito]

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

[Ai Makabi]

Created attachment 106077 [details]
Patch

[Ai Makabi]

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

[Hayato Ito]

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.

[Shinichiro Hamaji]

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)

?

[Ryosuke Niwa]

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

[Hayato Ito]

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