New issue
Advanced search Search tips

Issue 901923 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 20
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Task
Proj-VR
Proj-XR



Sign in to add a comment

Cleanup XR test use of EmbeddedTestServer

Project Member Reported by bsheedy@chromium.org, Nov 5

Issue description

We manually start and stop the EmbeddedTestServer in XR tests when we need it (e.g. for permissions). Instead, we should use something like EmbeddedTestServerRule that will automatically handle start and cleanup, although it would be preferable for it to be lazy unlike EmbeddedTestServerRule since we don't need the server most of the time.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8f4b1704bef97e31836dae3ab4601387a521aaa

commit a8f4b1704bef97e31836dae3ab4601387a521aaa
Author: bsheedy <bsheedy@chromium.org>
Date: Tue Nov 20 01:53:32 2018

Cleanup XR usage of EmbeddedTestServer

Cleans up manual usage of EmbeddedTestServer in XR tests, which was both
adding a lot of duplicate code and had the potential to fail to clean up
the server if the test failed, which could then affect other tests.

This is achieved by making EmbeddedTestServerRule lazy and applying it
to all ChromeActivityTestRules, not just WebappActivityTestRule.

Bug:  901923 
Change-Id: I431dac3ce2488a1a5762a1ab1f5ec68aea16c9f5
Reviewed-on: https://chromium-review.googlesource.com/c/1320024
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609556}
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserDialogTest.java
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserNativeUiTest.java
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserTransitionTest.java
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrArSessionTest.java
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrVrInputTest.java
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrVrTabTest.java
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestRule.java
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java
[modify] https://crrev.com/a8f4b1704bef97e31836dae3ab4601387a521aaa/net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerRule.java

Status: Fixed (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b49f5dccde73328df40f843666ab4a0b8608e3a

commit 0b49f5dccde73328df40f843666ab4a0b8608e3a
Author: Maxim Kolosovskiy <kolos@chromium.org>
Date: Tue Nov 20 11:35:42 2018

Revert "Cleanup XR usage of EmbeddedTestServer"

This reverts commit a8f4b1704bef97e31836dae3ab4601387a521aaa.

Reason for revert: Suspect this CL caused a failure of org.chromium.chrome.browser.customtabs.CustomTabActivityTest#testCloseButtonBehaviourWithDynamicModule 

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Android%20CFI/3725

org.junit.ComparisonFailure: expected:<http[s://www.google.com/search?q=cat]> but was:<http[://127.0.0.1:37221/chrome/test/data/android/test.html]>
  	at org.junit.Assert.assertEquals(Assert.java:115)
  	at org.junit.Assert.assertEquals(Assert.java:144)
  	at org.chromium.chrome.browser.customtabs.CustomTabActivityTest.testCloseButtonBehaviourWithDynamicModule(CustomTabActivityTest.java:1195)


Original change's description:
> Cleanup XR usage of EmbeddedTestServer
> 
> Cleans up manual usage of EmbeddedTestServer in XR tests, which was both
> adding a lot of duplicate code and had the potential to fail to clean up
> the server if the test failed, which could then affect other tests.
> 
> This is achieved by making EmbeddedTestServerRule lazy and applying it
> to all ChromeActivityTestRules, not just WebappActivityTestRule.
> 
> Bug:  901923 
> Change-Id: I431dac3ce2488a1a5762a1ab1f5ec68aea16c9f5
> Reviewed-on: https://chromium-review.googlesource.com/c/1320024
> Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
> Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
> Reviewed-by: Paul Jensen <pauljensen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609556}

TBR=nyquist@chromium.org,pauljensen@chromium.org,bsheedy@chromium.org

Change-Id: I676439292a0f90ee353dadef415437415d82bba4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  901923 
Reviewed-on: https://chromium-review.googlesource.com/c/1343265
Reviewed-by: Maxim Kolosovskiy <kolos@chromium.org>
Commit-Queue: Maxim Kolosovskiy <kolos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609665}
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserDialogTest.java
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserNativeUiTest.java
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserTransitionTest.java
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrArSessionTest.java
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrVrInputTest.java
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrVrTabTest.java
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestRule.java
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java
[modify] https://crrev.com/0b49f5dccde73328df40f843666ab4a0b8608e3a/net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerRule.java

Project Member

Comment 4 by bugdroid1@chromium.org, Nov 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a

commit 14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a
Author: bsheedy <bsheedy@chromium.org>
Date: Tue Nov 20 19:16:32 2018

Reland "Cleanup XR usage of EmbeddedTestServer"

This is a reland of a8f4b1704bef97e31836dae3ab4601387a521aaa

No changes from original CL (erroneously reverted)

Original change's description:
> Cleanup XR usage of EmbeddedTestServer
>
> Cleans up manual usage of EmbeddedTestServer in XR tests, which was both
> adding a lot of duplicate code and had the potential to fail to clean up
> the server if the test failed, which could then affect other tests.
>
> This is achieved by making EmbeddedTestServerRule lazy and applying it
> to all ChromeActivityTestRules, not just WebappActivityTestRule.
>
> Bug:  901923 
> Change-Id: I431dac3ce2488a1a5762a1ab1f5ec68aea16c9f5
> Reviewed-on: https://chromium-review.googlesource.com/c/1320024
> Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
> Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
> Reviewed-by: Paul Jensen <pauljensen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#609556}

TBR=nyquist@chromium.org, pauljensen@chromium.org

Bug:  901923 
Change-Id: I1d538672fc6b67d97a47f4bd165c4169e9b3f37c
Reviewed-on: https://chromium-review.googlesource.com/c/1344550
Reviewed-by: Brian Sheedy <bsheedy@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Commit-Queue: Brian Sheedy <bsheedy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609767}
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserDialogTest.java
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserNativeUiTest.java
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/VrBrowserTransitionTest.java
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrArSessionTest.java
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrVrInputTest.java
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/chrome/android/javatests/src/org/chromium/chrome/browser/vr/WebXrVrTabTest.java
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappActivityTestRule.java
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestRule.java
[modify] https://crrev.com/14c4fb3ba66ba75c5f7ca05af6689b723ea5f29a/net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerRule.java

Sign in to add a comment