Bug 48326 – [NRWT] Don't use image hash when it's no need in single test mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Gabor Rapcsanyi]

NRWT always use image hash when a test has it in single test mode.

[Gabor Rapcsanyi]

Created attachment 71885 [details]
proposed patch

[Ojan Vafai]

Comment on attachment 71885 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71885&action=review

Can you move this into a function call so the logic can be shared with the logic on line 521?

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:202
> +        image_hash = test_info.image_hash()
> +        if (image_hash and
> +            (self._test_args.new_baseline or self._test_args.reset_results or
> +            not self._options.pixel_tests)):
> +            image_hash = ""

How about:
if self._test_args.new_baseline or self._test_args.reset_results or not self._options.pixel_tests:
    image_hash = None
else:
    image_hash = test_info.image_hash()

I find that logic moe clear and readable. Also, I prefer using None to the empty string. That's more consistent with python style.

[Gabor Rapcsanyi]

Created attachment 71994 [details]
proposed_patch_v2

I have put the logic into a function as you suggested. Unfortunately these two class doesn't have any common base class so I've just created a global function for it.

[Ojan Vafai]

Comment on attachment 71994 [details]
proposed_patch_v2

View in context: https://bugs.webkit.org/attachment.cgi?id=71994&action=review

> WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:161
> +    image_hash = test_info.image_hash()
> +    if (test_args.new_baseline or test_args.reset_results or
> +        not options.pixel_tests):
> +        image_hash = None
> +    return image_hash

Standard webkit style for something like this is to use early returns. Also, there's no 80 column limit in WebKit code. You're supposed to go with what looks most readable. It's certainly arguable in this case.

if (test_args.new_baseline or test_args.reset_results or not options.pixel_tests):
    return None
return test_info.image_hash()

Please fix the early return issue. I don't feel strongly about the wrapping.

[Ojan Vafai]

(In reply to comment #3)
> I have put the logic into a function as you suggested. Unfortunately these two class doesn't have any common base class so I've just created a global function for it.

Yeah, this class could use some refactoring. Global function is fine for this patch.

[Csaba Osztrogonác]

Comment on attachment 71994 [details]
proposed_patch_v2

Landed in http://trac.webkit.org/changeset/70648 with modifications that Ojan suggested.