Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2883273006:
Better generalize VR test framework (Closed)
DescriptionBetter generalize VR test framework
Makes the following changes to the VR Java/JavaScript test framework:
- Adds a loadUrlAndAwaitInitialization function that waits until JavaScript
is in a state that's ready to test (page fully loaded, getVRDisplays
promise resolved, etc.).
- Remove the "VR Display found" assertions from tests that don't specifically
care about whether a display was found or not. Previously, they were being
used as a way to ensure that the getVRDisplays promise had resolved before
trying to interact with WebVR, but that's now handled by
loadUrlAndAwaitInitialization. Since the tests will fail anyways if a VR
display is not found, remove the redundant checks.
- Make VrTestRule extend ChromeTabbedActivityTestRule to reduce repeated setup code.
BUG=723806
Review-Url: https://codereview.chromium.org/2883273006
Cr-Commit-Position: refs/heads/master@{#475654}
Committed: https://chromium.googlesource.com/chromium/src/+/656a1d16a3e4acabb8926505879dc426cf8fff62
Patch Set 1 #
Total comments: 9
Patch Set 2 : Make VrTestRule extend ChromeTabbedActivityTestRule #
Total comments: 12
Patch Set 3 : Address nits #Messages
Total messages: 22 (8 generated)
bsheedy@chromium.org changed reviewers: + mthiesse@chromium.org, tiborg@chromium.org
+mthiesse for OWNERS +tiborg for additional feedback since you've had experience using the framework - was there anything else that you feel should be included in this? https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:107: mVrTestRule.pollJavaScriptBoolean( Alternatively, can use enterPresentationAndWait if adding a vrdisplaypresentchange listener that calls finishJavaScriptStep to the WebVR page is fine. https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:85: ChromeTabbedActivityTestRule rule) throws InterruptedException { I'm on the fence about the way this is implemented. In the short term, it would definitely be better to make VrTestRule extend ChromeTabbedActivityTestRule - that way, we don't need to pass the ChromeTabbedActivityTestRule in for this, and we can reduce repeated test initialization code by moving it out of the individual test classes and into VrTestRule's apply method. In general, cleaner and more scalable code. However, the existing rules are going to be broken up into multiple smaller rules once all tests have been ported to JUnit4. Depending on how this is done, it might not make sense to extend an existing rule anymore. So the question is whether we do the better approach now and maybe revert it later, or keep as-is for now and maybe switch to a better approach whenever the test rules are more finalized.
https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:107: mVrTestRule.pollJavaScriptBoolean( On 2017/05/17 22:40:58, bsheedy wrote: > Alternatively, can use enterPresentationAndWait if adding a > vrdisplaypresentchange listener that calls finishJavaScriptStep to the WebVR > page is fine. Either is fine with me. https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:85: ChromeTabbedActivityTestRule rule) throws InterruptedException { On 2017/05/17 22:40:59, bsheedy wrote: > I'm on the fence about the way this is implemented. > > In the short term, it would definitely be better to make VrTestRule extend > ChromeTabbedActivityTestRule - that way, we don't need to pass the > ChromeTabbedActivityTestRule in for this, and we can reduce repeated test > initialization code by moving it out of the individual test classes and into > VrTestRule's apply method. In general, cleaner and more scalable code. > > However, the existing rules are going to be broken up into multiple smaller > rules once all tests have been ported to JUnit4. Depending on how this is done, > it might not make sense to extend an existing rule anymore. > > So the question is whether we do the better approach now and maybe revert it > later, or keep as-is for now and maybe switch to a better approach whenever the > test rules are more finalized. Do you know when this split up will be done? Generally, I think we should make it work best with the way test rules are implemented at the moment unless you know for sure that the refactoring will be done very soon.
https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:85: ChromeTabbedActivityTestRule rule) throws InterruptedException { On 2017/05/18 14:23:32, tiborg wrote: > On 2017/05/17 22:40:59, bsheedy wrote: > > I'm on the fence about the way this is implemented. > > > > In the short term, it would definitely be better to make VrTestRule extend > > ChromeTabbedActivityTestRule - that way, we don't need to pass the > > ChromeTabbedActivityTestRule in for this, and we can reduce repeated test > > initialization code by moving it out of the individual test classes and into > > VrTestRule's apply method. In general, cleaner and more scalable code. > > > > However, the existing rules are going to be broken up into multiple smaller > > rules once all tests have been ported to JUnit4. Depending on how this is > done, > > it might not make sense to extend an existing rule anymore. > > > > So the question is whether we do the better approach now and maybe revert it > > later, or keep as-is for now and maybe switch to a better approach whenever > the > > test rules are more finalized. > > Do you know when this split up will be done? Generally, I think we should make > it work best with the way test rules are implemented at the moment unless you > know for sure that the refactoring will be done very soon. No definite timeline, but the switch over to JUnit4 is still in progress, and it'll probably take at least a couple weeks after that to decide the best way to split the rules up and actually implement it. I would guess we'd have at least a month before the rules get broken up.
https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:85: ChromeTabbedActivityTestRule rule) throws InterruptedException { On 2017/05/18 16:50:12, bsheedy wrote: > On 2017/05/18 14:23:32, tiborg wrote: > > On 2017/05/17 22:40:59, bsheedy wrote: > > > I'm on the fence about the way this is implemented. > > > > > > In the short term, it would definitely be better to make VrTestRule extend > > > ChromeTabbedActivityTestRule - that way, we don't need to pass the > > > ChromeTabbedActivityTestRule in for this, and we can reduce repeated test > > > initialization code by moving it out of the individual test classes and into > > > VrTestRule's apply method. In general, cleaner and more scalable code. > > > > > > However, the existing rules are going to be broken up into multiple smaller > > > rules once all tests have been ported to JUnit4. Depending on how this is > > done, > > > it might not make sense to extend an existing rule anymore. > > > > > > So the question is whether we do the better approach now and maybe revert it > > > later, or keep as-is for now and maybe switch to a better approach whenever > > the > > > test rules are more finalized. > > > > Do you know when this split up will be done? Generally, I think we should make > > it work best with the way test rules are implemented at the moment unless you > > know for sure that the refactoring will be done very soon. > > No definite timeline, but the switch over to JUnit4 is still in progress, and > it'll probably take at least a couple weeks after that to decide the best way to > split the rules up and actually implement it. I would guess we'd have at least a > month before the rules get broken up. In this case I would take the approach that is better with how the rules are implemented at the moment.
https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:85: ChromeTabbedActivityTestRule rule) throws InterruptedException { On 2017/05/25 01:12:23, tiborg wrote: > On 2017/05/18 16:50:12, bsheedy wrote: > > On 2017/05/18 14:23:32, tiborg wrote: > > > On 2017/05/17 22:40:59, bsheedy wrote: > > > > I'm on the fence about the way this is implemented. > > > > > > > > In the short term, it would definitely be better to make VrTestRule extend > > > > ChromeTabbedActivityTestRule - that way, we don't need to pass the > > > > ChromeTabbedActivityTestRule in for this, and we can reduce repeated test > > > > initialization code by moving it out of the individual test classes and > into > > > > VrTestRule's apply method. In general, cleaner and more scalable code. > > > > > > > > However, the existing rules are going to be broken up into multiple > smaller > > > > rules once all tests have been ported to JUnit4. Depending on how this is > > > done, > > > > it might not make sense to extend an existing rule anymore. > > > > > > > > So the question is whether we do the better approach now and maybe revert > it > > > > later, or keep as-is for now and maybe switch to a better approach > whenever > > > the > > > > test rules are more finalized. > > > > > > Do you know when this split up will be done? Generally, I think we should > make > > > it work best with the way test rules are implemented at the moment unless > you > > > know for sure that the refactoring will be done very soon. > > > > No definite timeline, but the switch over to JUnit4 is still in progress, and > > it'll probably take at least a couple weeks after that to decide the best way > to > > split the rules up and actually implement it. I would guess we'd have at least > a > > month before the rules get broken up. > > In this case I would take the approach that is better with how the rules are > implemented at the moment. Acknowledged. I actually already made the change to do this, but I want to wait for the feedback infobar CL to re-land before I submit so I can get the tests for it switched over, too. I'll need to wait for Michael to be back next week for the LGTM anyways.
Description was changed from
==========
Better generalize VR test framework
Makes the following changes to the VR Java/JavaScript test framework:
- Adds a loadUrlAndAwaitInitialization function that waits until JavaScript
is in a state that's ready to test (page fully loaded, getVRDisplays
promise resolved, etc.).
- Remove the "VR Display found" assertions from tests that don't specifically
care about whether a display was found or not. Previously, they were being
used as a way to ensure that the getVRDisplays promise had resolved before
trying to interact with WebVR, but that's now handled by
loadUrlAndAwaitInitialization. Since the tests will fail anyways if a VR
display is not found, remove the redundant checks.
BUG=723806
==========
to
==========
Better generalize VR test framework
Makes the following changes to the VR Java/JavaScript test framework:
- Adds a loadUrlAndAwaitInitialization function that waits until JavaScript
is in a state that's ready to test (page fully loaded, getVRDisplays
promise resolved, etc.).
- Remove the "VR Display found" assertions from tests that don't specifically
care about whether a display was found or not. Previously, they were being
used as a way to ensure that the getVRDisplays promise had resolved before
trying to interact with WebVR, but that's now handled by
loadUrlAndAwaitInitialization. Since the tests will fail anyways if a VR
display is not found, remove the redundant checks.
- Make VrTestRule extend ChromeTabbedActivityTestRule to reduce repeated setup
code.
BUG=723806
==========
PTAL https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:107: mVrTestRule.pollJavaScriptBoolean( On 2017/05/18 14:23:32, tiborg wrote: > On 2017/05/17 22:40:58, bsheedy wrote: > > Alternatively, can use enterPresentationAndWait if adding a > > vrdisplaypresentchange listener that calls finishJavaScriptStep to the WebVR > > page is fine. > > Either is fine with me. Leaving as-is for now. https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java (right): https://codereview.chromium.org/2883273006/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrTestRule.java:85: ChromeTabbedActivityTestRule rule) throws InterruptedException { On 2017/05/25 01:18:08, bsheedy wrote: > On 2017/05/25 01:12:23, tiborg wrote: > > On 2017/05/18 16:50:12, bsheedy wrote: > > > On 2017/05/18 14:23:32, tiborg wrote: > > > > On 2017/05/17 22:40:59, bsheedy wrote: > > > > > I'm on the fence about the way this is implemented. > > > > > > > > > > In the short term, it would definitely be better to make VrTestRule > extend > > > > > ChromeTabbedActivityTestRule - that way, we don't need to pass the > > > > > ChromeTabbedActivityTestRule in for this, and we can reduce repeated > test > > > > > initialization code by moving it out of the individual test classes and > > into > > > > > VrTestRule's apply method. In general, cleaner and more scalable code. > > > > > > > > > > However, the existing rules are going to be broken up into multiple > > smaller > > > > > rules once all tests have been ported to JUnit4. Depending on how this > is > > > > done, > > > > > it might not make sense to extend an existing rule anymore. > > > > > > > > > > So the question is whether we do the better approach now and maybe > revert > > it > > > > > later, or keep as-is for now and maybe switch to a better approach > > whenever > > > > the > > > > > test rules are more finalized. > > > > > > > > Do you know when this split up will be done? Generally, I think we should > > make > > > > it work best with the way test rules are implemented at the moment unless > > you > > > > know for sure that the refactoring will be done very soon. > > > > > > No definite timeline, but the switch over to JUnit4 is still in progress, > and > > > it'll probably take at least a couple weeks after that to decide the best > way > > to > > > split the rules up and actually implement it. I would guess we'd have at > least > > a > > > month before the rules get broken up. > > > > In this case I would take the approach that is better with how the rules are > > implemented at the moment. > > Acknowledged. I actually already made the change to do this, but I want to wait > for the feedback infobar CL to re-land before I submit so I can get the tests > for it switched over, too. I'll need to wait for Michael to be back next week > for the LGTM anyways. Done.
https://codereview.chromium.org/2883273006/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2883273006/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:202: @DisabledTest(message = "Broken by https://codereview.chromium.org/2889693005") Should work as of https://codereview.chromium.org/2903573004. https://codereview.chromium.org/2883273006/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:220: @DisabledTest(message = "Broken by https://codereview.chromium.org/2889693005") This too. https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... File chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js (right): https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js:5: // Add additional setup steps to the object from webvr_e2e.js if it exists Nit: end with period. https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js:10: // if it's defined Nit: end with period. https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... File chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js (right): https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:15: // after a page load, as Chromium thinking that the page has finished loading Nit: I think we use the name Chrome to refer to the product.
lgtm % nits (and Tibor's comments) https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... File chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js (right): https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:8: var initializationSteps = {"load": false}; nit: Unnecessary quotes around 'load'.
On 2017/05/30 15:30:46, mthiesse wrote: > lgtm % nits (and Tibor's comments) > > https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... > File chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js > (right): > > https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... > chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:8: var > initializationSteps = {"load": false}; > nit: Unnecessary quotes around 'load'. +1 lgtm % nits
https://codereview.chromium.org/2883273006/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java (right): https://codereview.chromium.org/2883273006/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:202: @DisabledTest(message = "Broken by https://codereview.chromium.org/2889693005") On 2017/05/30 14:41:23, tiborg wrote: > Should work as of https://codereview.chromium.org/2903573004. Done. https://codereview.chromium.org/2883273006/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/VrShellNavigationTest.java:220: @DisabledTest(message = "Broken by https://codereview.chromium.org/2889693005") On 2017/05/30 14:41:23, tiborg wrote: > This too. Done. https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... File chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js (right): https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js:5: // Add additional setup steps to the object from webvr_e2e.js if it exists On 2017/05/30 14:41:23, tiborg wrote: > Nit: end with period. Done. https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js:10: // if it's defined On 2017/05/30 14:41:23, tiborg wrote: > Nit: end with period. Done. https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... File chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js (right): https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:8: var initializationSteps = {"load": false}; On 2017/05/30 15:30:46, mthiesse wrote: > nit: Unnecessary quotes around 'load'. Done. https://codereview.chromium.org/2883273006/diff/20001/chrome/test/data/androi... chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:15: // after a page load, as Chromium thinking that the page has finished loading On 2017/05/30 14:41:23, tiborg wrote: > Nit: I think we use the name Chrome to refer to the product. Done.
The CQ bit was checked by bsheedy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mthiesse@chromium.org, tiborg@chromium.org Link to the patchset: https://codereview.chromium.org/2883273006/#ps40001 (title: "Address nits")
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by bsheedy@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496173330389770,
"parent_rev": "fcd78795faa1db43a347464fa235ead2a186636b", "commit_rev":
"656a1d16a3e4acabb8926505879dc426cf8fff62"}
Message was sent while issue was closed.
Description was changed from
==========
Better generalize VR test framework
Makes the following changes to the VR Java/JavaScript test framework:
- Adds a loadUrlAndAwaitInitialization function that waits until JavaScript
is in a state that's ready to test (page fully loaded, getVRDisplays
promise resolved, etc.).
- Remove the "VR Display found" assertions from tests that don't specifically
care about whether a display was found or not. Previously, they were being
used as a way to ensure that the getVRDisplays promise had resolved before
trying to interact with WebVR, but that's now handled by
loadUrlAndAwaitInitialization. Since the tests will fail anyways if a VR
display is not found, remove the redundant checks.
- Make VrTestRule extend ChromeTabbedActivityTestRule to reduce repeated setup
code.
BUG=723806
==========
to
==========
Better generalize VR test framework
Makes the following changes to the VR Java/JavaScript test framework:
- Adds a loadUrlAndAwaitInitialization function that waits until JavaScript
is in a state that's ready to test (page fully loaded, getVRDisplays
promise resolved, etc.).
- Remove the "VR Display found" assertions from tests that don't specifically
care about whether a display was found or not. Previously, they were being
used as a way to ensure that the getVRDisplays promise had resolved before
trying to interact with WebVR, but that's now handled by
loadUrlAndAwaitInitialization. Since the tests will fail anyways if a VR
display is not found, remove the redundant checks.
- Make VrTestRule extend ChromeTabbedActivityTestRule to reduce repeated setup
code.
BUG=723806
Review-Url: https://codereview.chromium.org/2883273006
Cr-Commit-Position: refs/heads/master@{#475654}
Committed:
https://chromium.googlesource.com/chromium/src/+/656a1d16a3e4acabb8926505879d...
==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/656a1d16a3e4acabb8926505879d... |
