Enable ChildProcessLauncherTest#testPendingSpawnQueue()

content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java

Index: content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
diff --git a/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java b/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
index eff9d4f28e728995b04de669da27eff4143c146a..d2b9ed2062ea44960b0ffde5ce4bcd8eabebee3c 100644
--- a/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
+++ b/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java
@@ -9,9 +9,9 @@ import android.os.RemoteException;
 import android.test.InstrumentationTestCase;
 import android.test.suitebuilder.annotation.MediumTest;
 
+import org.chromium.base.BaseSwitches;
 import org.chromium.base.library_loader.LibraryLoader;
 import org.chromium.base.library_loader.LibraryProcessType;
-import org.chromium.base.test.util.DisabledTest;
 import org.chromium.base.test.util.Feature;
 import org.chromium.content.browser.test.util.Criteria;
 import org.chromium.content.browser.test.util.CriteriaHelper;
@@ -20,6 +20,12 @@ import org.chromium.content.browser.test.util.CriteriaHelper;
  * Instrumentation tests for ChildProcessLauncher.
  */
 public class ChildProcessLauncherTest extends InstrumentationTestCase {
+    // Pseudo command line arguments to instruct the child process to wait until being killed.
+    // Allowing the process to continue would lead to a crash when attempting to initialize IPC
+    // channels that are not being set up in this test.

[Yaron, 2016/01/25 19:12:59]

so the idea is to just force the renderer to block until we're satisfied? Seems
viable but could break things if we ever ran multiple tests in one invocation.
:)

[pasko, 2016/01/25 19:38:04]

Thank you for fast review!!

On 2016/01/25 19:12:59, Yaron wrote:
Yes.
Do you mean that a child service leaked from one test will confuse various
connectedServicesCountForTesting() checks of another test?

I assumed that we restart the browser process between tests, but I am not sure
we are actually doing that. If not, this may be a reason for another flake.

If there is a way to kill all child processes after a test, I'd definitely love
to do that, otherwise suspicious that we will need to mock out process creation
entirely.

Anyway, I think this CL is a step towards less flake. WDYT?

[Yaron, 2016/01/25 20:43:50]

Yep.
No, you're right, we do. There are ways to batch tests so they don't have to go
through a full restart but we don't employ them. I would love to it cause it
would reduce cycle time immensely but it's hard in our current environment.

+    private static final String[] sProcessWaitArguments = {
+        "_", "--" + BaseSwitches.RENDERER_WAIT_FOR_JAVA_DEBUGGER };
+
     /**
      *  Tests cleanup for a connection that fails to connect in the first place.
      */
@@ -109,8 +115,7 @@ public class ChildProcessLauncherTest extends InstrumentationTestCase {
         assertEquals(1, ChildProcessLauncher.allocatedConnectionsCountForTesting(appContext));
 
         // Initiate the connection setup.
-        ChildProcessLauncher.triggerConnectionSetup(connection, new String[0], 1,
-                new FileDescriptorInfo[0], ChildProcessLauncher.CALLBACK_FOR_RENDERER_PROCESS, 0);
+        triggerConnectionSetup(connection);
 
         // Verify that the connection completes the setup.
         CriteriaHelper.pollForCriteria(new Criteria(
@@ -157,12 +162,8 @@ public class ChildProcessLauncherTest extends InstrumentationTestCase {
     /**
      * Tests spawning a pending process from queue.
      */
-    /*
     @MediumTest
     @Feature({"ProcessManagement"})
-    crbug.com/483089
-    */
-    @DisabledTest
     public void testPendingSpawnQueue() throws InterruptedException, RemoteException {
         final Context appContext = getInstrumentation().getTargetContext();
         assertEquals(0, ChildProcessLauncher.allocatedConnectionsCountForTesting(appContext));
@@ -171,13 +172,13 @@ public class ChildProcessLauncherTest extends InstrumentationTestCase {
         final ChildProcessConnectionImpl connection = startConnection();
         assertEquals(1, ChildProcessLauncher.allocatedConnectionsCountForTesting(appContext));
 
-        // Queue up a a new spawn request.
-        ChildProcessLauncher.enqueuePendingSpawnForTesting(appContext);
+        // Queue up a new spawn request. There is no way to kill the pending connection, leak it
+        // until the browser restart.
+        ChildProcessLauncher.enqueuePendingSpawnForTesting(appContext, sProcessWaitArguments);
         assertEquals(1, ChildProcessLauncher.pendingSpawnsCountForTesting());
 
         // Initiate the connection setup.
-        ChildProcessLauncher.triggerConnectionSetup(connection, new String[0], 1,
-                new FileDescriptorInfo[0], ChildProcessLauncher.CALLBACK_FOR_RENDERER_PROCESS, 0);
+        triggerConnectionSetup(connection);
 
         // Verify that the connection completes the setup.
         CriteriaHelper.pollForCriteria(
@@ -243,6 +244,11 @@ public class ChildProcessLauncherTest extends InstrumentationTestCase {
         return connection;
     }
 
+    private void triggerConnectionSetup(ChildProcessConnectionImpl connection) {
+        ChildProcessLauncher.triggerConnectionSetup(connection, sProcessWaitArguments, 1,
+                new FileDescriptorInfo[0], ChildProcessLauncher.CALLBACK_FOR_RENDERER_PROCESS, 0);
+    }
+
     @Override
     protected void setUp() throws Exception {
         super.setUp();