Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove Orchestrator's single method fast path #2133

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove Orchestrator's single method fast path #2133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
35 changes: 8 additions & 27 deletions ...er/android_test_orchestrator/java/androidx/test/orchestrator/AndroidTestOrchestrator.java
View file
Open in desktop
Expand Up @@ -130,9 +130,6 @@ public final class AndroidTestOrchestrator extends android.app.Instrumentation
private static final String TEST_COLLECTION_FILENAME = "testCollection.txt";
private static final int MAX_FILENAME_LENGTH = 255;

private static final Pattern FULLY_QUALIFIED_CLASS_AND_METHOD =
Pattern.compile("[\\w\\.?]+#\\w+");

private static final List<String> RUNTIME_PERMISSIONS =
Arrays.asList(permission.WRITE_EXTERNAL_STORAGE, permission.READ_EXTERNAL_STORAGE);

Expand Down Expand Up @@ -253,23 +250,14 @@ public void onServiceDisconnected(ComponentName className) {
};

private void collectTests() {
String classArg = arguments.getString(AJUR_CLASS_ARGUMENT);
// If we are given a single, fully qualified test then there's no point in test collection.
// Proceed as if we had done collection and gotten the single argument.
if (isSingleMethodTest(classArg)) {
Log.i(TAG, String.format("Single test parameter %s, skipping test collection", classArg));
callbackLogic.addTest(classArg);
runFinished();
} else {
Log.i(TAG, String.format("Multiple test parameter %s, starting test collection", classArg));
executorService.execute(
TestRunnable.testCollectionRunnable(
getContext(),
getSecret(arguments),
arguments,
getOutputStream(),
AndroidTestOrchestrator.this));
}
Log.i(TAG, "Starting test collection");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change looks reasonable but I'm concerned over the performance impact on the 'normal' case of just running a single, non-parameterized method. Test discovery will require launching and killing the target app process, so for large apps its not cheap.

Notably, some environments use a host-side based test collection mechanism. So with this change those environments might run test discovery twice.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's very fair. I suppose there are a few alternatives:

  • Just accept and ideally document this wart for parameterized tests.
  • Some kind of speculative single-method execution path that would keep the current behaviour in the common case where it really is just 1 test, or report a list of tests back to orchestrator in the parameterized case.

But you may well have some better suggestions, I'm not that familar with the internals of androidx.test. 😅

executorService.execute(
TestRunnable.testCollectionRunnable(
getContext(),
getSecret(arguments),
arguments,
getOutputStream(),
AndroidTestOrchestrator.this));
}

@VisibleForTesting
Expand All @@ -293,13 +281,6 @@ static String makeValidFilename(String testName, int maxFilenameLength) {
return testName + testRunFilenameSuffix;
}

@VisibleForTesting
static boolean isSingleMethodTest(String classArg) {
if (TextUtils.isEmpty(classArg)) {
return false;
}
return FULLY_QUALIFIED_CLASS_AND_METHOD.matcher(classArg).matches();
}

/** Invoked every time the TestRunnable finishes, including after test collection. */
@Override
Expand Down
20 changes: 0 additions & 20 deletions ...d_test_orchestrator/javatests/androidx/test/orchestrator/AndroidTestOrchestratorTest.java
View file
Open in desktop
Expand Up @@ -27,26 +27,6 @@
/** Unit tests for {@link AndroidTestOrchestrator}. */
@RunWith(AndroidJUnit4.class)
public class AndroidTestOrchestratorTest {

@Test
public void testSingleMethodTest() {
assertThat(AndroidTestOrchestrator.isSingleMethodTest("org.example.class#method"), is(true));
assertThat(AndroidTestOrchestrator.isSingleMethodTest("org.example.class"), is(false));
assertThat(
AndroidTestOrchestrator.isSingleMethodTest("org.example.class,org.example.another#method"),
is(false));
assertThat(
AndroidTestOrchestrator.isSingleMethodTest(
"org.example.class#method,org.example.class#anotherMethod"),
is(false));
}

@Test
public void testSingleMethodTest_blankInput() {
assertThat(AndroidTestOrchestrator.isSingleMethodTest(null), is(false));
assertThat(AndroidTestOrchestrator.isSingleMethodTest(""), is(false));
}

@Test
public void testMakeValidFilename_notTooLong() {
final int maxLength = 20;
Expand Down