Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2148283003:
Adding the ability to fake crash the browser in _FakeBrowser (Closed)
DescriptionAdding the ability to fake crash the browser in _FakeBrowser
BUG=catapult:#628022
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/a558b9bf0e65a2dab8bed8afc2f39010445bb948
Patch Set 1 #
Total comments: 3
Patch Set 2 : Updating crash functionatliy #
Total comments: 2
Patch Set 3 : Updating crash navigate url #
Total comments: 2
Patch Set 4 : Supressing unused variable warnings #Messages
Total messages: 23 (10 generated)
eyaich@google.com changed reviewers: + nednguyen@google.com
https://codereview.chromium.org/2148283003/diff/1/telemetry/telemetry/testing... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2148283003/diff/1/telemetry/telemetry/testing... telemetry/telemetry/testing/fakes/__init__.py:339: self.browser._is_crashed = True This is a clever idea. Though I think it should raise exception right to match with the actual browser's behavior.
kbr@chromium.org changed reviewers: + kbr@chromium.org
https://codereview.chromium.org/2148283003/diff/1/telemetry/telemetry/testing... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2148283003/diff/1/telemetry/telemetry/testing... telemetry/telemetry/testing/fakes/__init__.py:339: self.browser._is_crashed = True On 2016/07/14 19:05:50, nednguyen wrote: > This is a clever idea. Though I think it should raise exception right to match > with the actual browser's behavior. Right, I think Navigate() will raise an exception if this URL is navigated to. It's useful to be able to test this behavior in unittests. However, to simulate the behavior of the entire browser crashing in GpuIntegrationTest.setUp(), I think a different mechanism is needed. You could check for the URL string chrome://inducebrowsercrashforrealz : https://cs.chromium.org/chromium/src/content/public/common/url_constants.cc?s... and do what you've got here. Then, in SimpleIntegrationUnittest.setUp(), navigate to that URL before calling super(...).setUp(). Does that work?
Description was changed from ========== Adding the ability to fake crash the browser in _FakeBrowser BUG=628022 ========== to ========== Adding the ability to fake crash the browser in _FakeBrowser BUG=catapult:628022 ==========
Description was changed from ========== Adding the ability to fake crash the browser in _FakeBrowser BUG=catapult:628022 ========== to ========== Adding the ability to fake crash the browser in _FakeBrowser BUG=catapult:#628022 ==========
https://codereview.chromium.org/2148283003/diff/1/telemetry/telemetry/testing... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2148283003/diff/1/telemetry/telemetry/testing... telemetry/telemetry/testing/fakes/__init__.py:339: self.browser._is_crashed = True On 2016/07/14 21:38:25, Ken Russell wrote: > On 2016/07/14 19:05:50, nednguyen wrote: > > This is a clever idea. Though I think it should raise exception right to match > > with the actual browser's behavior. > > Right, I think Navigate() will raise an exception if this URL is navigated to. > It's useful to be able to test this behavior in unittests. However, to simulate > the behavior of the entire browser crashing in GpuIntegrationTest.setUp(), I > think a different mechanism is needed. > > You could check for the URL string chrome://inducebrowsercrashforrealz : > https://cs.chromium.org/chromium/src/content/public/common/url_constants.cc?s... > > and do what you've got here. Then, in SimpleIntegrationUnittest.setUp(), > navigate to that URL before calling super(...).setUp(). Does that work? Yes, you are right that it should throw an exception, so I added that. I changed the URL to chrome://inducebrowsercrashforrealz but like I noted in the other url, I don' think it matters whether we call it before or after. Since the failure we are writing this try/catch block for is actually the test that is run after this error is thrown, that is where we care about trying to restart the browser. I moved it in the other CL either way.
lgtm
LGTM and apologies for the waffling. https://codereview.chromium.org/2148283003/diff/20001/telemetry/telemetry/tes... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2148283003/diff/20001/telemetry/telemetry/tes... telemetry/telemetry/testing/fakes/__init__.py:338: if url == 'chrome://inducebrowsercrashforrealz': I'm sorry to go back and forth on this, but per your other CL https://codereview.chromium.org/2151983002/ , could you change this back to chrome://crash?
https://codereview.chromium.org/2148283003/diff/20001/telemetry/telemetry/tes... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2148283003/diff/20001/telemetry/telemetry/tes... telemetry/telemetry/testing/fakes/__init__.py:338: if url == 'chrome://inducebrowsercrashforrealz': On 2016/07/15 18:07:26, Ken Russell wrote: > I'm sorry to go back and forth on this, but per your other CL > https://codereview.chromium.org/2151983002/ , could you change this back to > chrome://crash? Done.
The CQ bit was checked by eyaich@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2148283003/#ps40001 (title: "Updating crash navigate url")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Catapult Presubmit on master.tryserver.client.catapult (JOB_FAILED, https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Pr...)
https://codereview.chromium.org/2148283003/diff/40001/telemetry/telemetry/tes... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2148283003/diff/40001/telemetry/telemetry/tes... telemetry/telemetry/testing/fakes/__init__.py:337: timeout=0): The way you suppress the unused-arguments presubmit is: del url, script_to_evaluate_on_commit, timeout # Unused.
https://codereview.chromium.org/2148283003/diff/40001/telemetry/telemetry/tes... File telemetry/telemetry/testing/fakes/__init__.py (right): https://codereview.chromium.org/2148283003/diff/40001/telemetry/telemetry/tes... telemetry/telemetry/testing/fakes/__init__.py:337: timeout=0): On 2016/07/16 00:30:52, nednguyen wrote: > The way you suppress the unused-arguments presubmit is: > del url, script_to_evaluate_on_commit, timeout # Unused. Thanks! Done.
The CQ bit was checked by eyaich@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nednguyen@google.com, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2148283003/#ps60001 (title: "Supressing unused variable warnings")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adding the ability to fake crash the browser in _FakeBrowser BUG=catapult:#628022 ========== to ========== Adding the ability to fake crash the browser in _FakeBrowser BUG=catapult:#628022 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
