Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2588703003:
Add WebVR WebVR E2E test capabilities via Android instrumentation (Closed)
DescriptionAdd WebVR WebVR E2E test capabilities via Android instrumentation
This adds support for running WebVR tests that can't be done in layout
tests, such as end-to-end tests using the actual GVR implementation.
The tests consist of two parts: The WebVR code written in Javascript
and the test runner code written in Java.
The Javascript code uses the same testharness.js that layout tests use,
so all of its functionality such as asserts, EventWatcher, and
async_tests are available and work exactly like in layout tests.
The Java code is responsible for loading the initial HTML page and
performing actions that aren't possible through Javascript, such as
performing user gestures or opening new tabs. Additionally, the Java
code is what actually passes or fails the test based on the results
reported by testharness.js.
BUG=679827, 674974
Review-Url: https://codereview.chromium.org/2588703003
Cr-Commit-Position: refs/heads/master@{#452586}
Committed: https://chromium.googlesource.com/chromium/src/+/eb52d480606f792b3f2c0ddfe0569eb67cd13bef
Patch Set 1 #
Total comments: 2
Patch Set 2 : Refactor + remove tests covered by layout tests #Patch Set 3 : Rework way tests are handled #Patch Set 4 : Switch to use testharness.js #Patch Set 5 : WIP #Patch Set 6 : Simplify Javascript code slightly #
Total comments: 22
Patch Set 7 : Address leilei@ comments #Patch Set 8 : Remove window.onload since we want to run that code before then #
Total comments: 6
Patch Set 9 : Address mthiesse@ comments, add additional test #Patch Set 10 : Make functions always take a WebContents instead of assuming the use of the first tab's WebContents #Patch Set 11 : Swap order in assertEquals #
Total comments: 2
Patch Set 12 : Fix copyright header #Messages
Total messages: 28 (13 generated)
Description was changed from ========== Add WebVR Javascript test runner via Android instrumentation. This adds support for running layout test-like Javascript tests via Android instrumentation tests as a temporary workaround for layout tests being broken on L+ and WebVR not working properly in ContentShell. Since WebVR's requestPresent requires a user gesture to succeed and there is not a way to emulate a user gesture from Javascript in instrumentation tests like there is in ContentShell, user gestures are performed from the Java code. This requires synchronizing the Java and Javascript code, hence the wonky test setup with asyncCounter and event firing. BUG= ========== to ========== Add WebVR Javascript test runner via Android instrumentation. This adds support for running layout test-like Javascript tests via Android instrumentation tests as a temporary workaround for layout tests being broken on L+ and WebVR not working properly in ContentShell. These tests are currently not yet included in chrome_public_test_apk, as VR tests in general still require some changes to the bots in order to run properly. Since WebVR's requestPresent requires a user gesture to succeed and there is not a way to emulate a user gesture from Javascript in instrumentation tests like there is in ContentShell, user gestures are performed from the Java code. This requires synchronizing the Java and Javascript code, hence the wonky test setup with asyncCounter and event firing. Whether a test passes or fails depends only on whether the output matches the expectation file for the device/test combination in chrome/test/data/android/webvr_instrumentation. testharness.js is identical to the version found in //third_party/WebKit/LayoutTests/resources. testharnessreport.js is also almost identical to the version found in that directory, except with asyncCounter being incremented to signal the Java code that the test is done. BUG= ==========
Description was changed from ========== Add WebVR Javascript test runner via Android instrumentation. This adds support for running layout test-like Javascript tests via Android instrumentation tests as a temporary workaround for layout tests being broken on L+ and WebVR not working properly in ContentShell. These tests are currently not yet included in chrome_public_test_apk, as VR tests in general still require some changes to the bots in order to run properly. Since WebVR's requestPresent requires a user gesture to succeed and there is not a way to emulate a user gesture from Javascript in instrumentation tests like there is in ContentShell, user gestures are performed from the Java code. This requires synchronizing the Java and Javascript code, hence the wonky test setup with asyncCounter and event firing. Whether a test passes or fails depends only on whether the output matches the expectation file for the device/test combination in chrome/test/data/android/webvr_instrumentation. testharness.js is identical to the version found in //third_party/WebKit/LayoutTests/resources. testharnessreport.js is also almost identical to the version found in that directory, except with asyncCounter being incremented to signal the Java code that the test is done. BUG= ========== to ========== Add WebVR Javascript test runner via Android instrumentation. This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. Has to do some wonky stuff to synchronize the Java and Javascript code, using a counter variable in Javascript to signal when the Java code should run and a stored function pointer to continue Javascript execution. Whether a test passes or fails depends only on whether the output matches the expectation file for the device/test combination in chrome/test/data/android/webvr_instrumentation. Considering changing test pass/fail to depend on actual asserts and using the @Restriction annotation to only run on certain devices instead of using expectation files. testharness.js is identical to the version found in //third_party/WebKit/LayoutTests/resources. testharnessreport.js is also almost identical to the version found in that directory, except with asyncCounter being incremented to signal the Java code that the test is done. BUG= ==========
Description was changed from ========== Add WebVR Javascript test runner via Android instrumentation. This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. Has to do some wonky stuff to synchronize the Java and Javascript code, using a counter variable in Javascript to signal when the Java code should run and a stored function pointer to continue Javascript execution. Whether a test passes or fails depends only on whether the output matches the expectation file for the device/test combination in chrome/test/data/android/webvr_instrumentation. Considering changing test pass/fail to depend on actual asserts and using the @Restriction annotation to only run on certain devices instead of using expectation files. testharness.js is identical to the version found in //third_party/WebKit/LayoutTests/resources. testharnessreport.js is also almost identical to the version found in that directory, except with asyncCounter being incremented to signal the Java code that the test is done. BUG= ========== to ========== Add WebVR Javascript test runner via Android instrumentation. This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. Has to do some wonky stuff to synchronize the Java and Javascript code, using a counter variable in Javascript to signal when the Java code should run and a stored function pointer to continue Javascript execution. Whether a test passes or fails depends only on whether the output matches the expectation file for the device/test combination in chrome/test/data/android/webvr_instrumentation. Considering changing test pass/fail to depend on actual asserts and using the @Restriction annotation to only run on certain devices instead of using expectation files. testharness.js is identical to the version found in //third_party/WebKit/LayoutTests/resources. testharnessreport.js is also almost identical to the version found in that directory, except with asyncCounter being incremented to signal the Java code that the test is done. BUG=679827 ==========
Description was changed from ========== Add WebVR Javascript test runner via Android instrumentation. This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. Has to do some wonky stuff to synchronize the Java and Javascript code, using a counter variable in Javascript to signal when the Java code should run and a stored function pointer to continue Javascript execution. Whether a test passes or fails depends only on whether the output matches the expectation file for the device/test combination in chrome/test/data/android/webvr_instrumentation. Considering changing test pass/fail to depend on actual asserts and using the @Restriction annotation to only run on certain devices instead of using expectation files. testharness.js is identical to the version found in //third_party/WebKit/LayoutTests/resources. testharnessreport.js is also almost identical to the version found in that directory, except with asyncCounter being incremented to signal the Java code that the test is done. BUG=679827 ========== to ========== Add WebVR WebVR E2E test capabilities via Android instrumentation This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. The tests consist of two parts: The WebVR code written in Javascript and the test runner code written in Java. The Javascript code uses the same testharness.js that layout tests use, so all of its functionality such as asserts, EventWatcher, and async_tests are available and work exactly like in layout tests. The Java code is responsible for loading the initial HTML page and performing actions that aren't possible through Javascript, such as performing user gestures or opening new tabs. BUG=679827 ==========
Description was changed from ========== Add WebVR WebVR E2E test capabilities via Android instrumentation This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. The tests consist of two parts: The WebVR code written in Javascript and the test runner code written in Java. The Javascript code uses the same testharness.js that layout tests use, so all of its functionality such as asserts, EventWatcher, and async_tests are available and work exactly like in layout tests. The Java code is responsible for loading the initial HTML page and performing actions that aren't possible through Javascript, such as performing user gestures or opening new tabs. BUG=679827 ========== to ========== Add WebVR WebVR E2E test capabilities via Android instrumentation This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. The tests consist of two parts: The WebVR code written in Javascript and the test runner code written in Java. The Javascript code uses the same testharness.js that layout tests use, so all of its functionality such as asserts, EventWatcher, and async_tests are available and work exactly like in layout tests. The Java code is responsible for loading the initial HTML page and performing actions that aren't possible through Javascript, such as performing user gestures or opening new tabs. Additionally, the Java code is what actually passes or fails the test based on the results reported by testharness.js. BUG=679827 ==========
leilei@chromium.org changed reviewers: + leilei@chromium.org
https://codereview.chromium.org/2588703003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2588703003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:102: Log.e(TAG, "Unable to check if VRDisplay found"); Nit, you can use Log.e to log the stack trace as well, Log.e(TAG, errormessage, e) https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/... https://codereview.chromium.org/2588703003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:183: e.printStackTrace(); ditto. https://codereview.chromium.org/2588703003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:15: import org.chromium.chrome.browser.media.RouterTestUtils; It is a little weird to util functions for other tests, it is better to refactor it and move the common code to a common place, probably org.chromium.content.browser.test.util.ClickUtils or something similar. https://codereview.chromium.org/2588703003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:204: pollJavascriptBoolean(POLL_TIMEOUT_LONG, "javascriptDone", webContents); I think we need to check the return value from pollJavascriptBoolean and mark the test failed if the return value if false. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/html/test_nfc_fires_onvrdisplayactivate.html (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/test_nfc_fires_onvrdisplayactivate.html:8: <style> You can put those CSS code in a css file and include it here to reduce the duplicated code. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/test_nfc_fires_onvrdisplayactivate.html:33: <script src="../resources/webvr_boilerplate.js"></script> I think you can remove this file in this test case, so you don't need to remove the event listener at the beginning to reduce the confusion. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/html/test_pose_data_unfocused_tab.html (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/test_pose_data_unfocused_tab.html:38: function onAnimationFrame1() { Nit: no need to add 1 at end of function name. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/test_pose_data_unfocused_tab.html:55: t.step( () => { Nit: you can call t.step_func_done which does t.step and t.done. http://testthewebforward.org/docs/testharness-library.html#asynchronous-tests https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/html/webvr_instrumentation_base.html (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/webvr_instrumentation_base.html:5: --> This file is unused. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js:30: onResize(); How about adding those code in window.onload instead of spreading in the whole file? https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js:39: asyncCounter++; Do you still need asyncCounter? https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:33: ". "; How about setting resultString to JSON.stringify(tests)? Since you already have testPassed variable to indicate if tests pass or not. The resultString can include all the information for debug.
https://codereview.chromium.org/2588703003/diff/100001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:15: import org.chromium.chrome.browser.media.RouterTestUtils; On 2017/02/16 22:04:49, Lei Lei wrote: > It is a little weird to util functions for other tests, it is better to refactor > it and move the common code to a common place, probably > org.chromium.content.browser.test.util.ClickUtils or something similar. Added a TODO to refactor this since that should be its own CL. https://codereview.chromium.org/2588703003/diff/100001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:204: pollJavascriptBoolean(POLL_TIMEOUT_LONG, "javascriptDone", webContents); On 2017/02/16 22:04:49, Lei Lei wrote: > I think we need to check the return value from pollJavascriptBoolean and mark > the test failed if the return value if false. Done, and added to the check in simNfcScanAndWait as well. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/html/test_nfc_fires_onvrdisplayactivate.html (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/test_nfc_fires_onvrdisplayactivate.html:8: <style> On 2017/02/16 22:04:49, Lei Lei wrote: > You can put those CSS code in a css file and include it here to reduce the > duplicated code. Done. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/test_nfc_fires_onvrdisplayactivate.html:33: <script src="../resources/webvr_boilerplate.js"></script> On 2017/02/16 22:04:49, Lei Lei wrote: > I think you can remove this file in this test case, so you don't need to remove > the event listener at the beginning to reduce the confusion. Done. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/html/test_pose_data_unfocused_tab.html (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/test_pose_data_unfocused_tab.html:38: function onAnimationFrame1() { On 2017/02/16 22:04:49, Lei Lei wrote: > Nit: no need to add 1 at end of function name. Done. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/test_pose_data_unfocused_tab.html:55: t.step( () => { On 2017/02/16 22:04:50, Lei Lei wrote: > Nit: you can call t.step_func_done which does t.step and t.done. > http://testthewebforward.org/docs/testharness-library.html#asynchronous-tests Done. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/html/webvr_instrumentation_base.html (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/html/webvr_instrumentation_base.html:5: --> On 2017/02/16 22:04:50, Lei Lei wrote: > This file is unused. Done. It was meant to be a template that people could copy, but with the amount of code removed by switching to a CSS file, not really helpful. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js:30: onResize(); On 2017/02/16 22:04:50, Lei Lei wrote: > How about adding those code in window.onload instead of spreading in the whole > file? Done. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/resources/webvr_boilerplate.js:39: asyncCounter++; On 2017/02/16 22:04:50, Lei Lei wrote: > Do you still need asyncCounter? No, done. https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:33: ". "; On 2017/02/16 22:04:50, Lei Lei wrote: > How about setting resultString to JSON.stringify(tests)? Since you already have > testPassed variable to indicate if tests pass or not. The resultString can > include all the information for debug. I feel like that ends up being too verbose. For a single test, JSON.stringify(tests) results in: [{"name":"NFC scan fires the vrdisplayactivate event","phase":3,"status":0,"timeout_id":null,"index":1,"properties":{},"timeout_length":null,"message":null,"stack":null,"steps":[],"cleanup_callbacks":[],"_structured_clone":{"name":"NFC scan fires the vrdisplayactivate event","properties":{},"PASS":0,"FAIL":1,"TIMEOUT":2,"NOTRUN":3,"status":0,"message":null,"stack":null,"index":1}}] compared to the current resultString: PASS NFC scan fires the vrdisplayactivate event. Since resultString is used as the description for the Java assert at the very end of a test, it'll get printed out anytime testharness.js reports a failure, so I'd prefer to keep it from being too long.
https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:33: ". "; On 2017/02/16 23:30:14, bsheedy wrote: > On 2017/02/16 22:04:50, Lei Lei wrote: > > How about setting resultString to JSON.stringify(tests)? Since you already > have > > testPassed variable to indicate if tests pass or not. The resultString can > > include all the information for debug. > > I feel like that ends up being too verbose. For a single test, > JSON.stringify(tests) results in: > > [{"name":"NFC scan fires the vrdisplayactivate > event","phase":3,"status":0,"timeout_id":null,"index":1,"properties":{},"timeout_length":null,"message":null,"stack":null,"steps":[],"cleanup_callbacks":[],"_structured_clone":{"name":"NFC > scan fires the vrdisplayactivate > event","properties":{},"PASS":0,"FAIL":1,"TIMEOUT":2,"NOTRUN":3,"status":0,"message":null,"stack":null,"index":1}}] > > compared to the current resultString: > > PASS NFC scan fires the vrdisplayactivate event. > > Since resultString is used as the description for the Java assert at the very > end of a test, it'll get printed out anytime testharness.js reports a failure, > so I'd prefer to keep it from being too long. It is fine to keep it short, what is the message if the test is failed? Does it include stack?
https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... File chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js (right): https://codereview.chromium.org/2588703003/diff/100001/chrome/test/data/andro... chrome/test/data/android/webvr_instrumentation/resources/webvr_e2e.js:33: ". "; On 2017/02/17 00:18:14, Lei Lei wrote: > On 2017/02/16 23:30:14, bsheedy wrote: > > On 2017/02/16 22:04:50, Lei Lei wrote: > > > How about setting resultString to JSON.stringify(tests)? Since you already > > have > > > testPassed variable to indicate if tests pass or not. The resultString can > > > include all the information for debug. > > > > I feel like that ends up being too verbose. For a single test, > > JSON.stringify(tests) results in: > > > > [{"name":"NFC scan fires the vrdisplayactivate > > > event","phase":3,"status":0,"timeout_id":null,"index":1,"properties":{},"timeout_length":null,"message":null,"stack":null,"steps":[],"cleanup_callbacks":[],"_structured_clone":{"name":"NFC > > scan fires the vrdisplayactivate > > > event","properties":{},"PASS":0,"FAIL":1,"TIMEOUT":2,"NOTRUN":3,"status":0,"message":null,"stack":null,"index":1}}] > > > > compared to the current resultString: > > > > PASS NFC scan fires the vrdisplayactivate event. > > > > Since resultString is used as the description for the Java assert at the very > > end of a test, it'll get printed out anytime testharness.js reports a failure, > > so I'd prefer to keep it from being too long. > > It is fine to keep it short, what is the message if the test is failed? Does it > include stack? An example of a test failing when reaching an assert_unreached("Some description"): FAIL Test pose data in unfocused tab: assert_unreached: Some description Reached unreachable code.
bsheedy@chromium.org changed reviewers: + bajones@chromium.org, bshe@chromium.org, mthiesse@chromium.org, tedchoc@chromium.org
+tedchoc for chrome/android OWNERS +some Chrome VR SWEs for feedback since they'll be some of the people using this for writing tests
Description was changed from ========== Add WebVR WebVR E2E test capabilities via Android instrumentation This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. The tests consist of two parts: The WebVR code written in Javascript and the test runner code written in Java. The Javascript code uses the same testharness.js that layout tests use, so all of its functionality such as asserts, EventWatcher, and async_tests are available and work exactly like in layout tests. The Java code is responsible for loading the initial HTML page and performing actions that aren't possible through Javascript, such as performing user gestures or opening new tabs. Additionally, the Java code is what actually passes or fails the test based on the results reported by testharness.js. BUG=679827 ========== to ========== Add WebVR WebVR E2E test capabilities via Android instrumentation This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. The tests consist of two parts: The WebVR code written in Javascript and the test runner code written in Java. The Javascript code uses the same testharness.js that layout tests use, so all of its functionality such as asserts, EventWatcher, and async_tests are available and work exactly like in layout tests. The Java code is responsible for loading the initial HTML page and performing actions that aren't possible through Javascript, such as performing user gestures or opening new tabs. Additionally, the Java code is what actually passes or fails the test based on the results reported by testharness.js. BUG=679827,674974 ==========
Awesome! https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:98: * the test, which is the case unless code is being run in multiple tabs. This comment doesn't make sense to me. What are you saying is the case? Why provide this helper function at all? It doesn't look like it's necessary. https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:128: assertTrue("Polling Javascript boolean javascriptDone succeeded", I'm worried that this is going to be flaky, even with a 10 second timeout. I don't know what we can do about that though... Are these tests going to be running on bots? Also, is simNfcScan() guaranteed to work all of the time? Does daydream still run their timeout logic with this method, where we need to wait ~5 seconds after losing Nfc to re-scan? https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:165: assertTrue(result, result.equals("Passed")); nit: use assertEquals?
https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:98: * the test, which is the case unless code is being run in multiple tabs. On 2017/02/21 22:58:58, mthiesse wrote: > This comment doesn't make sense to me. What are you saying is the case? > > Why provide this helper function at all? It doesn't look like it's necessary. It's not necessary at all, and in hindsight, isn't particularly helpful either since it only shortens a line in each test by a dozen characters. I'll go ahead and remove this. My original thinking was: Whenever we run any Javascript code, we need to provide the WebContents object for the tab we want to run it in. In most cases, we'll only be running a test in the tab that's automatically opened at the start of the test, whose WebContents is stored in mWebContents. Since it's the most common case, I thought that being able to call vrDisplayFound() in tests instead of vrDisplayFound(mWebContents) might be helpful. In general, is it better to not add functions like this, and instead always make the function take a specific WebContents object? executeStepAndWait still has a version that assumes you want to use mWebContents, and endTest() only assumes that the results will be in the first tab. https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:128: assertTrue("Polling Javascript boolean javascriptDone succeeded", On 2017/02/21 22:58:58, mthiesse wrote: > I'm worried that this is going to be flaky, even with a 10 second timeout. I > don't know what we can do about that though... Are these tests going to be > running on bots? > > Also, is simNfcScan() guaranteed to work all of the time? Does daydream still > run their timeout logic with this method, where we need to wait ~5 seconds after > losing Nfc to re-scan? Yes, the plan is to eventually get these running on bots. I have yet to experience any flakiness with a 10 second timeout, but it's always a possibility. Like you said, there aren't really any better solutions, though. We could always extend the timeout a bit to say, 15 seconds. I would say that if it's taking more than 15 seconds to enter VR after inserting into the headset, then something's wrong with either Chrome or VRCore, so I wouldn't consider failing due to a timeout here a flake. As for whether it is guaranteed to work, I haven't been able to actually test calling simNfcScan(), exiting VR, then calling simNfcScan() again, as the second simNfcScan() causes the null pointer exception from Issue 690625. However, we do get a "No NFC tag given in intent, skipping debounce operation..." in logcat, which comes from https://cs.corp.google.com/piper///depot/google3/java/com/google/vr/vrcore/nf.... If I'm looking at that correctly, then the way simNfcScan() is implemented happens to bypass the timeout. Yay for accidentally implementing it the way we want it? https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:165: assertTrue(result, result.equals("Passed")); On 2017/02/21 22:58:58, mthiesse wrote: > nit: use assertEquals? Done. Thought assertEquals() would just use ==, but looks like it works properly for comparing Strings, as well.
On 2017/02/22 00:26:29, bsheedy wrote: > https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... > File > chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java > (right): > > https://codereview.chromium.org/2588703003/diff/140001/chrome/android/javates... > chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:98: > * the test, which is the case unless code is being run in multiple tabs. > On 2017/02/21 22:58:58, mthiesse wrote: > > This comment doesn't make sense to me. What are you saying is the case? > > > > Why provide this helper function at all? It doesn't look like it's necessary. > > It's not necessary at all, and in hindsight, isn't particularly helpful either > since it only shortens a line in each test by a dozen characters. I'll go ahead > and remove this. > > My original thinking was: Whenever we run any Javascript code, we need to > provide the WebContents object for the tab we want to run it in. In most cases, > we'll only be running a test in the tab that's automatically opened at the start > of the test, whose WebContents is stored in mWebContents. Since it's the most > common case, I thought that being able to call vrDisplayFound() in tests instead > of vrDisplayFound(mWebContents) might be helpful. > > In general, is it better to not add functions like this, and instead always make > the function take a specific WebContents object? executeStepAndWait still has a > version that assumes you want to use mWebContents, and endTest() only assumes Yeah, in general I wouldn't add functions like this unless they're clearly making code more readable. In this case it was probably making the code less readable.
Meaningless lgtm ;) I didn't thoroughly go through the js, I'll leave that for others.
js/html LGTM. I didn't thoroughly go through the Java, I'll leave that for others. ;D
bsheedy@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor for chrome/android OWNERS
chrome/android lgtm https://codereview.chromium.org/2588703003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2588703003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. 2017
https://codereview.chromium.org/2588703003/diff/200001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java (right): https://codereview.chromium.org/2588703003/diff/200001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/vr_shell/WebVrTest.java:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/23 15:57:51, David Trainor wrote: > 2017 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, dtrainor@chromium.org, bajones@chromium.org Link to the patchset: https://codereview.chromium.org/2588703003/#ps220001 (title: "Fix copyright header")
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": 220001, "attempt_start_ts": 1487871898163970,
"parent_rev": "8a9ed5b3b71d3915fb734b99a5e6e0db550d79db", "commit_rev":
"eb52d480606f792b3f2c0ddfe0569eb67cd13bef"}
Message was sent while issue was closed.
Description was changed from ========== Add WebVR WebVR E2E test capabilities via Android instrumentation This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. The tests consist of two parts: The WebVR code written in Javascript and the test runner code written in Java. The Javascript code uses the same testharness.js that layout tests use, so all of its functionality such as asserts, EventWatcher, and async_tests are available and work exactly like in layout tests. The Java code is responsible for loading the initial HTML page and performing actions that aren't possible through Javascript, such as performing user gestures or opening new tabs. Additionally, the Java code is what actually passes or fails the test based on the results reported by testharness.js. BUG=679827,674974 ========== to ========== Add WebVR WebVR E2E test capabilities via Android instrumentation This adds support for running WebVR tests that can't be done in layout tests, such as end-to-end tests using the actual GVR implementation. The tests consist of two parts: The WebVR code written in Javascript and the test runner code written in Java. The Javascript code uses the same testharness.js that layout tests use, so all of its functionality such as asserts, EventWatcher, and async_tests are available and work exactly like in layout tests. The Java code is responsible for loading the initial HTML page and performing actions that aren't possible through Javascript, such as performing user gestures or opening new tabs. Additionally, the Java code is what actually passes or fails the test based on the results reported by testharness.js. BUG=679827,674974 Review-Url: https://codereview.chromium.org/2588703003 Cr-Commit-Position: refs/heads/master@{#452586} Committed: https://chromium.googlesource.com/chromium/src/+/eb52d480606f792b3f2c0ddfe056... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/eb52d480606f792b3f2c0ddfe056... |
