Bug 49431

Created attachment 73719 [details] testoutput-refactor

Created attachment 73720 [details] testoutput-refactor-fix-typo-and-remove-unused-functions

Comment on attachment 73720 [details] testoutput-refactor-fix-typo-and-remove-unused-functions View in context: https://bugs.webkit.org/attachment.cgi?id=73720&action=review I've only gotten about half-way through so far, but it looks pretty good; definitely on the right track. I will finish reviewing tomorrow, as it's now 2:30 in the morning my time ;) > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:82 > + self.image = image Don't worry about lazy retrieval. Once the multi-processing patch lands, all of the image retrieval will only be done when it's needed. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:241 > + image_hash_to_driver) Ideally driver.run_test() returns a TestOutput. Then you can pull _get_driver_output_image() inside run_test(), and skip the write to the filesystem for DumpRenderTree-based implementations. I actually also want there to be a TestInput class that is the sole argument passed to run_test(), but that's a separate patch :). Feel free to do that as well, if you like; it's less important. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:249 > + test_output, end - start) I think you can and should pull end-start into TestOutput.test_time (or something like that). I think all of the fields in test_info should also be in TestOutput (and TestInput), so you don't need to pass both test_info and test_output. I.e. TestOutput subclasses TestInput which subclasses TestInfo, or something. Actually, I think once the multiprocessing stuff lands TestInput *is* TestInfo. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:286 > self._driver.stop() We eventually need to flip these things so that SingleTestThread returns the TestOutput, not the TestResult, and that way process_output can become an instance method on TestShellThread instead of a static function (this allows us to pare off two more arguments to process output, and the routine actually becomes manageable). But that's a separate patch as well.

Comment on attachment 73720 [details] testoutput-refactor-fix-typo-and-remove-unused-functions View in context: https://bugs.webkit.org/attachment.cgi?id=73720&action=review Change looks good otherwise. If you can make the changes suggested above, the next rev should be fine. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:77 > + error: any unexpected or additional (or error) text output Please add test_run_time to this as well. > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:118 > + return port.expected_checksum(filename) Does having these three routines really buy you anything over just calling port.expected_* in _expected_test_output(), below? > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:214 > +def _image_hash_to_driver(port, filename, test_args, options): Maybe rename this to get_expected_image_hash() or get_input_image_hash()? >> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:249 >> + test_output, end - start) > > I think you can and should pull end-start into TestOutput.test_time (or something like that). > > I think all of the fields in test_info should also be in TestOutput (and TestInput), so you don't need to pass both test_info and test_output. I.e. TestOutput subclasses TestInput which subclasses TestInfo, or something. Actually, I think once the multiprocessing stuff lands TestInput *is* TestInfo. (this repeats the comment above in the TestOutput constructor (a bit)). > WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py:108 > + test_args.new_baseline) Much cleaner :)

Created attachment 74049 [details] updated-with-comments

Thank you for the review. I've updated the patch. (In reply to comment #3) > (From update of attachment 73720 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73720&action=review > > I've only gotten about half-way through so far, but it looks pretty good; definitely on the right track. I will finish reviewing tomorrow, as it's now 2:30 in the morning my time ;) > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:82 > > + self.image = image > > Don't worry about lazy retrieval. Once the multi-processing patch lands, all of the image retrieval will only be done when it's needed. Thank you for letting me know. I've removed the comment about lazy retrieval. > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:241 > > + image_hash_to_driver) > > Ideally driver.run_test() returns a TestOutput. Then you can pull _get_driver_output_image() inside run_test(), and skip the write to the filesystem for DumpRenderTree-based implementations. Done. Now driver.run_test() returns TestOutput. > > I actually also want there to be a TestInput class that is the sole argument passed to run_test(), but that's a separate patch :). Feel free to do that as well, if you like; it's less important. > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:249 > > + test_output, end - start) > > I think you can and should pull end-start into TestOutput.test_time (or something like that). Done. I've added a test_time field to TestOutput. > > I think all of the fields in test_info should also be in TestOutput (and TestInput), so you don't need to pass both test_info and test_output. I.e. TestOutput subclasses TestInput which subclasses TestInfo, or something. Actually, I think once the multiprocessing stuff lands TestInput *is* TestInfo. Thank you for suggestion. I agree that the current data grouping is not ideal. Instead of fixing it in this patch, I'd like to rewrite TestInfo completely in a separate patch. My very *rough* idea about grouping is: TestInfo object (which is created for each test.) which has: - filename, uri and so on. - expected_test_output (a TestOutput object) - actual_test_output: (a TestOutput object) - test_result: (a TestResult object) TestOutput object (an actual (or an expected) output) which has: - text - image - image_hash - (Optional) crash, test_time, timeout, error, TestResult: (a result of comparing an actual output with an expected output) - test_failures - time_diff - and so on. TestEnvironment object (which should be immutable once created and is shared in all tests) - test_args - test_types - port - options - and so on... I think we have to pass only two parameters, test_env and test_info, in most cases by this grouping. Maybe we have to pass just one parameter, test_info, if each test_info has a reference to a global test_env object. What do you think about this idea? I'll try to refactor if you guys agree my idea. Anyway, such a change may be a separate patch, I think. > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:286 > > self._driver.stop() > > We eventually need to flip these things so that SingleTestThread returns the TestOutput, not the TestResult, and that way process_output can become an instance method on TestShellThread instead of a static function (this allows us to pare off two more arguments to process output, and the routine actually becomes manageable). But that's a separate patch as well. (In reply to comment #4) > (From update of attachment 73720 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73720&action=review > > Change looks good otherwise. If you can make the changes suggested above, the next rev should be fine. > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:77 > > + error: any unexpected or additional (or error) text output > > Please add test_run_time to this as well. test_run_time is now included in TestOutput object. > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:118 > > + return port.expected_checksum(filename) > > Does having these three routines really buy you anything over just calling port.expected_* in _expected_test_output(), below? I agree. The three routines don't have a significant reason to exist in this refactoring stage. Sorry for confusing. I've removed the three routines. I'll make such separate functions again when I try to add reftest support. > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:214 > > +def _image_hash_to_driver(port, filename, test_args, options): > > Maybe rename this to get_expected_image_hash() or get_input_image_hash()? I am afraid that such a naming, get_input_image_hash(), is confusing because a image_hash parameter to be passed to driver.run_test() is not always same to a result of get_expected_image_hash(). I just wanted to emphasize the a this function's return value is not same to *expected_image_hash* by such a naming.

