Bug 49573 – new-run-webkit-tests: rename TestInfo to TestInput, move image hash to work thread

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: rename TestInfo to TestInput, move image hash to work thread

[Dirk Pranke]

Created attachment 73947 [details]
Patch

[Hayato Ito]

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.

[Ojan Vafai]

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.

[Ojan Vafai]

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.

[Dirk Pranke]

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

[Dirk Pranke]

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

[Dirk Pranke]

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.

[Dirk Pranke]

(In reply to comment #6)
> needs_image_checksum (now renamed to test_input_needs_image_checksum 

make that _should_fetch_expected_checksum()

[Dirk Pranke]

Created attachment 74054 [details]
Patch

[Dirk Pranke]

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

[Dirk Pranke]

Comment on attachment 74054 [details]
Patch

Clearing flags on attachment: 74054

Committed r72149: <http://trac.webkit.org/changeset/72149>

[Dirk Pranke]

All reviewed patches have been landed.  Closing bug.