Skip to content

Commit a6e7608

Browse files
Fix issue with forwarding arguments with space
The Android Platform has a [very naive options parsing implemented within the instrumentation code](https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/cmds/am/src/com/android/commands/am/Am.java;l=160-204;drc=61197364367c9e404c7da6900658f1b16c42d0da;bpv=0;bpt=0?q=am.java&ss=android%2Fplatform%2Fsuperproject%2Fmain) where it assumes that arguments delimited by strings live in their own index (e.g. "am A B" is translated to 3 different strings of "am", "A", "B"). This is clearly illustrated by the usage of `opt.equals("..")` where spaces break that assumption. However, the bigger issue here is the error-reporting for this is horrible because it's being reported via `System.err` which the shell executor will give back to the Orchestrator, which puts it in a file that is not used at all! This behaviour should be changed and `stderr` must be printed out to the logcat. However, that's a separate issue The previous trial to fix this have tried to throw an exception whenever white space was found. However, it seems that Kotlin allows test methods with space in them. In order to avoid falling into the same mistake, I did the following: 1. I avoided throwing an exception. This will allow unexpected corner-cases to keep working. However, if they are destined to fail then debugging will be a headache. 2. Only data that is ingested through `ORCHESTRATOR_FORWARDED_INSTRUMENTATION_ARGS` is processed. This is because keys can only exist in the value of that key, which is what can cause problems. Other sources of data don't need processing as they're not keys but values which the option parser can deal with. PiperOrigin-RevId: 759194585
1 parent c711332 commit a6e7608

File tree

4 files changed

+48
-2
lines changed

4 files changed

+48
-2
lines changed

runner/android_test_orchestrator/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@
66

77
**Bug Fixes**
88

9+
* Fix a bug where the instrumentation test application would not startup if the
10+
arguments passed to `ORCHESTRATOR_FORWARDED_INSTRUMENTATION_ARGS` contains
11+
spaces.
12+
913
**New Features**
1014

1115
**Breaking Changes**

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

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import java.io.InputStream;
3434
import java.io.OutputStream;
3535
import java.util.ArrayList;
36+
import java.util.Collections;
3637
import java.util.List;
3738

3839
/** Runnable to run a single am instrument command to execute a single test. */
@@ -187,14 +188,26 @@ private Bundle getTargetInstrumentationArguments() {
187188

188189
/**
189190
* Instrumentation params are delimited by comma, each param is stripped from leading and trailing
190-
* whitespace.
191+
* whitespace. *
192+
*
193+
* <p>The order of the params are critical to the correctness here as we split up params that have
194+
* whitespace (eg: key value) into two different params `key` and `value` which means that those
195+
* two different params must be next to each other the entire time.
191196
*/
192197
private List<String> getInstrumentationParamsAndRemoveBundleArgs(Bundle arguments) {
193198
List<String> cleanedParams = new ArrayList<>();
194199
String forwardedArgs = arguments.getString(ORCHESTRATOR_FORWARDED_INSTRUMENTATION_ARGS);
195200
if (forwardedArgs != null) {
196201
for (String param : forwardedArgs.split(",")) {
197-
cleanedParams.add(param.strip());
202+
// The instrumentation code within the Android Platform was not designed to deal
203+
// with whitespace in the arguments. The options parsing logic:
204+
// https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/cmds/am/src/com/android/commands/am/Am.java;l=160-204;drc=61197364367c9e404c7da6900658f1b16c42d0da;bpv=0;bpt=0?q=am.java&ss=android%2Fplatform%2Fsuperproject%2Fmain
205+
// uses `opt.equal("--key")` which assumes that each individual key will live
206+
// in its separate string. However, if we start sending strings like "--key value" then
207+
// options parsing will fail. The problem here is that this termination is very subtle.
208+
// as the instrumentation does not report to logcat, but to System.err which can sometimes
209+
// buffer the error and silently drop it on process exit.
210+
Collections.addAll(cleanedParams, param.strip().split(" "));
198211
}
199212
arguments.remove(ORCHESTRATOR_FORWARDED_INSTRUMENTATION_ARGS);
200213
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ axt_android_local_test(
5555
"//ext/junit",
5656
"//runner/android_test_orchestrator",
5757
"@maven//:com_google_guava_guava",
58+
"@maven//:com_google_truth_truth",
5859
"@maven//:org_hamcrest_hamcrest_core",
5960
"@maven//:org_hamcrest_hamcrest_library",
6061
],

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
package androidx.test.orchestrator;
1717

1818
import static androidx.test.core.app.ApplicationProvider.getApplicationContext;
19+
import static com.google.common.truth.Truth.assertThat;
1920
import static org.hamcrest.MatcherAssert.assertThat;
2021
import static org.hamcrest.Matchers.containsString;
2122
import static org.hamcrest.Matchers.endsWith;
@@ -202,6 +203,33 @@ public void testRun_buildsParams_givenInstrumentationParamsAreHandledCorrectlySi
202203
assertContainsRunnerArgs(runnable.params, "--no-hidden-api-checks");
203204
}
204205

206+
@Test
207+
public void testRun_buildsParams_givenInstrumentationParamsAreSpaceDelimited() {
208+
arguments.putString("orchestratorInstrumentationArgs", "--abi x86_64");
209+
FakeListener listener = new FakeListener();
210+
FakeTestRunnable runnable =
211+
new FakeTestRunnable(context, "secret", arguments, outputStream, listener, null, true);
212+
runnable.run();
213+
assertThat(runnable.params).containsAtLeast("--abi", "x86_64").inOrder();
214+
}
215+
216+
@Test
217+
public void testRun_buildsParams_allowTestNamesWithSpace() {
218+
FakeListener listener = new FakeListener();
219+
FakeTestRunnable runnable =
220+
new FakeTestRunnable(
221+
context,
222+
"secret",
223+
arguments,
224+
outputStream,
225+
listener,
226+
"com.google.android.example.MyClass#methodNameWith Space",
227+
true);
228+
runnable.run();
229+
assertContainsRunnerArgs(
230+
runnable.params, "-e class com.google.android.example.MyClass#methodNameWith Space");
231+
}
232+
205233
@Test
206234
public void testRun_buildsParams_givenInstrumentationParamsAreHandledCorrectlyMultipleParam() {
207235
arguments.putString("orchestratorInstrumentationArgs", "--A,--B,--C,--key value");

0 commit comments

Comments
 (0)