(In reply to comment #6) > Thank you for the review. I've updated the patch. > > (In reply to comment #3) > > (From update of attachment 73720 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=73720&action=review > > > > I've only gotten about half-way through so far, but it looks pretty good; definitely on the right track. I will finish reviewing tomorrow, as it's now 2:30 in the morning my time ;) > > > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:82 > > > + self.image = image > > > > Don't worry about lazy retrieval. Once the multi-processing patch lands, all of the image retrieval will only be done when it's needed. > > Thank you for letting me know. I've removed the comment about lazy retrieval. > > > > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:241 > > > + image_hash_to_driver) > > > > Ideally driver.run_test() returns a TestOutput. Then you can pull _get_driver_output_image() inside run_test(), and skip the write to the filesystem for DumpRenderTree-based implementations. > > Done. Now driver.run_test() returns TestOutput. > > > > > I actually also want there to be a TestInput class that is the sole argument passed to run_test(), but that's a separate patch :). Feel free to do that as well, if you like; it's less important. > > > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:249 > > > + test_output, end - start) > > > > I think you can and should pull end-start into TestOutput.test_time (or something like that). > > Done. I've added a test_time field to TestOutput. > > > > > I think all of the fields in test_info should also be in TestOutput (and TestInput), so you don't need to pass both test_info and test_output. I.e. TestOutput subclasses TestInput which subclasses TestInfo, or something. Actually, I think once the multiprocessing stuff lands TestInput *is* TestInfo. > > Thank you for suggestion. I agree that the current data grouping is not ideal. > Instead of fixing it in this patch, I'd like to rewrite TestInfo completely in a separate patch. > I've actually done the rewriting of TestInput in bug 49573 :) > My very *rough* idea about grouping is: > > TestInfo object (which is created for each test.) which has: > - filename, uri and so on. > - expected_test_output (a TestOutput object) > - actual_test_output: (a TestOutput object) > - test_result: (a TestResult object) > There is definitely a TestInput concept separate from the "everything" TestInfo concept you have above. My other concern with TestInfo as you have it above is that it is quite large (containing two PNGs as strings), and there is no place that needs all of that data at once. I retract my earlier comment about TestOutput having all the fields that TestInput has; there's no real reason for this to be true either. They just share the primary key (test_name). > TestEnvironment object (which should be immutable once created and is shared in all tests) > - test_args > - test_types > - port > - options test_args needs to go away (we need to just use options and pass the worker number to the create_driver() call). I'm not sure if test_types needs to exist or not yet. So, I'm not sure if the concept of a TestEnvironment helps that much, although I appreciate the idea. Up to you to try it out and see if it simplifies things or not. Definitely a separate patch :) > > > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:214 > > > +def _image_hash_to_driver(port, filename, test_args, options): > > > > Maybe rename this to get_expected_image_hash() or get_input_image_hash()? > > I am afraid that such a naming, get_input_image_hash(), is confusing because a image_hash parameter to be passed to driver.run_test() is not always same to a result of get_expected_image_hash(). > I just wanted to emphasize the a this function's return value is not same to *expected_image_hash* by such a naming. Good point. In bug 49573, I broke this into two separate functions to better show the control flow. I will try to stop changing code out from under you now :) There's more cleanup that can be done, but this is enough changes for now. Patch LGTM, but I'm not a reviewer.

Created attachment 74125 [details] merge-tot

Hi, Dirk Thank you for the review. I've updated the patch, merging to ToT. I've added Ojan to cc for review. Hi, Ojan I'll appreciate if you review this patch. > > I've actually done the rewriting of TestInput in bug 49573 :) > > > There's more cleanup that can be done, but this is enough changes for now. Patch LGTM, but I'm not a reviewer.

