Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
445503004:
Dismiss multiple error dialogs when running tests (Closed)
|
Created:
6 years, 4 months ago by aberent Modified:
6 years, 4 months ago CC:
chromium-reviews, klundberg+watch_chromium.org, telemetry+watch_chromium.org, yfriedman+watch_chromium.org, ilevy-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionDismiss multiple error dialogs when running tests
Previously the code would dismiss at most one error dialog before or
after running a test. This now loops until no error dialogs are shown.
This should reduce the instance of INJECTION_EVENT failures, but won't
cure them completely.
BUG=399870
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287592
Patch Set 1 #
Total comments: 4
Patch Set 2 : Accidently uploaded wrong version of one file #Patch Set 3 : Limit number of attempts to dismiss dialog #
Total comments: 4
Patch Set 4 : Fix Sami's comment #
Messages
Total messages: 18 (0 generated)
skyostil@chromium.org: Please review changes in telemetry
https://codereview.chromium.org/445503004/diff/1/build/android/pylib/instrume... File build/android/pylib/instrumentation/test_runner.py (right): https://codereview.chromium.org/445503004/diff/1/build/android/pylib/instrume... build/android/pylib/instrumentation/test_runner.py:396: for _ in xrange(10): 10 is an arbitrary number I imagine? It would be good to capture that in a comment. https://codereview.chromium.org/445503004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/445503004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:276: while self._adb.device().old_interface.DismissCrashDialogIfNeeded(): I'm a bit wary of using this in a (potentially endless) loop. Can we limit this to the same 10 tries to make sure we don't lock up bots due to some silly edge-case?
https://codereview.chromium.org/445503004/diff/1/build/android/pylib/instrume... File build/android/pylib/instrumentation/test_runner.py (right): https://codereview.chromium.org/445503004/diff/1/build/android/pylib/instrume... build/android/pylib/instrumentation/test_runner.py:396: for _ in xrange(10): On 2014/08/05 14:03:43, Peter Beverloo wrote: > 10 is an arbitrary number I imagine? It would be good to capture that in a > comment. Already fixed. See patch set 3. https://codereview.chromium.org/445503004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/445503004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:276: while self._adb.device().old_interface.DismissCrashDialogIfNeeded(): On 2014/08/05 14:03:44, Peter Beverloo wrote: > I'm a bit wary of using this in a (potentially endless) loop. Can we limit this > to the same 10 tries to make sure we don't lock up bots due to some silly > edge-case? Already fixed. See patch set 3.
https://codereview.chromium.org/445503004/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/445503004/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:279: if not self._adb.device().old_interface.DismissCrashDialogIfNeeded(): Are there any users of this API that don't want all dialogs to be dismissed? I wonder whether the for-loop should be inside it instead of around it?
https://codereview.chromium.org/445503004/diff/40001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py (right): https://codereview.chromium.org/445503004/diff/40001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/android_browser_backend.py:279: if not self._adb.device().old_interface.DismissCrashDialogIfNeeded(): On 2014/08/05 14:34:01, tonyg wrote: > Are there any users of this API that don't want all dialogs to be dismissed? I > wonder whether the for-loop should be inside it instead of around it? I considered this, but code in test_runner.py (see above) wants to check whether the dialog comes from the test package, so it is easiest to leave the loop outside. The alternative would be to return a list of the packages which had had error dialogs.
lgtm
Thanks Anthony -- one comment below. https://codereview.chromium.org/445503004/diff/40001/build/android/pylib/inst... File build/android/pylib/instrumentation/test_runner.py (right): https://codereview.chromium.org/445503004/diff/40001/build/android/pylib/inst... build/android/pylib/instrumentation/test_runner.py:398: for _ in xrange(10): How about reducing the repetition a bit like this: for _ in xrange(10): package = self.device.old_interface.DismissCrashDialogIfNeeded() if not package: break # Assume test package convention of ".test" suffix if package in self.test_pkg.GetPackageName(): result_type = base_test_result.ResultType.CRASH break
https://codereview.chromium.org/445503004/diff/40001/build/android/pylib/inst... File build/android/pylib/instrumentation/test_runner.py (right): https://codereview.chromium.org/445503004/diff/40001/build/android/pylib/inst... build/android/pylib/instrumentation/test_runner.py:398: for _ in xrange(10): On 2014/08/05 14:53:05, Sami wrote: > How about reducing the repetition a bit like this: > > for _ in xrange(10): > package = self.device.old_interface.DismissCrashDialogIfNeeded() > if not package: > break > # Assume test package convention of ".test" suffix > if package in self.test_pkg.GetPackageName(): > result_type = base_test_result.ResultType.CRASH > break Done.
The CQ bit was checked by aberent@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aberent@chromium.org/445503004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42621) mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tonyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/aberent@chromium.org/445503004/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42749)
Message was sent while issue was closed.
Change committed as 287592 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
