Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2209673003:
Unittest for pushing the restart logic into start browser. (Closed)
DescriptionUnittest for pushing restart logic into the browser. Original patch for the restart logic was checked in outside this CL in https://codereview.chromium.org/2219593003/ so it could get in earlier, this is just the follow on CL for the unittest.
BUG=chromium:628022
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/6e61cce3dc6538310ef292be09b16593f3b18c6e
Cr-Commit-Position: refs/heads/master@{#410454}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Changing logging #Patch Set 3 : Adding in test success assertions #Patch Set 4 : Updating test with new Fakes api #Patch Set 5 : Updating fakes api #Patch Set 6 : Adding back in test changes #
Total comments: 2
Patch Set 7 : tracking browser starts #Messages
Total messages: 31 (10 generated)
Description was changed from ========== Pushing the restart logic into start browser. I am having difficulty testing this functionality. I can get restart browser to fail and restart three times, but I am not sure how to mock its success on the third try. It is proving difficult because I have to get something *within* StartBrowser() to throw the exception, so that means one of the two asserts has to fail and I have no way to reset it for the next call to StartBrowser(). Any thoughts on a better way to unittest are greatly appreciated. BUG=chromium:628022 ========== to ========== Pushing the restart logic into start browser. I am having difficulty testing this functionality. I can get restart browser to fail and restart three times, but I am not sure how to mock its success on the third try. It is proving difficult because I have to get something *within* StartBrowser() to throw the exception, so that means one of the two asserts has to fail and I have no way to reset it for the next call to StartBrowser(). Any thoughts on a better way to unittest are greatly appreciated. BUG=chromium:628022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
eyaich@google.com changed reviewers: + kbr@chromium.org, nednguyen@google.com
LGTM Though I realize you haven't gotten the tests working to your satisfaction yet, I trust you to CQ this whenever you think it's ready. A suggestion about the test, and one small request. https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_integration_test.py (right): https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_integration_test.py:37: restart = 'Starting browser %d time' % (x + 1) Could you add " (of 3)" to the end? e.g. "Starting browser, attempt 1 of 3" or similar. Thanks. https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_integration_test.py:39: super(GpuIntegrationTest, cls).StartBrowser() How about adding a call to a no-op method called something like "_PossiblyFailForTesting()" or similar here? Then the test subclass can have a little per-test state that raises an exception in its override the first two times. Would that help with the unit test?
On 2016/08/03 22:21:39, Ken Russell wrote: > LGTM > > Though I realize you haven't gotten the tests working to your satisfaction yet, > I trust you to CQ this whenever you think it's ready. A suggestion about the > test, and one small request. > > https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... > File content/test/gpu/gpu_tests/gpu_integration_test.py (right): > > https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... > content/test/gpu/gpu_tests/gpu_integration_test.py:37: restart = 'Starting > browser %d time' % (x + 1) > Could you add " (of 3)" to the end? e.g. "Starting browser, attempt 1 of 3" or > similar. Thanks. > > https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... > content/test/gpu/gpu_tests/gpu_integration_test.py:39: super(GpuIntegrationTest, > cls).StartBrowser() > How about adding a call to a no-op method called something like > "_PossiblyFailForTesting()" or similar here? Then the test subclass can have a > little per-test state that raises an exception in its override the first two > times. Would that help with the unit test? Can't you add some extra field to the browser like expected_num_startup_crashes & set it to 2?
On 2016/08/03 22:56:43, nednguyen wrote: > On 2016/08/03 22:21:39, Ken Russell wrote: > > LGTM > > > > Though I realize you haven't gotten the tests working to your satisfaction > yet, > > I trust you to CQ this whenever you think it's ready. A suggestion about the > > test, and one small request. > > > > > https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... > > File content/test/gpu/gpu_tests/gpu_integration_test.py (right): > > > > > https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... > > content/test/gpu/gpu_tests/gpu_integration_test.py:37: restart = 'Starting > > browser %d time' % (x + 1) > > Could you add " (of 3)" to the end? e.g. "Starting browser, attempt 1 of 3" or > > similar. Thanks. > > > > > https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... > > content/test/gpu/gpu_tests/gpu_integration_test.py:39: > super(GpuIntegrationTest, > > cls).StartBrowser() > > How about adding a call to a no-op method called something like > > "_PossiblyFailForTesting()" or similar here? Then the test subclass can have a > > little per-test state that raises an exception in its override the first two > > times. Would that help with the unit test? > > Can't you add some extra field to the browser like expected_num_startup_crashes > & set it to 2? What do you mean by adding a field to browser? Like brower.py in internal telemetry? How would that necessarily help?
https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_integration_test.py (right): https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_integration_test.py:37: restart = 'Starting browser %d time' % (x + 1) On 2016/08/03 22:21:39, Ken Russell wrote: > Could you add " (of 3)" to the end? e.g. "Starting browser, attempt 1 of 3" or > similar. Thanks. Done. https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_integration_test.py:39: super(GpuIntegrationTest, cls).StartBrowser() On 2016/08/03 22:21:39, Ken Russell wrote: > How about adding a call to a no-op method called something like > "_PossiblyFailForTesting()" or similar here? Then the test subclass can have a > little per-test state that raises an exception in its override the first two > times. Would that help with the unit test? The problem with calling a super method here (unless I am missing something, I am new to python) is that it knows nothing about the test subclass. I could easily manufacture something in the overriden method in the test class, but that only gets called once. Each time this restarts, it only calls the super.StartBrowser method and catches any exceptions here, it doesn't return to the test subclass until the three attempts are over.
https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_integration_test_unittest.py (right): https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_integration_test_unittest.py:181: nits: this extra blank line is not needed
https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_integration_test_unittest.py (right): https://codereview.chromium.org/2209673003/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_integration_test_unittest.py:181: On 2016/08/04 17:03:48, nednguyen wrote: > nits: this extra blank line is not needed Done.
The CQ bit was checked by nednguyen@google.com
lgtm
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2209673003/#ps40001 (title: "Adding in test success assertions")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/08/04 22:24:59, nednguyen wrote: > lgtm Since Emily will be out for a while, I stamped & cq'ed this to stop 628022 from causing whole shard failures. We can improve the unittest later.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/08/04 22:32:35, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) Unfortunately, it looks like this CL doesn't pass pylint. Ned, is there any chance you can pick it up?
On 2016/08/05 00:52:32, Ken Russell wrote: > On 2016/08/04 22:32:35, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) > > Unfortunately, it looks like this CL doesn't pass pylint. Ned, is there any > chance you can pick it up? Sure, I will land Emily's first patch & add TODO(eyiach) to improve the unittest
Ok merge of Neds patch is complete and I added back in unittest changes to use the new fakes api. ptal
lgtm but let wait for Ken. Can you also update the description?
https://codereview.chromium.org/2209673003/diff/90001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/gpu_integration_test_unittest.py (right): https://codereview.chromium.org/2209673003/diff/90001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/gpu_integration_test_unittest.py:120: def CrashOnStart(cls): nit: also probably keep the counter on how many times this is called, so we can also assert that browser is started 3 times (the first 2 are crashed upon start-up)
https://codereview.chromium.org/2209673003/diff/90001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/gpu_integration_test_unittest.py (right): https://codereview.chromium.org/2209673003/diff/90001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/gpu_integration_test_unittest.py:120: def CrashOnStart(cls): On 2016/08/08 15:04:30, nednguyen wrote: > nit: also probably keep the counter on how many times this is called, so we can > also assert that browser is started 3 times (the first 2 are crashed upon > start-up) Done.
Description was changed from ========== Pushing the restart logic into start browser. I am having difficulty testing this functionality. I can get restart browser to fail and restart three times, but I am not sure how to mock its success on the third try. It is proving difficult because I have to get something *within* StartBrowser() to throw the exception, so that means one of the two asserts has to fail and I have no way to reset it for the next call to StartBrowser(). Any thoughts on a better way to unittest are greatly appreciated. BUG=chromium:628022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unittest for pushing restart logic into the browser. Original patch for the restart logic was checked in outside this CL in https://codereview.chromium.org/2219593003/ so it could get in earlier, this is just the follow on CL for the unittest. BUG=chromium:628022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Excellent. LGTM
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 Link to the patchset: https://codereview.chromium.org/2209673003/#ps110001 (title: "tracking browser starts")
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 ========== Unittest for pushing restart logic into the browser. Original patch for the restart logic was checked in outside this CL in https://codereview.chromium.org/2219593003/ so it could get in earlier, this is just the follow on CL for the unittest. BUG=chromium:628022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unittest for pushing restart logic into the browser. Original patch for the restart logic was checked in outside this CL in https://codereview.chromium.org/2219593003/ so it could get in earlier, this is just the follow on CL for the unittest. BUG=chromium:628022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Message was sent while issue was closed.
Committed patchset #7 (id:110001)
Message was sent while issue was closed.
Description was changed from ========== Unittest for pushing restart logic into the browser. Original patch for the restart logic was checked in outside this CL in https://codereview.chromium.org/2219593003/ so it could get in earlier, this is just the follow on CL for the unittest. BUG=chromium:628022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Unittest for pushing restart logic into the browser. Original patch for the restart logic was checked in outside this CL in https://codereview.chromium.org/2219593003/ so it could get in earlier, this is just the follow on CL for the unittest. BUG=chromium:628022 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/6e61cce3dc6538310ef292be09b16593f3b18c6e Cr-Commit-Position: refs/heads/master@{#410454} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/6e61cce3dc6538310ef292be09b16593f3b18c6e Cr-Commit-Position: refs/heads/master@{#410454} |
