Bug 49573 - new-run-webkit-tests: rename TestInfo to TestInput, move image hash to work thread
Summary: new-run-webkit-tests: rename TestInfo to TestInput, move image hash to work t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 49566
  Show dependency treegraph
 
Reported: 2010-11-15 17:02 PST by Dirk Pranke
Modified: 2010-11-16 16:36 PST (History)
4 users (show)

See Also:


Attachments
Patch (16.76 KB, patch)
2010-11-15 17:07 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (17.98 KB, patch)
2010-11-16 16:09 PST, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Pranke 2010-11-15 17:02:43 PST
new-run-webkit-tests: rename TestInfo to TestInput, move image hash to work thread
Comment 1 Dirk Pranke 2010-11-15 17:07:24 PST
Created attachment 73947 [details]
Patch
Comment 2 Hayato Ito 2010-11-15 17:46:58 PST
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 3 Ojan Vafai 2010-11-16 06:44:45 PST
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.
Comment 4 Ojan Vafai 2010-11-16 06:47:23 PST
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.
Comment 5 Dirk Pranke 2010-11-16 15:49:00 PST
(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.
Comment 6 Dirk Pranke 2010-11-16 15:53:34 PST
(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?
Comment 7 Dirk Pranke 2010-11-16 15:56:42 PST
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.
Comment 8 Dirk Pranke 2010-11-16 15:59:16 PST
(In reply to comment #6)
> needs_image_checksum (now renamed to test_input_needs_image_checksum 

make that _should_fetch_expected_checksum()
Comment 9 Dirk Pranke 2010-11-16 16:09:19 PST
Created attachment 74054 [details]
Patch
Comment 10 Dirk Pranke 2010-11-16 16:10:33 PST
(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 11 Dirk Pranke 2010-11-16 16:36:17 PST
Comment on attachment 74054 [details]
Patch

Clearing flags on attachment: 74054

Committed r72149: <http://trac.webkit.org/changeset/72149>
Comment 12 Dirk Pranke 2010-11-16 16:36:23 PST
All reviewed patches have been landed.  Closing bug.