[webkit-reviews] review denied: [Bug 117831] Develop rebase info tool for EWS : [Attachment 205078] working progress patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied János Badics
<jbadics at inf.u-szeged.hu>'s request for review:
Bug 117831: Develop rebase info tool for EWS
https://bugs.webkit.org/show_bug.cgi?id=117831

Attachment 205078: working progress patch
https://bugs.webkit.org/attachment.cgi?id=205078&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=205078&action=review


Thanks for working on this feature but I think some methods and variable aren't
named properly.

> Tools/Scripts/webkitpy/tool/bot/earlywarningsystemtask.py:61
> +	       return self._run_rebased_tests()

I don't think run_rebased_tests makes sense as "rebased" is an adjective.
I would call it regenerate_expected_results instead.

> Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:55
> +    def _get_test_name_stub(self, expected):

We don't normally use get_ prefixes.

> Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:61
> +	   match = re.match(os.path.join('LayoutTests', 'platform', '(.+?)',
'(.*)'), absdir)

Why don't we use port.baseline_platform_dir or port.baseline_version_dir
instead?

> Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:70
> +    def _get_tests_by_expected(self, port, expected):

I don't understand why this function exits at all. Why can't the caller just
call _get_tests_in_dir?
Also, I don't understand why the third argument is called expected.

> Tools/Scripts/webkitpy/tool/steps/rebased_tests.py:83
> +    def _get_first_generic_by_filename(self, port, filename):
> +	   paths = self._get_tests_by_expected(port, filename)
> +	   filestub = self._get_test_name_stub(filename)
> +	   ret = None
> +	   for path in paths:
> +	       hit = re.match('.*' + filestub + '\..*', path)
> +	       if hit:
> +		   ret = hit.group(0)
> +		   break
> +	   return ret

What is this function trying to do?