[webkit-reviews] review denied: [Bug 92299] [Chromium-Android] NRWT better handling of DRT deadlocks and crashes : [Attachment 154448] Patch

[bugzilla-daemon at webkit.org]

Dirk Pranke <dpranke at chromium.org> has denied Xianzhu Wang
<wangxianzhu at chromium.org>'s request for review:
Bug 92299: [Chromium-Android] NRWT better handling of DRT deadlocks and crashes
https://bugs.webkit.org/show_bug.cgi?id=92299

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

------- Additional Comments from Dirk Pranke <dpranke at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154448&action=review


> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:402
> +	   _log.debug('Run adb result: ' + result[:80])

nit: any particular reason to limit this to [:80], instead of the whole output?


> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:516
>	       time.sleep(1)

typically in a slow-spin look like this I'll key this off of time instead of a
count, e.g.
deadline = time.time() + DRT_START_STO_SECS:
while time.time() < deadline:
   if ...:
     break
   time.sleep(0.1)

the advantage here is that your sleep granularity isn't quite so coarse; also,
I think the logic can be a bit clearer. 

You have these slow-spins in several places in the code; it would probably be a
good idea to wrap them into a loop_with_timeout() function or something to
reuse the code to ensure consistency.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:548
> +		   time.sleep(1)

same comment re: using deadlines instead of a range and a fixed sleep interval
...

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:592
> +	   raise AssertionError('DumpRenderTree is killed by Android for too
many times!')

why do you need to retry the test? is there anything else running on the
android device that might be causing OOMs and/or causing the DRT to get killed?
It seems like you should just treat this as a crash and return and let the rest
of the infrastructure in NRWT deal with handling potential flakiness?

> Tools/Scripts/webkitpy/layout_tests/port/server_process.py:234
> +		       _log.debug('unexpected EOF of stdout')

why did you change these?