[Android] JUnit runner + gyp changes.

testing/android/java/src/org/chromium/testing/local/JunitTestMain.java

Index: testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
diff --git a/testing/android/java/src/org/chromium/testing/local/JunitTestMain.java b/testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
new file mode 100644
index 0000000000000000000000000000000000000000..21fcf8592180978b3d6af3b98221a1ccc603a1e8
--- /dev/null
+++ b/testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
@@ -0,0 +1,110 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.testing.local;
+
+import org.junit.runner.JUnitCore;
+import org.junit.runner.Request;
+import org.junit.runner.RunWith;
+
+import java.io.IOException;
+import java.util.Enumeration;
+import java.util.LinkedList;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
+
+/**
+ *  Runs tests based on JUnit from the classpath on the host JVM based on the
+ *  provided filter configurations.
+ */
+public class JunitTestMain {

[nyquist, 2014/09/19 01:26:03]

JunitTestMain only has static methods and lacks a private constructor. Also, it
could be final, since extending it doesn't gain anything.

[jbudorick, 2014/09/19 20:09:07]

Done.

+
+    /**
+     *  Finds all classes on the class path annotated with RunWith.
+     */
+    public static Class[] findClassesFromClasspath(String prefix, Class<?> runner) {

[nyquist, 2014/09/19 01:26:03]

None of these two arguments are used.

[jbudorick, 2014/09/19 20:09:07]

Removed.

+        String[] jarpaths = System.getProperty("java.class.path").split(":");

[nyquist, 2014/09/19 01:26:03]

jarPaths

[jbudorick, 2014/09/19 20:09:07]

Done.

+        LinkedList<Class> classes = new LinkedList<Class>();
+        for (String jp : jarpaths) {
+            try {
+                JarFile jf = new JarFile(jp);
+                for (Enumeration<JarEntry> eje = jf.entries(); eje.hasMoreElements();) {
+                    JarEntry je = eje.nextElement();
+                    String cn = je.getName();
+                    if (!cn.endsWith(".class") || cn.indexOf('$') != -1) {
+                        continue;
+                    }
+                    try {
+                        // Replace the / with .
+                        cn = cn.substring(0, cn.length() - 6).replace('/', '.');

[nyquist, 2014/09/19 01:26:03]

where does this magic -6 come from? Also, move this outside the try.

[jbudorick, 2014/09/19 20:09:07]

-6 comes from ".class".length()

Done, and magic removed.

+                        // Check for annotations
+                        Class<?> c = Class.forName(cn);

[nyquist, 2014/09/19 01:26:04]

Could you instead do something like:

Class<?> c = getClassOrNull(cn);
if (c != null && c.isAnnotationPresent(RunWith.class)) {
  classes.push(c);
}

That way you don't litter this high-level method with try-catch uglyness.

[jbudorick, 2014/09/19 20:09:07]

Done.

+                        if (c.isAnnotationPresent(RunWith.class)) {
+                            classes.push(c);
+                        }
+                    } catch (ClassNotFoundException e) {
+                        System.err.println("Class not found: " + cn);

[nyquist, 2014/09/19 01:26:03]

Is there any other form of logging than System.err that JUnit provides? Or maybe
use the logger you created in some way? Seems bad to litter this code with
System.err, but maybe there is no other choice.

[jbudorick, 2014/09/19 20:09:07]

I mean, some of JUnit's other code paths use System.out, but beyond that, no.

+                    } catch (NoClassDefFoundError e) {
+                        System.err.println("Class definition not found: " + cn);

[nyquist, 2014/09/19 01:26:03]

Is it safe to just print error and continue? Or should we break? I think it
seems safe to continue, but I just wanted to check.

[jbudorick, 2014/09/19 20:09:07]

Should be fine. We won't run tests from that class, but that's it.

+                    } catch (Exception e) {
+                        System.err.println("Other exception while reading class: " + cn);
+                    }
+                }
+            } catch (IOException e) {
+                System.err.println("Error while reading classes from " + jp);
+            }
+        }
+        return classes.toArray(new Class[classes.size()]);
+    }
+
+    public static void main(String[] args) throws ClassNotFoundException {
+        String packageFilter = null;
+        Class<?> runnerFilter = null;
+        String gtestFilter = null;
+
+        for (int i = 0; i < args.length; ++i) {

[nyquist, 2014/09/19 01:26:03]

Could you extract this for-loop to a class that parses the arguments, and then
has three members with has-ers and get-ers for package, runner and gtest filter?

That would simplify this main-method a lot, to something like:

JUnitCore core = new JUnitCore();
core.addListener(new GtestListener());
Request testRequest = Request.classes(new GtestComputer(),
findClassesFromClasspath());
Parser parser = Parser.parse(args);
if (parser.hasPackageFilter()) {
    testRequest = testRequest.filterWith(parser.getPackageFilter());
}
if (parser.hasRunnerFilter()) {
    testRequest = testRequest.filterWith(parser.getRunnerFilter());
}
if (parser.hasGtestFilter()) {
    testRequest = testRequest.filterWith(parser.getGtestFilter());
}
System.exit(core.run(testRequest).wasSuccessful() ? 0 : 1);

[jbudorick, 2014/09/19 20:09:07]

Done.

+            if (args[i].startsWith("-")) {
+                String argName;
+                if (args[i].startsWith("-", 1)) {
+                    argName = args[i].substring(2, args[i].length());
+                } else {
+                    argName = args[i].substring(1, args[i].length());
+                }
+                try {
+                    if (argName.equals("package-filter")) {

[nyquist, 2014/09/19 01:26:04]

flip all these equals-statements.

[jbudorick, 2014/09/19 20:09:07]

Done.

+                        packageFilter = args[++i];
+                    } else if (argName.equals("runner-filter")) {
+                        runnerFilter = Class.forName(args[++i]);

[nyquist, 2014/09/19 01:26:03]

This ++i was a bit awkward. I don't have a good suggestion to what to do instead
though, except adding "// Read next command line argument." or something above
this line (and below).

[jbudorick, 2014/09/19 20:09:07]

Done.

+                    } else if (argName.equals("gtest-filter")) {
+                        gtestFilter = args[++i];
+                    } else {
+                        System.out.println("Ignoring flag: \"" + args[i] + "\"");
+                    }
+                } catch (ArrayIndexOutOfBoundsException e) {
+                    System.err.println("No value specified for argument \"" + argName + "\"");
+                    System.exit(1);
+                }
+            } else {
+                System.out.println("Ignoring argument: \"" + args[i] + "\"");
+            }// else ignore the argument
+        }
+
+        Class[] classes = findClassesFromClasspath(packageFilter, runnerFilter);
+        JUnitCore core = new JUnitCore();
+        core.addListener(new GtestListener());
+        Request testRequest = Request.classes(new GtestComputer(), classes);
+        if (packageFilter != null) {
+            testRequest = testRequest.filterWith(new PackageFilter(packageFilter));
+        }
+        if (runnerFilter != null) {
+            testRequest = testRequest.filterWith(new RunnerFilter(runnerFilter));
+        }
+        if (gtestFilter != null) {
+            testRequest = testRequest.filterWith(new GtestFilter(gtestFilter));
+        }
+        System.exit(core.run(testRequest).wasSuccessful() ? 0 : 1);
+    }
+
+}
+