Ensure we properly run junit3 tests

In case of duplicate method name (with different args), we
ensure to only consider only potential tests method to be
included or excluded.

Test: ./gts-tradefed run gts -m GtsGmscoreHostTestCases -t
com.google.android.gts.devicepolicy.ManagedProfileTest
--log-level-display verbose -a armeabi-v7a
TF unit tests
Bug: 62618770

Change-Id: I6c440a9e086ef2c0cdd3200b92e4bb4ed9ca0d95

2 files changed, 50 insertions, 31 deletions

diff --git a/src/com/android/tradefed/testtype/DeviceTestCase.java b/src/com/android/tradefed/testtype/DeviceTestCase.java
index b52d9beff..293e49296 100644
--- a/src/com/android/tradefed/testtype/DeviceTestCase.java
+++ b/src/com/android/tradefed/testtype/DeviceTestCase.java
@@ -278,14 +278,22 @@ public class DeviceTestCase extends MetricTestCase
                 for (Method method : methods) {
                     // If the method in child class was considered already, we do not
                     if (!overridenMethod.contains(method.getName())) {
-                        if (mFilterHelper.shouldRun(
-                                theClass.getPackage().getName(), theClass, method)) {
-                            addTestMethod(method, mMethodNames);
+                        // if it at least meet the requirements for a test method
+                        if (Modifier.isPublic(method.getModifiers())
+                                && method.getReturnType().equals(Void.TYPE)
+                                && method.getParameterTypes().length == 0
+                                && method.getName().startsWith("test")) {
+                            // We add it to the child method seen
+                            if (theClass.equals(method.getDeclaringClass())) {
+                                overridenMethod.add(method.getName());
+                            }
+                            // We check if it should actually run
+                            if (mFilterHelper.shouldRun(
+                                    theClass.getPackage().getName(), theClass, method)) {
+                                mMethodNames.addElement(method.getName());
+                            }
                         }
                     }
-                    if (theClass.equals(method.getDeclaringClass())) {
-                        overridenMethod.add(method.getName());
-                    }
                 }
                 superClass = superClass.getSuperclass();
             }
@@ -296,31 +304,6 @@ public class DeviceTestCase extends MetricTestCase
         return mMethodNames;
     }
 
-    private void addTestMethod(Method m, Vector<String> names) {
-        String name = m.getName();
-        if (names.contains(name)) {
-            return;
-        }
-        if (!isPublicTestMethod(m)) {
-            if (isTestMethod(m)) {
-                Log.w(LOG_TAG, String.format("Test method isn't public: %s", m.getName()));
-            }
-            return;
-        }
-        names.addElement(name);
-    }
-
-    private boolean isPublicTestMethod(Method m) {
-        return isTestMethod(m) && Modifier.isPublic(m.getModifiers());
-    }
-
-    private boolean isTestMethod(Method m) {
-        String name = m.getName();
-        Class<?>[] parameters = m.getParameterTypes();
-        Class<?> returnType = m.getReturnType();
-        return parameters.length == 0 && name.startsWith("test") && returnType.equals(Void.TYPE);
-    }
-
     /**
      * {@inheritDoc}
      */
diff --git a/tests/src/com/android/tradefed/testtype/DeviceTestCaseTest.java b/tests/src/com/android/tradefed/testtype/DeviceTestCaseTest.java
index 5ab61131d..37ae3bc80 100644
--- a/tests/src/com/android/tradefed/testtype/DeviceTestCaseTest.java
+++ b/tests/src/com/android/tradefed/testtype/DeviceTestCaseTest.java
@@ -52,6 +52,20 @@ public class DeviceTestCaseTest extends TestCase {
         public void test2() {}
     }
 
+    /** A test class that illustrate duplicate names but from only one real test. */
+    public static class DuplicateTest extends DeviceTestCase {
+
+        public void test1() {
+            test1("");
+        }
+
+        private void test1(String arg) {
+            assertTrue(arg.isEmpty());
+        }
+
+        public void test2() {}
+    }
+
     /**
      * Simple Annotation class for testing
      */
@@ -275,4 +289,26 @@ public class DeviceTestCaseTest extends TestCase {
         test.run(listener);
         EasyMock.verify(listener);
     }
+
+    /**
+     * Test that when a test class has some private method with a method name we properly ignore it
+     * and only consider the actual real method that can execute in the filtering.
+     */
+    public void testRun_duplicateName() throws Exception {
+        DuplicateTest test = new DuplicateTest();
+        ITestInvocationListener listener = EasyMock.createMock(ITestInvocationListener.class);
+
+        listener.testRunStarted(DuplicateTest.class.getName(), 2);
+        final TestIdentifier test1 = new TestIdentifier(DuplicateTest.class.getName(), "test1");
+        final TestIdentifier test2 = new TestIdentifier(DuplicateTest.class.getName(), "test2");
+        listener.testStarted(test1);
+        listener.testEnded(test1, Collections.emptyMap());
+        listener.testStarted(test2);
+        listener.testEnded(test2, Collections.emptyMap());
+        listener.testRunEnded(EasyMock.anyLong(), EasyMock.anyObject());
+        EasyMock.replay(listener);
+
+        test.run(listener);
+        EasyMock.verify(listener);
+    }
 }