Bug 53908

Created attachment 81462 [details] refactor-test-result-writing

Did you mean to set the r? flag for this patch?

Oh. Thank you for taking care of that. I am reluctant to set 'r?' to this patch (A) because it depends on other patch (B), https://bugs.webkit.org/show_bug.cgi?id=53071, in my local git repository. These two patches are not strongly related, but this patch (A) needs patch (B) to be applied correctly, I guess. I am now setting 'r?' to this patch. I think this patch doesn't have such a 'physical' patchablity issue. I can merge that easily.

Created attachment 81610 [details] refactor-test-result-writing

Comment on attachment 81610 [details] refactor-test-result-writing View in context: https://bugs.webkit.org/attachment.cgi?id=81610&action=review > 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. > 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. > 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, > 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() > 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() > Tools/Scripts/webkitpy/layout_tests/test_types/image_diff.py:-84 > - self._copy_image(filename, actual_driver_output.image, expected_image=None) You can actually delete _copy_image() and _copy_hash() as well, right?

Comment on attachment 81610 [details] refactor-test-result-writing 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. :)

Comment on attachment 81610 [details] refactor-test-result-writing 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().

Created attachment 82775 [details] fixed-as-suggested

Comment on attachment 82775 [details] fixed-as-suggested View in context: https://bugs.webkit.org/attachment.cgi?id=82775&action=review > Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:153 > + def write_text_diff(self, actual_text, expected_text): Unlike the other methods, write_text_diff() and write_diff_image() are actually doing the diffs as well as writing them. They should probably be renamed, and I'm not sure if they even belong in this module as opposed to being in the caller (or elsewhere). But, I'm keen to see this patch land so we can make progress, so as long as all of the tests pass this is fine for now. Maybe add a FIXME and at least rename them? LGTM otherwise.

Comment on attachment 82775 [details] fixed-as-suggested View in context: https://bugs.webkit.org/attachment.cgi?id=82775&action=review >> Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:153 >> + def write_text_diff(self, actual_text, expected_text): > > Unlike the other methods, write_text_diff() and write_diff_image() are actually doing the diffs as well as writing them. They should probably be renamed, and I'm not sure if they even belong in this module as opposed to being in the caller (or elsewhere). But, I'm keen to see this patch land so we can make progress, so as long as all of the tests pass this is fine for now. Maybe add a FIXME and at least rename them? > > LGTM otherwise. Thank you. I've renamed two functions. The two functions are now 'create_text_diff_and_write_result' and 'create_image_diff_and_write_result'. I've added a FIXME as well.

Created attachment 82897 [details] renamed-write-functions

Committed r78961: <http://trac.webkit.org/changeset/78961>

http://trac.webkit.org/changeset/78961 might have broken GTK Linux 32-bit Release