[webkit-reviews] review denied: [Bug 52039] Make rebaseline server usable with bot results : [Attachment 78195] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Mihai Parparita
<mihaip at chromium.org>'s request for review:
Bug 52039: Make rebaseline server usable with bot results
https://bugs.webkit.org/show_bug.cgi?id=52039

Attachment 78195: Patch
https://bugs.webkit.org/attachment.cgi?id=78195&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=78195&action=review

Seems this needs at least one more round.

> Tools/Scripts/webkitpy/common/system/filesystem_mock.py:112
> +	   if not path.endswith('/'):

Do we need to normalize the path?  symlinks? ..?  foo/bar// etc?

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:171
> +	   def get_result_path(suffix):

We don't normally use get_ in names, right?

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:173
> +	       return test_config.filesystem.join(
> +		   test_config.results_directory, test_name + suffix)

Most our code doesn't wory about 80c, but it's not a deal breaker.

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:189
> +	       results_expected_path = test_config.filesystem.join(
> +		   test_config.results_directory,
> +		   test_name + '-expected' + extension)
> +	       if test_config.filesystem.isfile(results_expected_path):
> +		   return results_expected_path
> +	       test_path = test_config.filesystem.join(
> +		   test_config.layout_tests_directory, test_file)
> +	       test_baselines = test_config.test_port.expected_baselines(
> +		   test_path, extension)
> +	       return test_config.filesystem.join(
> +		   test_baselines[0][0], test_baselines[0][1])

This is really hard to parse.  Can't we make this easier to read?

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:198
> +	       file_path = get_result_path('-actual.checksum')

Yeah, just drop the get_ it's awkward (and I think explicitly avoided in
webkitstyle)

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:420
> +def _construct_results_json_from_directory(test_config):

This feels a lot like what we do in LayoutTestResults to construct TestResults
objects from a results.html file.  I wonder if these concepts should be merged.


> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:439
> +	   failure_type = _RESULT_EXTENSION_TO_FAILURE_TYPE.get(
> +	       test_extension, 'FAIL')

80c is just hurting readability here.

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver.py:445
> +	       filesystem.listdir(
> +		   filesystem.join(
> +		       test_config.layout_tests_directory, test_dir)),
> +	       test_name + '.*')

and here, wow.

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:220
> +	   test_config = get_test_config(

get_ must be google style?

> Tools/Scripts/webkitpy/tool/commands/rebaselineserver_unittest.py:248
> +    def test(self):

A more specific name here?