Summary: | [Chromium-Android] Should retry a few times when failed to start DumpRenderTree | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Xianzhu Wang <wangxianzhu> | ||||||||||
Component: | Tools / Tests | Assignee: | Xianzhu Wang <wangxianzhu> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, dpranke, jnd, ojan, tony, webkit.review.bot, zhenghao | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 66687 | ||||||||||||
Attachments: |
|
Description
Xianzhu Wang
2012-06-14 13:48:43 PDT
Created attachment 147647 [details]
patch
Created attachment 147649 [details]
patch v2 (sleep before retry)
Comment on attachment 147649 [details] patch v2 (sleep before retry) View in context: https://bugs.webkit.org/attachment.cgi?id=147649&action=review r+. Leaving as cq? in case you want to make my purely optional change :). > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:494 > + time.sleep(2) View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review r+ ... leaving cq? in case you want to make the change I suggest (it's purely optional). > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:493 > + raise AssertionError('Failed to start DumpRenderTree application multiple times. Give up.') You could also do: for i in range(3): if self._start_once(pixel_tests, per_test_args): return _log.error(...) time.sleep(2) raise AssertionError(...') Comment on attachment 147647 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:489 > + while True: > + if self._start_once(pixel_tests, per_test_args): > + break Nit: while not self._start_once(pixel_tests, per_test_args): > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > self._port._run_adb_command(['logcat', '-c']) > self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) Do you want to rerun these every time? Do these ever fail? Comment on attachment 147649 [details] patch v2 (sleep before retry) View in context: https://bugs.webkit.org/attachment.cgi?id=147649&action=review >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:493 >> + raise AssertionError('Failed to start DumpRenderTree application multiple times. Give up.') > > You could also do: > > for i in range(3): > if self._start_once(pixel_tests, per_test_args): > return > _log.error(...) > time.sleep(2) > raise AssertionError(...') Nice suggestion. However I'd like Tony's suggestion as we use that style elsewhere in this file :) Comment on attachment 147647 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:489 >> + break > > Nit: while not self._start_once(pixel_tests, per_test_args): Done. >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 >> self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) > > Do you want to rerun these every time? Do these ever fail? 'logcat -c' which is to clear the Android log is necessary because we don't want duplicated logs during retries. Initialize command line is not necessary to do every time, but I'd keep _start_once() a complete function to start DRT. And it is logically the precondition of 'am start' and I'd keep them together. Created attachment 147653 [details]
patch v3
(In reply to comment #6) > (From update of attachment 147647 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review > > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > >> self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) > > > > Do you want to rerun these every time? Do these ever fail? > > 'logcat -c' which is to clear the Android log is necessary because we don't want duplicated logs during retries. > Initialize command line is not necessary to do every time, but I'd keep _start_once() a complete function to start DRT. And it is logically the precondition of 'am start' and I'd keep them together. I was thinking it might be helpful to move these two commands before the while loop. It might be useful to have the logcat output for when 'am start' fails. I don't feel strongly about this. (In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 147647 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review > > > > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > > >> self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) > > > > > > Do you want to rerun these every time? Do these ever fail? > > > > 'logcat -c' which is to clear the Android log is necessary because we don't want duplicated logs during retries. > > Initialize command line is not necessary to do every time, but I'd keep _start_once() a complete function to start DRT. And it is logically the precondition of 'am start' and I'd keep them together. > > I was thinking it might be helpful to move these two commands before the while loop. It might be useful to have the logcat output for when 'am start' fails. I don't feel strongly about this. The error message of 'am start' won't be lost. Log is cleared before 'am start' to remove useless logs produced by other Android activities before this 'am start'. (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > (From update of attachment 147647 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=147647&action=review > > > > > > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:497 > > > >> self._port._run_adb_command(['shell', 'echo'] + self.cmd_line(pixel_tests, per_test_args) + ['>', COMMAND_LINE_FILE]) > > > > > > > > Do you want to rerun these every time? Do these ever fail? > > > > > > 'logcat -c' which is to clear the Android log is necessary because we don't want duplicated logs during retries. > > > Initialize command line is not necessary to do every time, but I'd keep _start_once() a complete function to start DRT. And it is logically the precondition of 'am start' and I'd keep them together. > > > > I was thinking it might be helpful to move these two commands before the while loop. It might be useful to have the logcat output for when 'am start' fails. I don't feel strongly about this. > > The error message of 'am start' won't be lost. Log is cleared before 'am start' to remove useless logs produced by other Android activities before this 'am start'. Correction: ... produced by other Android activities or previous _start_once() (which has already been outputted)... Comment on attachment 147653 [details]
patch v3
cq+. Verified with Tony offline.
Comment on attachment 147653 [details]
patch v3
Want to add a line self.stop() to ensure clear state before retry.
Created attachment 147659 [details]
patch for landing
Comment on attachment 147659 [details] patch for landing Clearing flags on attachment: 147659 Committed r120385: <http://trac.webkit.org/changeset/120385> All reviewed patches have been landed. Closing bug. |