[webkit-reviews] review denied: [Bug 103268] Make it possible to run performance tests on Chromium Android : [Attachment 176019] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Peter Beverloo
<peter at chromium.org>'s request for review:
Bug 103268: Make it possible to run performance tests on Chromium Android
https://bugs.webkit.org/show_bug.cgi?id=103268

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

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


> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:134
>  DEVICE_LAYOUT_TESTS_DIR = DEVICE_SOURCE_ROOT_DIR +
'third_party/WebKit/LayoutTests/'
> +DEVICE_PERF_TESTS_DIR = DEVICE_SOURCE_ROOT_DIR +
'third_party/WebKit/PerformanceTests/'

This is crazy. You should be able to dynamically resolve the path by replacing
the test path's root by DEVICE_SOURCE_ROOT_DIR + 'third_party/WebKit'. Also,
you should be using filesystem.join instead.
r- because of this.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:266
> +    def fail_perf_tests_on_stderr(self):
> +	   """Chromium for Android can have warnings in the stderr output for
not yet
> +	   implemented features. There is no reason to fail because of them."""

> +	   return False
> +

Please fix all those warnings before turning on performance tests.
Since webkit-perf.appspot.com is completely broken now, there isn't much point
in rushing to land this patch.
r- because of this as well.

>> Tools/Scripts/webkitpy/performance_tests/perftest.py:93
>> +		    return False
> 
> It seems like logging to stderr shouldn't cause a test to fail on any
platform, but I don't really know if that's by design or not. We should confirm
w/ rniwa.

By design, a performance test fails if we see any line we don't recognize. That
includes any line in stdout and sterr.