Add a new utility to improve stack traces from automatic testing
A common pattern in tests is to use try-finally to make sure
cleanup is executed. This is necessary in CTS in particular to
make sure the test does not leave the device in a bad state.
The problem with using try-finally in this manner is that any
exception thrown in the finally{} block will override any thrown
in the try{} block. If the exception in finally{} is caused by
the one from try{}, then the stack trace reported by the tools
is the consequence and not the cause, making it difficult to
interpret the stack trace of automated tests.
So today, code has to make sure that either the code in
finally{} can't throw, or can't be affected by any code in
try{}, and both of these are really difficult to ensure in
presence in finally{} of code the tester does not control.
What we'd want ideally is a structure like try-finally, that
guaratees the code in finally{} is executed in all cases,
but that bubbles up the exception from try{} if any, and
will still bubble up any exception from finally{} if the
try{} block hasn't thrown.
That's what this new tool does.
Usage from Kotlin is like try-finally :
tryTest {
testing code
} cleanup {
cleanup code
}
Usage from Java can't be made as nice, but it's relatively okay :
testAndCleanup(() -> {
testing code
}, () -> {
cleanup code
});
Bugs listed below are some tests that have been affected by
this issue and have unhelpful traces.
Test: new test for this code
Bug: 198586720
Bug: 198998862
Change-Id: I54b30a7d53772feeade99274b6120a79707ad1c9
diff --git a/staticlibs/tests/unit/src/com/android/net/module/util/CleanupTest.kt b/staticlibs/tests/unit/src/com/android/net/module/util/CleanupTest.kt
new file mode 100644
index 0000000..f4a7d10
--- /dev/null
+++ b/staticlibs/tests/unit/src/com/android/net/module/util/CleanupTest.kt
@@ -0,0 +1,102 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.net.module.util
+
+import android.util.Log
+import com.android.testutils.tryTest
+import kotlin.test.assertFailsWith
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+import kotlin.test.fail
+
+private val TAG = CleanupTest::class.toString()
+
+@RunWith(JUnit4::class)
+class CleanupTest {
+ class TestException1 : Exception()
+ class TestException2 : Exception()
+
+ @Test
+ fun testNotThrow() {
+ var x = 1
+ tryTest {
+ x = 2
+ Log.e(TAG, "Do nothing")
+ } cleanup {
+ assert(x == 2)
+ x = 3
+ Log.e(TAG, "Do nothing")
+ }
+ assert(x == 3)
+ }
+
+ @Test
+ fun testThrowTry() {
+ var x = 1
+ assertFailsWith<TestException1> {
+ tryTest {
+ x = 2
+ throw TestException1()
+ x = 4
+ } cleanup {
+ assert(x == 2)
+ x = 3
+ Log.e(TAG, "Do nothing")
+ }
+ }
+ assert(x == 3)
+ }
+
+ @Test
+ fun testThrowCleanup() {
+ var x = 1
+ assertFailsWith<TestException2> {
+ tryTest {
+ x = 2
+ Log.e(TAG, "Do nothing")
+ } cleanup {
+ assert(x == 2)
+ x = 3
+ throw TestException2()
+ x = 4
+ }
+ }
+ assert(x == 3)
+ }
+
+ @Test
+ fun testThrowBoth() {
+ var x = 1
+ try {
+ tryTest {
+ x = 2
+ throw TestException1()
+ x = 3
+ } cleanup {
+ assert(x == 2)
+ x = 4
+ throw TestException2()
+ x = 5
+ }
+ fail("Expected failure with TestException1")
+ } catch (e: TestException1) {
+ assert(e.suppressedExceptions[0] is TestException2)
+ }
+ assert(x == 4)
+ }
+}
diff --git a/staticlibs/tests/unit/src/com/android/net/module/util/CleanupTestJava.java b/staticlibs/tests/unit/src/com/android/net/module/util/CleanupTestJava.java
new file mode 100644
index 0000000..ba4e679
--- /dev/null
+++ b/staticlibs/tests/unit/src/com/android/net/module/util/CleanupTestJava.java
@@ -0,0 +1,94 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.android.net.module.util;
+
+import static com.android.testutils.Cleanup.testAndCleanup;
+import static com.android.testutils.MiscAsserts.assertThrows;
+
+import static org.junit.Assert.assertEquals;
+
+import android.util.Log;
+
+import org.junit.Test;
+
+import java.util.concurrent.atomic.AtomicInteger;
+
+public class CleanupTestJava {
+ private static final String TAG = CleanupTestJava.class.getSimpleName();
+ private static final class TestException1 extends Exception {}
+ private static final class TestException2 extends Exception {}
+
+ @Test
+ public void testNotThrow() {
+ final AtomicInteger x = new AtomicInteger(1);
+ testAndCleanup(() -> {
+ x.compareAndSet(1, 2);
+ Log.e(TAG, "Do nothing");
+ }, () -> {
+ x.compareAndSet(2, 3);
+ Log.e(TAG, "Do nothing");
+ });
+ assertEquals(3, x.get());
+ }
+
+ @Test
+ public void testThrowTry() {
+ final AtomicInteger x = new AtomicInteger(1);
+ assertThrows(TestException1.class, () ->
+ testAndCleanup(() -> {
+ x.compareAndSet(1, 2);
+ throw new TestException1();
+ // Java refuses to call x.set(3) here because this line is unreachable
+ }, () -> {
+ x.compareAndSet(2, 3);
+ Log.e(TAG, "Do nothing");
+ })
+ );
+ assertEquals(3, x.get());
+ }
+
+ @Test
+ public void testThrowCleanup() {
+ final AtomicInteger x = new AtomicInteger(1);
+ assertThrows(TestException2.class, () ->
+ testAndCleanup(() -> {
+ x.compareAndSet(1, 2);
+ Log.e(TAG, "Do nothing");
+ }, () -> {
+ x.compareAndSet(2, 3);
+ throw new TestException2();
+ // Java refuses to call x.set(4) here because this line is unreachable
+ })
+ );
+ assertEquals(3, x.get());
+ }
+
+ @Test
+ public void testThrowBoth() {
+ final AtomicInteger x = new AtomicInteger(1);
+ assertThrows(TestException1.class, () ->
+ testAndCleanup(() -> {
+ x.compareAndSet(1, 2);
+ throw new TestException1();
+ }, () -> {
+ x.compareAndSet(2, 3);
+ throw new TestException2();
+ })
+ );
+ assertEquals(3, x.get());
+ }
+}
diff --git a/staticlibs/testutils/Android.bp b/staticlibs/testutils/Android.bp
index 89aa136..153285b 100644
--- a/staticlibs/testutils/Android.bp
+++ b/staticlibs/testutils/Android.bp
@@ -51,6 +51,9 @@
"//frameworks/libs/net/common/tests:__subpackages__",
"//frameworks/libs/net/client-libs/tests:__subpackages__",
],
+ libs: [
+ "jsr305",
+ ],
static_libs: [
"kotlin-test"
]
diff --git a/staticlibs/testutils/hostdevice/com/android/testutils/Cleanup.kt b/staticlibs/testutils/hostdevice/com/android/testutils/Cleanup.kt
new file mode 100644
index 0000000..769d980
--- /dev/null
+++ b/staticlibs/testutils/hostdevice/com/android/testutils/Cleanup.kt
@@ -0,0 +1,95 @@
+/*
+ * Copyright (C) 2021 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+@file:JvmName("Cleanup")
+
+package com.android.testutils
+
+import com.android.testutils.ExceptionUtils.ThrowingRunnable
+import javax.annotation.CheckReturnValue
+
+/**
+ * Utility to do cleanup in tests without replacing exceptions with those from a finally block.
+ *
+ * This utility is meant for tests that want to do cleanup after they execute their test
+ * logic, whether the test fails (and throws) or not.
+ *
+ * The usual way of doing this is to have a try{}finally{} block and put cleanup in finally{}.
+ * However, if any code in finally{} throws, the exception thrown in finally{} is thrown before
+ * any thrown in try{} ; that means errors reported from tests are from finally{} even if they
+ * have been caused by errors in try{}. This is unhelpful in tests, because it results in a
+ * stacktrace for a symptom rather than a stacktrace for a cause.
+ *
+ * To alleviate this, tests are encouraged to make sure the code in finally{} can't throw, or
+ * that the code in try{} can't cause it to fail. This is not always realistic ; not only does
+ * it require the developer thinks about complex interactions of code, test code often relies
+ * on bricks provided by other teams, not controlled by the team writing the test, which may
+ * start throwing with an update (see b/198998862 for an example).
+ *
+ * This utility allows a different approach : it offers a new construct, tryTest{}cleanup{} similar
+ * to try{}finally{}, but that will always throw the first exception that happens. In other words,
+ * if only tryTest{} throws or only cleanup{} throws, that exception will be thrown, but contrary
+ * to the standard try{}finally{}, if both throws, the construct throws the exception that happened
+ * in tryTest{} rather than the one that happened in cleanup{}.
+ *
+ * Kotlin usage is as try{}finally{} :
+ * tryTest {
+ * testing code
+ * } cleanup {
+ * cleanup code
+ * }
+ *
+ * Java doesn't allow this kind of syntax, so instead a function taking 2 lambdas is provided.
+ * testAndCleanup(() -> {
+ * testing code
+ * }, () -> {
+ * cleanup code
+ * });
+ */
+class ExceptionCleanupBlock(val originalException: Exception?) {
+ inline infix fun cleanup(block: () -> Unit) {
+ try {
+ block()
+ if (null != originalException) throw originalException
+ } catch (e: Exception) {
+ if (null == originalException) {
+ throw e
+ } else {
+ originalException.addSuppressed(e)
+ throw originalException
+ }
+ }
+ }
+}
+
+@CheckReturnValue
+inline fun tryTest(block: () -> Unit): ExceptionCleanupBlock {
+ try {
+ block()
+ } catch (e: Exception) {
+ return ExceptionCleanupBlock(e)
+ }
+ return ExceptionCleanupBlock(null)
+}
+
+// Java support
+fun testAndCleanup(tryBlock: ThrowingRunnable, cleanupBlock: ThrowingRunnable) {
+ tryTest {
+ tryBlock.run()
+ } cleanup {
+ cleanupBlock.run()
+ }
+}