Comment on attachment 74125 [details] merge-tot View in context: https://bugs.webkit.org/attachment.cgi?id=74125&action=review > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:96 > + test_output: a actual test_output not sure what "actual" means in this context > WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py:94 > + expected_test_output): indentation is off > WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:-58 > - def _get_normalized_expected_text(self, filename): isn't this still being used below?

Created attachment 74148 [details] fix-comment-and-indent

Hi Ojan, Thank you for the review. I've updated the patch. Could you review this again? (In reply to comment #10) > (From update of attachment 74125 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74125&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:96 > > + test_output: a actual test_output > > not sure what "actual" means in this context I've updated the comment. I should have removed 'actual' from here because there is no 'expected' test_ouput in this point. Thank you. > > > WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py:94 > > + expected_test_output): > > indentation is off Done. > > > WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py:-58 > > - def _get_normalized_expected_text(self, filename): > > isn't this still being used below? This is not used anymore. Now a 'expected_text' is passed from a caller. So we don't need this function.

Comment on attachment 74148 [details] fix-comment-and-indent Clearing flags on attachment: 74148 Committed r72249: <http://trac.webkit.org/changeset/72249>

All reviewed patches have been landed. Closing bug.

(In reply to comment #11) > Created an attachment (id=74148) [details] > fix-comment-and-indent This patch looks good. You even threw in some other cleanup that I had refrained from suggesting :) What made you decide that image_checksum should not be a field on TestInput?

Oh. I've already submitted this patch. I'll do other clean up in a separate patch. (In reply to comment #15) > (In reply to comment #11) > > Created an attachment (id=74148) [details] [details] > > fix-comment-and-indent > > This patch looks good. You even threw in some other cleanup that I had refrained from suggesting :) > > What made you decide that image_checksum should not be a field on TestInput? I am sorry for the lack of explanations. I should have mentioned it. I think that image_cheksum should be a member of (expected) TestOutput object. That should be enough, I think. So I've removed that from a TestInput.

(In reply to comment #16) > Oh. I've already submitted this patch. I'll do other clean up in a separate patch. > > (In reply to comment #15) > > (In reply to comment #11) > > > Created an attachment (id=74148) [details] [details] [details] > > > fix-comment-and-indent > > > > This patch looks good. You even threw in some other cleanup that I had refrained from suggesting :) > > > > What made you decide that image_checksum should not be a field on TestInput? > > I am sorry for the lack of explanations. I should have mentioned it. > > I think that image_cheksum should be a member of (expected) TestOutput object. That should be enough, I think. So I've removed that from a TestInput. But it still needs to be passed to DRT as a (semi-optional) input, though. To me, that sort of qualifies it for being on the TestInput object. I don't feel super strongly about this, but not having it be part of the TestInput keeps the interface to Driver.run_test() from being simply a single TestInput parameter, which feels pretty clean. Best case, it would be a TestInput plus a checksum, which would be more awkward. Or you could do what you did, which is have a TestInput class that isn't actually handed as input to the test driver, which is unfortunate. It's definitely a judgement call, as it's not clear that the solution I describe above is better than what you've done. I was wondering what your arguments for *not* including it in TestInput were. Duplication between TestInput and expected TestOutput is one potential argument; were there others?

Hi Dirk, I agree that the interface to Driver.run_test() becomes simple if TestInput has a checksum field. I've missed that fact. That should be a reason that TestInput has a checksum field. Before reading your comment, I had sent another patch, which removes 'uri' field from TestInput for reviews: https://bugs.webkit.org/show_bug.cgi?id=49691 So I'll make another patch which makes Driver.run_test() take only one parameter, TestInput, to simplify Driver.run_test() after the above patch is reviewed and landed. Anyway, thank you for your attention. I appreciated that. (In reply to comment #17) > (In reply to comment #16) > > Oh. I've already submitted this patch. I'll do other clean up in a separate patch. > > > > (In reply to comment #15) > > > (In reply to comment #11) > > > > Created an attachment (id=74148) [details] [details] [details] [details] > > > > fix-comment-and-indent > > > > > > This patch looks good. You even threw in some other cleanup that I had refrained from suggesting :) > > > > > > What made you decide that image_checksum should not be a field on TestInput? > > > > I am sorry for the lack of explanations. I should have mentioned it. > > > > I think that image_cheksum should be a member of (expected) TestOutput object. That should be enough, I think. So I've removed that from a TestInput. > > But it still needs to be passed to DRT as a (semi-optional) input, though. To me, that sort of qualifies it for being on the TestInput object. I don't feel super strongly about this, but not having it be part of the TestInput keeps the interface to Driver.run_test() from being simply a single TestInput parameter, which feels pretty clean. Best case, it would be a TestInput plus a checksum, which would be more awkward. Or you could do what you did, which is have a TestInput class that isn't actually handed as input to the test driver, which is unfortunate. > > It's definitely a judgement call, as it's not clear that the solution I describe above is better than what you've done. I was wondering what your arguments for *not* including it in TestInput were. Duplication between TestInput and expected TestOutput is one potential argument; were there others?