Split by whitespace for commands

This is needed because the ShellExecutor uses ProcessBuilder which is known to have encoding issues as
it converts Java Strings to C-strings (byte arrays) with no regards to encoding. In order to avoid, the
best we can do here is to avoid using characters which can cause encoding issues (eg:whitespaces).

Empty-strings also causes the same behavior for which we will raise an IllegalStateException to protect
from that as debugging the unusual behavior is time-consuming without any idea what's going wrong so
crashing early will make it much easier.

PiperOrigin-RevId: 611160643

runner/android_test_orchestrator/java/androidx/test/orchestrator/TestRunnable.java

@@ -33,6 +33,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
 
 /** Runnable to run a single am instrument command to execute a single test. */
@@ -188,19 +189,32 @@ private Bundle getTargetInstrumentationArguments() {
   /**
    * Instrumentation params are delimited by comma, each param is stripped from leading and trailing
    * whitespace.
+   *
+   * <p>The order of the params are critical to the correctness here as we split up params that have
+   * whitespace (eg: key value) into two different params `key` and `value` which means that those
+   * two different params must be next to each other the entire time.
    */
   private List<String> getInstrumentationParamsAndRemoveBundleArgs(Bundle arguments) {
     List<String> cleanedParams = new ArrayList<>();
     String forwardedArgs = arguments.getString(ORCHESTRATOR_FORWARDED_INSTRUMENTATION_ARGS);
     if (forwardedArgs != null) {
       for (String param : forwardedArgs.split(",")) {
-        cleanedParams.add(param.strip());
+        // ShellExecutor exhibits weird behavior when a param has a whitespace in it.
+        // so we need to split by white-space to remove the spaces.
+        Collections.addAll(cleanedParams, param.strip().split(" "));
       }
       arguments.remove(ORCHESTRATOR_FORWARDED_INSTRUMENTATION_ARGS);
     }
     return cleanedParams;
   }
 
+  /**
+   * This method must maintain the order of the params
+   *
+   * <p>The order of the params are critical to the correctness here as we split up params that have
+   * whitespace (eg: key value) into two different params `key` and `value` which means that those
+   * two different params must be next to each other the entire time.
+   */
   private List<String> buildShellParams(Bundle arguments) throws IOException, ClientNotConnected {
     List<String> params = new ArrayList<>();
     params.add("instrument");
@@ -215,6 +229,12 @@ private List<String> buildShellParams(Bundle arguments) throws IOException, Clie
       params.add(arguments.getString(key));
     }
     params.add(getTargetInstrumentation());
+    for (String param : params) {
+      if (param.isEmpty() || param.contains(" ")) {
+        throw new IllegalStateException(
+            "Params must not contain any white-space to avoid encoding issues.");
+      }
+    }
     return params;
   }
 

runner/android_test_orchestrator/javatests/androidx/test/orchestrator/BUILD

@@ -61,6 +61,7 @@ axt_android_local_test(
         "//ext/junit",
         "//runner/android_test_orchestrator",
         "@maven//:com_google_guava_guava",
+        "@maven//:com_google_truth_truth",
         "@maven//:org_hamcrest_hamcrest_core",
         "@maven//:org_hamcrest_hamcrest_library",
     ],

runner/android_test_orchestrator/javatests/androidx/test/orchestrator/TestRunnableTest.java

@@ -21,11 +21,13 @@
 import static org.hamcrest.Matchers.endsWith;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.startsWith;
+import static org.junit.Assert.assertThrows;
 
 import android.content.Context;
 import android.os.Bundle;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import com.google.common.base.Joiner;
+import com.google.common.truth.Truth;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.InputStream;
@@ -212,6 +214,24 @@ public void testRun_buildsParams_givenInstrumentationParamsAreHandledCorrectlyMu
     assertContainsRunnerArgs(runnable.params, "--A --B --C --key value");
   }
 
+  @Test
+  public void testRun_buildsParams_givenValueWithSpaceParamThrows() {
+    arguments.putString("someArgument", "A B");
+    FakeListener listener = new FakeListener();
+    FakeTestRunnable runnable =
+        new FakeTestRunnable(context, "secret", arguments, outputStream, listener, null, true);
+    assertThrows(IllegalStateException.class, runnable::run);
+  }
+
+  @Test
+  public void testRun_buildsParams_givenEmptyStringParamThrows() {
+    arguments.putString("someArgument", "");
+    FakeListener listener = new FakeListener();
+    FakeTestRunnable runnable =
+        new FakeTestRunnable(context, "secret", arguments, outputStream, listener, null, true);
+    assertThrows(IllegalStateException.class, runnable::run);
+  }
+
   private static void assertContainsRunnerArgs(List<String> params, String... containsArgs) {
     String cmdArgs = Joiner.on(" ").join(params);
     assertThat(cmdArgs, startsWith("instrument -w -r"));
@@ -220,4 +240,14 @@ private static void assertContainsRunnerArgs(List<String> params, String... cont
     }
     assertThat(cmdArgs, endsWith("targetInstrumentation/targetRunner"));
   }
+
+  @Test
+  public void testRun_buildsParams_givenInstrumentationParamsWithSpaceMaintainsOrder() {
+    arguments.putString("orchestratorInstrumentationArgs", "--A,--B,--key value");
+    FakeListener listener = new FakeListener();
+    FakeTestRunnable runnable =
+        new FakeTestRunnable(context, "secret", arguments, outputStream, listener, null, true);
+    runnable.run();
+    Truth.assertThat(runnable.params).containsAtLeast("--A", "--B", "--key", "value").inOrder();
+  }
 }