Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2271543002:
Port context_lost test to new GpuIntegrationTest harness. (Closed)
|
Created:
4 years, 4 months ago by Ken Russell (switch to Gerrit) Modified:
4 years, 4 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, piman+watch_chromium.org, Zhenyao Mo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPort context_lost test to new GpuIntegrationTest harness.
The main trick here is to get hand-written tests to trampoline through
the _RunGpuTest harness, which is where test expectations are
implemented.
This is done by adopting a convention that the "real" test, like
"_GPUProcessCrashesExactlyOnce", is prefixed with an underscore. This
is implemented in self-contained fashion in
ContextLostIntegrationTest.GenerateGpuTests. If necessary, this will
be generalized further into a base class.
The waterfalls will be switched to run the new version of the test in
a follow-on CL.
BUG=352807
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/3aa872b6d1efcc9938ee8e4a205634fc3e9c7918
Cr-Commit-Position: refs/heads/master@{#414279}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Addressed review feedback from eyaich. #
Total comments: 10
Patch Set 3 : More refactorings suggested by eyaich. #
Messages
Total messages: 17 (9 generated)
Description was changed from ========== Port context_lost test to new GpuIntegrationTest harness. The main trick here is to get hand-written tests to trampoline through the _RunGpuTest harness, which is where test expectations are implemented. This is done by adopting a convention that the "real" test, like "_GPUProcessCrashesExactlyOnce", is prefixed with an underscore. This is implemented in self-contained fashion in ContextLostIntegrationTest.GenerateGpuTests. If necessary, this will be generalized further into a base class. The waterfalls will be switched to run the new version of the test in a follow-on CL. BUG=352807 ========== to ========== Port context_lost test to new GpuIntegrationTest harness. The main trick here is to get hand-written tests to trampoline through the _RunGpuTest harness, which is where test expectations are implemented. This is done by adopting a convention that the "real" test, like "_GPUProcessCrashesExactlyOnce", is prefixed with an underscore. This is implemented in self-contained fashion in ContextLostIntegrationTest.GenerateGpuTests. If necessary, this will be generalized further into a base class. The waterfalls will be switched to run the new version of the test in a follow-on CL. BUG=352807 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 ==========
kbr@chromium.org changed reviewers: + eyaich@chromium.org, nednguyen@google.com
Ned, Emily: please review. Thanks. Sorry for this taking so long, but it was difficult to find uninterrupted time to think this through. This port turned out pretty well and I think the subsequent tests will be easier. Compare to context_lost.py in the same directory. Mo: FYI. https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/gpu_integration_test.py (right): https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/gpu_integration_test.py:61: def _RunGpuTest(self, url, test_name, *args): This bug fix was necessary in order to be able to pass the test name correctly as an argument.
The CQ bit was checked by kbr@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
I took a first pass of it, I would like to look closer at context_lost.py tomorrow to determine the difference in execution. https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/context_lost_integration_test.py (right): https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:34: domAutomationController._succeeded = true; Do we no longer need to care about squelching earlier failures? https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:97: cls.CustomizeOptions() Why do we start the browser in the setUpClass of the subclass? Why not do this in the superclass? These same 5 lines exist in the other ported webgl test. https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:201: def _GPUProcessCrashesExactlyOnce(self, test_path): The first five lines of each of these tests is identical, is there a reason not to pull it out to eliminate duplication? https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:207: self._KillGPUProcess(tab, 2, True) The test name indicates "exactly once" and if I am reading this right this tells it to kill the GPU process twice. https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:224: 'window.domAutomationController._finished') What are you testing here?
https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... File content/test/gpu/gpu_tests/context_lost_integration_test.py (right): https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:34: domAutomationController._succeeded = true; On 2016/08/24 01:28:46, eyaich1 wrote: > Do we no longer need to care about squelching earlier failures? Thanks for catching that. I'm not sure how I managed to copy a wrong version of this harness script. Fixed. https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:97: cls.CustomizeOptions() On 2016/08/24 01:28:46, eyaich1 wrote: > Why do we start the browser in the setUpClass of the subclass? Why not do this > in the superclass? These same 5 lines exist in the other ported webgl test. The superclass doesn't know the static server dir. I could refactor this if desired. I'd slightly prefer to do that in a follow-on CL so I don't touch webgl_conformance_integration_test.py in this CL. https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:201: def _GPUProcessCrashesExactlyOnce(self, test_path): On 2016/08/24 01:28:46, eyaich1 wrote: > The first five lines of each of these tests is identical, is there a reason not > to pull it out to eliminate duplication? No good reason. Thanks. Refactored. https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:207: self._KillGPUProcess(tab, 2, True) On 2016/08/24 01:28:46, eyaich1 wrote: > The test name indicates "exactly once" and if I am reading this right this tells > it to kill the GPU process twice. You're right, it's a poorly named test. Fixed. https://codereview.chromium.org/2271543002/diff/1/content/test/gpu/gpu_tests/... content/test/gpu/gpu_tests/context_lost_integration_test.py:224: 'window.domAutomationController._finished') On 2016/08/24 01:28:46, eyaich1 wrote: > What are you testing here? See src/content/test/data/gpu/webgl.html and the code path for the query arg "WEBGL_lose_context". It cooperatively loses the WebGL context via an extension, and doesn't crash the GPU process. This test just waits for the target page to indicate it's finished. Failure is generally reported as a timeout.
lgtm https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/context_lost_integration_test.py (right): https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:220: tab = self.tab remove this line, done in _NavigateAndWaitForLoad https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:230: 'window.domAutomationController._finished') Can't you replace this entire function with self._NavigateAndWatForLoad https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:233: tab = self.tab remove this line, done in _NavigateAndWaitForLoad https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:242: tab = self.tab remove this line, done in _NavigateAndWaitForLoad https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:247: tab = self.tab remove this line, done in _NavigateAndWaitForLoad
https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... File content/test/gpu/gpu_tests/context_lost_integration_test.py (right): https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:220: tab = self.tab On 2016/08/24 13:28:53, eyaich1 wrote: > remove this line, done in _NavigateAndWaitForLoad Done. https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:230: 'window.domAutomationController._finished') On 2016/08/24 13:28:53, eyaich1 wrote: > Can't you replace this entire function with self._NavigateAndWatForLoad No, because this one waits for _finished rather than _loaded. https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:233: tab = self.tab On 2016/08/24 13:28:53, eyaich1 wrote: > remove this line, done in _NavigateAndWaitForLoad This method calls self.tab.CollectGarbage(), but I made it call it explicitly. https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:242: tab = self.tab On 2016/08/24 13:28:53, eyaich1 wrote: > remove this line, done in _NavigateAndWaitForLoad Done. https://codereview.chromium.org/2271543002/diff/20001/content/test/gpu/gpu_te... content/test/gpu/gpu_tests/context_lost_integration_test.py:247: tab = self.tab On 2016/08/24 13:28:53, eyaich1 wrote: > remove this line, done in _NavigateAndWaitForLoad This one uses "tab" locally in a couple of places, but I moved it down.
The CQ bit was checked by kbr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eyaich@chromium.org Link to the patchset: https://codereview.chromium.org/2271543002/#ps40001 (title: "More refactorings suggested by eyaich.")
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Port context_lost test to new GpuIntegrationTest harness. The main trick here is to get hand-written tests to trampoline through the _RunGpuTest harness, which is where test expectations are implemented. This is done by adopting a convention that the "real" test, like "_GPUProcessCrashesExactlyOnce", is prefixed with an underscore. This is implemented in self-contained fashion in ContextLostIntegrationTest.GenerateGpuTests. If necessary, this will be generalized further into a base class. The waterfalls will be switched to run the new version of the test in a follow-on CL. BUG=352807 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 ========== Port context_lost test to new GpuIntegrationTest harness. The main trick here is to get hand-written tests to trampoline through the _RunGpuTest harness, which is where test expectations are implemented. This is done by adopting a convention that the "real" test, like "_GPUProcessCrashesExactlyOnce", is prefixed with an underscore. This is implemented in self-contained fashion in ContextLostIntegrationTest.GenerateGpuTests. If necessary, this will be generalized further into a base class. The waterfalls will be switched to run the new version of the test in a follow-on CL. BUG=352807 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/3aa872b6d1efcc9938ee8e4a205634fc3e9c7918 Cr-Commit-Position: refs/heads/master@{#414279} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/3aa872b6d1efcc9938ee8e4a205634fc3e9c7918 Cr-Commit-Position: refs/heads/master@{#414279} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
