Hardening AndroidJUnitRunner against exceptions that can be thrown wh…

…en there are problems in the underlying test libraries. PiperOrigin-RevId: 664908801
android · Aug 19, 2024 · 6ef269f · 6ef269f
1 parent dd2b59a
commit 6ef269f
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 23 deletions.
diff --git a/runner/android_junit_runner/CHANGELOG.md b/runner/android_junit_runner/CHANGELOG.md
@@ -7,6 +7,7 @@
 **Bug Fixes**
 
 * Exceptions during `@AfterClass` were not being reported via `InstrumentationResultPrinter`.
+* Exceptions arising in AndroidJUnitRunner.buildRequest are now handled.
 
 **New Features**
 

diff --git a/runner/android_junit_runner/java/androidx/test/runner/AndroidJUnitRunner.java b/runner/android_junit_runner/java/androidx/test/runner/AndroidJUnitRunner.java
@@ -426,6 +426,21 @@ InstrumentationResultPrinter getInstrumentationResultPrinter() {
     return instrumentationResultPrinter;
   }
 
+  private void invokeRemoteMethod() {
+    try {
+      new ReflectiveMethod<Void>(
+              runnerArgs.remoteMethod.testClassName, runnerArgs.remoteMethod.methodName)
+          .invokeStatic();
+    } catch (ReflectionException e) {
+      Log.e(
+          LOG_TAG,
+          String.format(
+              "Reflective call to remote method %s#%s failed",
+              runnerArgs.remoteMethod.testClassName, runnerArgs.remoteMethod.methodName),
+          e);
+    }
+  }
+
   @Override
   public void onStart() {
     Log.d(LOG_TAG, "onStart is called.");
@@ -434,21 +449,11 @@ public void onStart() {
     try {
       setJsBridgeClassName("androidx.test.espresso.web.bridge.JavaScriptBridge");
       super.onStart();
+
       Request testRequest = buildRequest(runnerArgs, getArguments());
 
       if (runnerArgs.remoteMethod != null) {
-        try {
-          new ReflectiveMethod<Void>(
-                  runnerArgs.remoteMethod.testClassName, runnerArgs.remoteMethod.methodName)
-              .invokeStatic();
-        } catch (ReflectionException e) {
-          Log.e(
-              LOG_TAG,
-              String.format(
-                  "Reflective call to remote method %s#%s failed",
-                  runnerArgs.remoteMethod.testClassName, runnerArgs.remoteMethod.methodName),
-              e);
-        }
+        invokeRemoteMethod();
       }
 
       // TODO(b/162075422): using deprecated isPrimaryInstrProcess(argsProcessName) method
@@ -457,16 +462,13 @@ public void onStart() {
         return;
       }
 
-      try {
-        TestExecutor.Builder executorBuilder = new TestExecutor.Builder(this);
-        addListeners(runnerArgs, executorBuilder);
-        results = executorBuilder.build().execute(testRequest);
-      } catch (Throwable t) {
-        final String msg = "Fatal exception when running tests";
-        Log.e(LOG_TAG, msg, t);
-        onException(this, t);
-      }
-
+      TestExecutor.Builder executorBuilder = new TestExecutor.Builder(this);
+      addListeners(runnerArgs, executorBuilder);
+      results = executorBuilder.build().execute(testRequest);
+    } catch (Throwable t) {
+      final String msg = "Fatal exception when running tests";
+      Log.e(LOG_TAG, msg, t);
+      onException(this, t);
     } finally {
       Trace.endSection();
     }
@@ -623,6 +625,13 @@ public boolean onException(Object obj, Throwable e) {
       final StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskWrites();
       try {
         instResultPrinter.reportProcessCrash(e);
+      } catch (Throwable t) {
+        // It is possible for the test infrastructure to get sufficiently messed up that even this
+        // can fail (with a NoSuchMethodError from Failure.getTrace()!). Don't let that get in the
+        // way of sending events reporting the problems, since that can make the difference between
+        // "test failed with a useful error message" and "test mysteriously timed out and the
+        // developer has to go spelunking in the system logcat".
+        Log.e(LOG_TAG, "Failed to report process crash.", t);
       } finally {
         StrictMode.setThreadPolicy(oldPolicy);
       }

diff --git a/services/CHANGELOG.md b/services/CHANGELOG.md
@@ -7,6 +7,7 @@
 **Bug Fixes**
 
 * TestStorage: Use input directory location for internal files 
+* StackTrimmer: harden against exceptions coming from Failure.getMessage().
 
 **New Features**
 

diff --git a/services/events/java/androidx/test/services/events/internal/StackTrimmer.java b/services/events/java/androidx/test/services/events/internal/StackTrimmer.java
@@ -47,7 +47,14 @@ public static String getTrimmedStackTrace(Failure failure) {
   }
 
   public static String getTrimmedMessage(Failure failure) {
-    String message = failure.getMessage();
+    String message = null;
+    try {
+      // Even this can fail! If so, don't let it wreck error reporting.
+      message = failure.getMessage();
+    } catch (Throwable t) {
+      Log.e(TAG, "Failed to get message from " + failure, t);
+      message = "Couldn't get message due to " + t.getMessage();
+    }
     if (message != null && message.length() > MAX_TRACE_SIZE) {
       // Since AJUR needs to report failures back to AM via a binder IPC, we need to make sure that
       // we don't exceed the Binder transaction limit - which is 1MB per process.