[Webkit-unassigned] [Bug 53908] Get rid of code which writes test results from test_type/* classes.

[bugzilla-daemon at webkit.org]

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


Ojan Vafai <ojan at chromium.org> changed:

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




--- Comment #6 from Ojan Vafai <ojan at chromium.org>  2011-02-17 00:58:09 PST ---
(From update of attachment 81610)
View in context: https://bugs.webkit.org/attachment.cgi?id=81610&action=review

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:53
> +            writer.copy_image_hash(driver_output.image_hash,
> +                                   expected_driver_output.image_hash)

webkit doesn't have an 80char rule. I think this line and the ones below would be easier to read on one line.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:57
> +            writer.copy_image_hash(driver_output.image_hash,
> +                                   expected_image_hash=None)

ditto

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:61
> +            writer.copy_image_hash(driver_output.image_hash,
> +                                   expected_driver_output.image_hash)

ditto

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:75
> +    # whether iamges are same or not. So need this hack until we have a better

typo: iamges

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:92
> +    FILENAME_SUFFIX_COMPARE = "-diff.png"

FILENAME_SUFFIX_IMAGE_DIFF ?

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:95
> +        """Initialize a TestTypeBase object.

This comment is wrong and not necessary anyways (i.e. clearly __init__ initializes). :)

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:109
> +        """Creates the output directory (if needed) for a given test
> +        filename."""

no need to wrap here

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:125
> +        For example, if filename is c:/.../fast/dom/foo.html and modifier is
> +        "-expected.txt", the return value is
> +        c:/cygwin/tmp/layout-test-results/fast/dom/foo-expected.txt
> +
> +        Args:
> +          modifier: a string to replace the extension of filename with
> +
> +        Return:
> +          The absolute windows path to the output filename

I think these comments are outdated. I don't see anything windows specific about these paths.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:131
>> +    def _write_into_file_at_path(self, file_path, contents, encoding):
> 
> You can get rid of the encoding parameter now, right? At which point, this whole function might be kind of unnecessary.

Looks like it's still used to me. There one case where 'ascii' is passed in. Although, I'm not convinced it's needed for the image hashes.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:158
> +        actual_filename = self.output_filename(self.FILENAME_SUFFIX_ACTUAL +
> +                                               file_type)
> +        expected_filename = self.output_filename(self.FILENAME_SUFFIX_EXPECTED +
> +                                                 file_type)

ditto: re 80c wrapping. this would be more readable without wrapping.

> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:160
> +        # FIXME: This function is poorly designed.  We should be passing in some
> +        # sort of encoding information from the callers.

I'm confused by this FIXME. Aren't we already already passing the encoding from the callers?

> Tools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:69
>              # Text doesn't match, write output files.

This comment no longer matches. It's a pretty useless comment anyways. Please just delete it. :)

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