| Summary: | new-run-webkit-tests: rename TestInfo to TestInput, move image hash to work thread | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Pranke <dpranke> | ||||||
| Component: | New Bugs | Assignee: | Dirk Pranke <dpranke> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | eric, hayato, ojan, tony | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | OS X 10.5 | ||||||||
| Bug Depends on: | |||||||||
| Bug Blocks: | 49566 | ||||||||
| Attachments: |
|
||||||||
|
Description
Dirk Pranke
2010-11-15 17:02:43 PST
Created attachment 73947 [details]
Patch
Comment on attachment 73947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73947&action=review > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:193 > + if _needs_image_checksum(self._options): I am not sure what is a correct behaviour, but it seems your patch will change the behaviour. I mean: Before your patch: If test_args.new_baseline is true, self._driver.run_test() always receives 'None' as an image_hash. After your patch: If test_args.new_baseline is true, self._driver.run_test() may receive non-None value as a image_hash if a test has a '.checksum' file. I am wondering that is your intentional or not. Comment on attachment 73947 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73947&action=review >> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:193 >> + if _needs_image_checksum(self._options): > > I am not sure what is a correct behaviour, but it seems your patch will change the behaviour. I mean: > > Before your patch: > If test_args.new_baseline is true, self._driver.run_test() always receives 'None' as an image_hash. > > After your patch: > If test_args.new_baseline is true, self._driver.run_test() may receive non-None value as a image_hash if a test has a '.checksum' file. > > I am wondering that is your intentional or not. Can we make _needs_image_checksum a method of TestInput and give TestInput a populate_checksum method that takes port and options as arguments? Maybe it should be populate_checksum_and_uri. Then the filename_to_uri logic could go back into TestInput as well. Whoops. Hit publish too early. I meant to say: r- just to address the two questions brought up. Otherwise, looks like a good change. On a side note, this patch would have been much easier to review if it were a rename patch followed by the logic change patch as the rename is trivial and 90% of the diff. I'm also not sure if the change in behavior matters. I'm fine with the change if you are confident it won't break anything. (In reply to comment #2) > (From update of attachment 73947 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73947&action=review > > > WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:193 > > + if _needs_image_checksum(self._options): > > I am not sure what is a correct behaviour, but it seems your patch will change the behaviour. I mean: > > Before your patch: > If test_args.new_baseline is true, self._driver.run_test() always receives 'None' as an image_hash. > > After your patch: > If test_args.new_baseline is true, self._driver.run_test() may receive non-None value as a image_hash if a test has a '.checksum' file. > > I am wondering that is your intentional or not. Good catch. That was not an intentional change. Will fix in the next patch. (In reply to comment #3) > Can we make _needs_image_checksum a method of TestInput and give TestInput a populate_checksum method that takes port and options as arguments? Maybe it should be populate_checksum_and_uri. Then the filename_to_uri logic could go back into TestInput as well. I would prefer not to make these changes. TestInput is really just a data class with no logic that exists so that we don't have to pass a bunch of individual fields around. needs_image_checksum (now renamed to test_input_needs_image_checksum is solely concerned with the command line flags passed to NRWT, and so the logic feels like it makes more sense elsewhere, ideally as a member function on the TestShellThread. Similarly, the fact that TestInput holds a URI field is actually a mistake. The correct signature to Driver.run_test() should be to take a test name (which is a unix-style relative filename), rather than the full path (which is what it's getting) or a URI (which some implementations don't actually need). TestInput's input parameter should be changed, and URI should be removed (letting the port call filename_to_uri as necessary). I've added FIXMEs for those, and will make the changes in other patches. Does that all make sense? Oh, I should have mentioned that I realize you possibly made these suggestions because the logic is being repeated in two different places; that's definitely an issue. One of the later patches in this set will eliminate this. (In reply to comment #6) > needs_image_checksum (now renamed to test_input_needs_image_checksum make that _should_fetch_expected_checksum() Created attachment 74054 [details]
Patch
(In reply to comment #4) > On a side note, this patch would have been much easier to review if it were a rename patch followed by the logic change patch as the rename is trivial and 90% of the diff. True, and good point. I can attempt to isolate things better in the future. I was thinking of both things as part of a general refactoring cleanup that was logically one change. Comment on attachment 74054 [details] Patch Clearing flags on attachment: 74054 Committed r72149: <http://trac.webkit.org/changeset/72149> All reviewed patches have been landed. Closing bug. |