[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





--- Comment #7 from Hayato Ito <hayato at chromium.org>  2011-02-17 03:07:44 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
>> +                                   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.

Thank you. I've fixed the style of this file, assuming about 100 chars per line are permitted.

>> 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

Thank you. Fixed.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:81
>> +
> 
> I feel like this should still be a transitional patch in the code and we can still do better. I think the fact that we're keeping test_types as a list of classes and iterating over them without knowing what they are is just making us have to create these big complicated if blocks. I think if we got rid of the lists of classes and just created the test type objects directly in single_test runner (at line 285) and then handled the failures directly it would be a lot more obvious what's going on. But, if that's too much to change at once, this looks okay as a stopgap.

Thank you. I agree this is an ugly long 'if' blocks. I tried moving this each clause to each corresponding TestFailures class , but I didn't.

In Subsequent patches,  I will eliminate test type classes. So I think it will be possible to handle these failures directly in single_test_runner. For now, I'd like to keep this ugly if else block as is.

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

Thank you. Done.

>> 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). :)

Thank you. I removed this doc comment itself at all.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:109
>> +        filename."""
> 
> no need to wrap here

Done.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:125
>> +          The absolute windows path to the output filename
> 
> I think these comments are outdated. I don't see anything windows specific about these paths.

Thank you. I updated the comment.

>>> 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.

We can remove encoding parameter. We can assume all data is in 'binary', which includes 'ASCII' safely.

I did similar things in another patch. https://bugs.webkit.org/show_bug.cgi?id=54066

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:160
>> +        # sort of encoding information from the callers.
> 
> I'm confused by this FIXME. Aren't we already already passing the encoding from the callers?

I removed the comment.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:173
>> +    def copy_text(self, actual_text, expected_text):
> 
> I would probably call this "copy_text_files" to make it clear that you are copying both files. I was confused at first in write_test_result() because it looked like you were copying the value of actual_text to expected_text,

Thank you and sorry for confusion. I renamed all 'copy_xxx' function to 'write_xxx' . I think 'write_xx' is better than 'copy_' in these cases.

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:208
>> +    def copy_image(self, actual_image, expected_image):
> 
> smae thing ... copy_image_files()

Thank you. I renamed that to write_image_files().

>> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:213
>> +    def copy_image_hash(self, actual_image_hash, expected_image_hash):
> 
> copy_image_hashes()

Thank you. I renamed that to write_image_hashes().

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