[Android] JUnit runner + gyp changes.

[Android] JUnit runner + gyp changes.

This adds Java code for running junit tests, as well as gyp targets for
both runnable and non-runnable host-side JARs.

BUG=316383
Committed: https://crrev.com/2e56d4508e33de5fc60bbbb41c5a5d5534e88174
Cr-Commit-Position: refs/heads/master@{#296340}

[jbudorick, 2014-09-15 15:43:37]

jbudorick@chromium.org changed reviewers:
+ aurimas@chromium.org, cjhopman@chromium.org, nyquist@chromium.org

[jbudorick, 2014-09-15 15:43:38]

Note that this is blocked on https://codereview.chromium.org/556893004/

cjhopman for gyp
nyquist for testing/android/*
aurimas because this is finally happening

[cjhopman, 2014-09-15 22:27:37]

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/jar.py
File build/android/gyp/jar.py (right):

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/jar.py...
build/android/gyp/jar.py:16: def Jar(class_files, classes_dir, jar_path,
manifest_file=None):
I would like to actually get rid of jar.py. javac.py already has an option to
create a .jar of the compiled classes, could we just add the manifest file
handling there? This would make the gyp files simpler, too.

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/manife...
File build/android/gyp/manifest.py (right):

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/manife...
build/android/gyp/manifest.py:4: # found in the LICENSE file.
Could you add a docstring here (something really short is fine).

Alink to something that describes the structure of a manifest file would be
really helpful.

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/manife...
build/android/gyp/manifest.py:23: output = []
could probably use textwrap

return '\r\n'.join(textwrap.wrap(str_to_wrap, width=_MAX_LINE_LEN - 2,
expand_tabs=False, break_long_words=True, subsequent_indent=' '))

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/manife...
build/android/gyp/manifest.py:34: parser.add_argument('--main-class',
dest='main_class',
Is there a reason to not just infer dest from the keyword?

https://codereview.chromium.org/574433003/diff/20001/build/host_executable_ja...
File build/host_executable_jar.gypi (right):

https://codereview.chromium.org/574433003/diff/20001/build/host_executable_ja...
build/host_executable_jar.gypi:70: 'java_sources!': ['<!@(find
<@(excluded_src_paths) -name "*.java")']
This won't allow you to exclude sources in generated_src_dirs (it'll actually
work if the files exist, but won't work if you rm -rf out first).

https://codereview.chromium.org/574433003/diff/20001/build/host_executable_ja...
build/host_executable_jar.gypi:77: '^@(java_sources)',
Does this have to be '^' because of the 'java_sources!'?

https://codereview.chromium.org/574433003/diff/20001/build/host_jar.gypi
File build/host_jar.gypi (right):

https://codereview.chromium.org/574433003/diff/20001/build/host_jar.gypi#newc...
build/host_jar.gypi:41: 'classes_dir': '<(intermediate_dir)/classes',
I'd prefer this to just generate the .jar from javac.py directly. In that case,
classes_dir doesn't need to be specified (and will just become a TempDir in
javac.py).

Same for the executable jar.

[jbudorick, 2014-09-16 02:50:41]

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/jar.py
File build/android/gyp/jar.py (right):

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/jar.py...
build/android/gyp/jar.py:16: def Jar(class_files, classes_dir, jar_path,
manifest_file=None):
On 2014/09/15 22:27:36, cjhopman wrote:
> I would like to actually get rid of jar.py. javac.py already has an option to
> create a .jar of the compiled classes, could we just add the manifest file
> handling there? This would make the gyp files simpler, too.

This has to keep some manifest logic since javac.py calls JarDirectory, but I've
moved things around to use javac. The stuff in main() is gone.

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/manife...
File build/android/gyp/manifest.py (right):

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/manife...
build/android/gyp/manifest.py:4: # found in the LICENSE file.
On 2014/09/15 22:27:36, cjhopman wrote:
> Could you add a docstring here (something really short is fine).
> 
> Alink to something that describes the structure of a manifest file would be
> really helpful.

Manifest functionality moved into javac.py. Added a link to in the docstring for
the subsequent function.

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/manife...
build/android/gyp/manifest.py:23: output = []
On 2014/09/15 22:27:36, cjhopman wrote:
> could probably use textwrap
> 
> return '\r\n'.join(textwrap.wrap(str_to_wrap, width=_MAX_LINE_LEN - 2,
> expand_tabs=False, break_long_words=True, subsequent_indent=' '))

Didn't realize this existed. Nice.

https://codereview.chromium.org/574433003/diff/20001/build/android/gyp/manife...
build/android/gyp/manifest.py:34: parser.add_argument('--main-class',
dest='main_class',
On 2014/09/15 22:27:36, cjhopman wrote:
> Is there a reason to not just infer dest from the keyword?

Nope. I think I was in the habit of being explicit with dest when I wrote this a
few months back. Removed.

https://codereview.chromium.org/574433003/diff/20001/build/host_executable_ja...
File build/host_executable_jar.gypi (right):

https://codereview.chromium.org/574433003/diff/20001/build/host_executable_ja...
build/host_executable_jar.gypi:70: 'java_sources!': ['<!@(find
<@(excluded_src_paths) -name "*.java")']
On 2014/09/15 22:27:36, cjhopman wrote:
> This won't allow you to exclude sources in generated_src_dirs (it'll actually
> work if the files exist, but won't work if you rm -rf out first).
> 

Hm, I hadn't thought of people trying to exclude things from generated_src_dirs.
Thoughts on how to handle that case?

https://codereview.chromium.org/574433003/diff/20001/build/host_executable_ja...
build/host_executable_jar.gypi:77: '^@(java_sources)',
On 2014/09/15 22:27:37, cjhopman wrote:
> Does this have to be '^' because of the 'java_sources!'?

Yep. Exclusion happens between PHASE_LATE (>) and PHASE_LATELATE (^).

https://codereview.chromium.org/574433003/diff/20001/build/host_jar.gypi
File build/host_jar.gypi (right):

https://codereview.chromium.org/574433003/diff/20001/build/host_jar.gypi#newc...
build/host_jar.gypi:41: 'classes_dir': '<(intermediate_dir)/classes',
On 2014/09/15 22:27:37, cjhopman wrote:
> I'd prefer this to just generate the .jar from javac.py directly. In that
case,
> classes_dir doesn't need to be specified (and will just become a TempDir in
> javac.py).
> 

Done.

> Same for the executable jar.

Now that I'm doing the manifest logic in javac, I was able to get rid of
host_executable_jar.gypi entirely.

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

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestComputer.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestComputer.java:29:
super();
this call does nothing

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestFilter.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:17: class
GtestFilter extends Filter {
I know this is meta, but is there any way we could write a test for this?

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:21: private
Set<String> mPositiveRegexes;
final for both regexes

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:22: private
Set<String> mNegativeRegexes;
Can these be Set<Pattern> ?
Then using Pattern.compile(...) when adding them, and
p.matcher(gtestName).matches() below?

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:33:
String[] filterStrings = filterString.split(":");
Regexpes are better to compile up front instead of doing them within the
for-loop, etc.

How about adding these:
    private static final Pattern PERIOD = Pattern.compile("\\.");
    private static final Pattern ASTERIX = Pattern.compile("\\*");
    private static final Pattern COLON = Pattern.compile(":");
    private static final Pattern DASH = Pattern.compile("-");

The code below would then read:
String[] filterStrings = COLON.split(filterString);
etc.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:62: if
(description.getMethodName() == null) {
could you keep all these if-statements with their returns on one line?

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:67:
description.getClassName() + "." + description.getMethodName();
single line (java is 100 chars)

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:69: if
(Pattern.matches(p, gtestName)) {
p.matcher(gtestName).matches() seems better to me. See comment above.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:91: return
new String("gtest-filter: " + mFilterString);
just return "gtest-filter: " + mFilterString;

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestListener.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestListener.java:18:
private HashSet<Description> mFailedTests;
Just Set<Description>

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestListener.java:21:
private int mTestsTotal;
This member is not used.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestLogger.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:20:
System.out.format("[ RUN      ] %s.%s", test.getClassName(),
one line

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:28: public
static void testFinished(Description test, boolean passed,
one line

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:44:
System.out.format("[----------] Run %d test cases from %s", testCount,
one line

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:86:
System.out.format("[  FAILED  ] %s.%s", d.getClassName(),
one line

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:21:
public class JunitTestMain {
JunitTestMain only has static methods and lacks a private constructor. Also, it
could be final, since extending it doesn't gain anything.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:26:
public static Class[] findClassesFromClasspath(String prefix, Class<?> runner) {
None of these two arguments are used.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:27:
String[] jarpaths = System.getProperty("java.class.path").split(":");
jarPaths

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:40: cn =
cn.substring(0, cn.length() - 6).replace('/', '.');
where does this magic -6 come from? Also, move this outside the try.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:42:
Class<?> c = Class.forName(cn);
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.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:47:
System.err.println("Class not found: " + cn);
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.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:49:
System.err.println("Class definition not found: " + cn);
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.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:66: for
(int i = 0; i < args.length; ++i) {
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);

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:75: if
(argName.equals("package-filter")) {
flip all these equals-statements.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:78:
runnerFilter = Class.forName(args[++i]);
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).

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/PackageFilter.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/PackageFilter.java:11: * 
Filters tests based on the package.
This implementation doesn't require package only, does it? You can also specify
classes? "com.example.MyClass" would match getClassName().startsWith(...), no?

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/PackageFilter.java:13: class
PackageFilter extends Filter {
Is this missing @Override statements? I think it goes for all these classes.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/PackageFilter.java:36:
return new String("package-filter: " + mFilterString);
Just return "package-filter: " + mFilterString;

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/RunnerFilter.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/RunnerFilter.java:32: if (c
== null) {
does it really return null? Shouldn't this instead be in the catch-block for
ClassNotFoundException?

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/RunnerFilter.java:47: return
new String("runner-filter: " + mRunnerClass.toString());
just return "runner-filter: " + mRunnerClass;

https://codereview.chromium.org/574433003/diff/60001/testing/android/junit_te...
File testing/android/junit_test.gyp (right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/junit_te...
testing/android/junit_test.gyp:1: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.
Remove (c)

https://codereview.chromium.org/574433003/diff/60001/third_party/junit/junit.gyp
File third_party/junit/junit.gyp (right):

https://codereview.chromium.org/574433003/diff/60001/third_party/junit/junit....
third_party/junit/junit.gyp:1: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.
Remove (c)

[jbudorick, 2014-09-19 20:09:08]

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestComputer.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestComputer.java:29:
super();
On 2014/09/19 01:26:02, nyquist wrote:
> this call does nothing

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestFilter.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:17: class
GtestFilter extends Filter {
On 2014/09/19 01:26:03, nyquist wrote:
> I know this is meta, but is there any way we could write a test for this?

Added tests for this, PackageFilter, RunnerFilter, and GtestLogger... which get
run by all of this code. Meta indeed.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:21: private
Set<String> mPositiveRegexes;
On 2014/09/19 01:26:03, nyquist wrote:
> final for both regexes

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:22: private
Set<String> mNegativeRegexes;
On 2014/09/19 01:26:02, nyquist wrote:
> Can these be Set<Pattern> ?
> Then using Pattern.compile(...) when adding them, and
> p.matcher(gtestName).matches() below?

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:33:
String[] filterStrings = filterString.split(":");
On 2014/09/19 01:26:02, nyquist wrote:
> Regexpes are better to compile up front instead of doing them within the
> for-loop, etc.
> 
> How about adding these:
>     private static final Pattern PERIOD = Pattern.compile("\\.");
>     private static final Pattern ASTERIX = Pattern.compile("\\*");
>     private static final Pattern COLON = Pattern.compile(":");
>     private static final Pattern DASH = Pattern.compile("-");
> 
> The code below would then read:
> String[] filterStrings = COLON.split(filterString);
> etc.

These are regular string operations, not regexes.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:62: if
(description.getMethodName() == null) {
On 2014/09/19 01:26:02, nyquist wrote:
> could you keep all these if-statements with their returns on one line?

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:67:
description.getClassName() + "." + description.getMethodName();
On 2014/09/19 01:26:02, nyquist wrote:
> single line (java is 100 chars)

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:69: if
(Pattern.matches(p, gtestName)) {
On 2014/09/19 01:26:03, nyquist wrote:
> p.matcher(gtestName).matches() seems better to me. See comment above.

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:91: return
new String("gtest-filter: " + mFilterString);
On 2014/09/19 01:26:02, nyquist wrote:
> just return "gtest-filter: " + mFilterString;

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestListener.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestListener.java:18:
private HashSet<Description> mFailedTests;
On 2014/09/19 01:26:03, nyquist wrote:
> Just Set<Description>

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestListener.java:21:
private int mTestsTotal;
On 2014/09/19 01:26:03, nyquist wrote:
> This member is not used.

Removed.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestLogger.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:20:
System.out.format("[ RUN      ] %s.%s", test.getClassName(),
On 2014/09/19 01:26:03, nyquist wrote:
> one line

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:28: public
static void testFinished(Description test, boolean passed,
On 2014/09/19 01:26:03, nyquist wrote:
> one line

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:44:
System.out.format("[----------] Run %d test cases from %s", testCount,
On 2014/09/19 01:26:03, nyquist wrote:
> one line

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:86:
System.out.format("[  FAILED  ] %s.%s", d.getClassName(),
On 2014/09/19 01:26:03, nyquist wrote:
> one line

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:21:
public class JunitTestMain {
On 2014/09/19 01:26:03, nyquist wrote:
> JunitTestMain only has static methods and lacks a private constructor. Also,
it
> could be final, since extending it doesn't gain anything.

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:26:
public static Class[] findClassesFromClasspath(String prefix, Class<?> runner) {
On 2014/09/19 01:26:03, nyquist wrote:
> None of these two arguments are used.

Removed.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:27:
String[] jarpaths = System.getProperty("java.class.path").split(":");
On 2014/09/19 01:26:03, nyquist wrote:
> jarPaths

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:40: cn =
cn.substring(0, cn.length() - 6).replace('/', '.');
On 2014/09/19 01:26:03, nyquist wrote:
> where does this magic -6 come from? Also, move this outside the try.

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

Done, and magic removed.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:42:
Class<?> c = Class.forName(cn);
On 2014/09/19 01:26:04, nyquist wrote:
> 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.

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:47:
System.err.println("Class not found: " + cn);
On 2014/09/19 01:26:03, nyquist wrote:
> 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.

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

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:49:
System.err.println("Class definition not found: " + cn);
On 2014/09/19 01:26:03, nyquist wrote:
> 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.

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

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:66: for
(int i = 0; i < args.length; ++i) {
On 2014/09/19 01:26:03, nyquist wrote:
> 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);

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:75: if
(argName.equals("package-filter")) {
On 2014/09/19 01:26:04, nyquist wrote:
> flip all these equals-statements.

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:78:
runnerFilter = Class.forName(args[++i]);
On 2014/09/19 01:26:03, nyquist wrote:
> 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).

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/PackageFilter.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/PackageFilter.java:11: * 
Filters tests based on the package.
On 2014/09/19 01:26:04, nyquist wrote:
> This implementation doesn't require package only, does it? You can also
specify
> classes? "com.example.MyClass" would match getClassName().startsWith(...), no?

True for this implementation. Revised implementation does require package only.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/PackageFilter.java:13: class
PackageFilter extends Filter {
On 2014/09/19 01:26:04, nyquist wrote:
> Is this missing @Override statements? I think it goes for all these classes.

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/PackageFilter.java:36:
return new String("package-filter: " + mFilterString);
On 2014/09/19 01:26:04, nyquist wrote:
> Just return "package-filter: " + mFilterString;

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/RunnerFilter.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/RunnerFilter.java:32: if (c
== null) {
On 2014/09/19 01:26:04, nyquist wrote:
> does it really return null? Shouldn't this instead be in the catch-block for
> ClassNotFoundException?

Switched to Description.getTestClass(), which handles ClassNotFoundException.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/RunnerFilter.java:47: return
new String("runner-filter: " + mRunnerClass.toString());
On 2014/09/19 01:26:04, nyquist wrote:
> just return "runner-filter: " + mRunnerClass;

Done.

https://codereview.chromium.org/574433003/diff/60001/testing/android/junit_te...
File testing/android/junit_test.gyp (right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/junit_te...
testing/android/junit_test.gyp:1: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.
On 2014/09/19 01:26:04, nyquist wrote:
> Remove (c)

Done.

https://codereview.chromium.org/574433003/diff/60001/third_party/junit/junit.gyp
File third_party/junit/junit.gyp (right):

https://codereview.chromium.org/574433003/diff/60001/third_party/junit/junit....
third_party/junit/junit.gyp:1: # Copyright (c) 2014 The Chromium Authors. All
rights reserved.
On 2014/09/19 01:26:04, nyquist wrote:
> Remove (c)

Done.

[nyquist, 2014-09-19 22:21:45]

thanks! this was easier to read.

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestFilter.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:33:
String[] filterStrings = filterString.split(":");
On 2014/09/19 20:09:06, jbudorick wrote:
> On 2014/09/19 01:26:02, nyquist wrote:
> > Regexpes are better to compile up front instead of doing them within the
> > for-loop, etc.
> > 
> > How about adding these:
> >     private static final Pattern PERIOD = Pattern.compile("\\.");
> >     private static final Pattern ASTERIX = Pattern.compile("\\*");
> >     private static final Pattern COLON = Pattern.compile(":");
> >     private static final Pattern DASH = Pattern.compile("-");
> > 
> > The code below would then read:
> > String[] filterStrings = COLON.split(filterString);
> > etc.
> 
> These are regular string operations, not regexes.

Well, that's the same thing though. Behind the scenes, they are  compiled into
regex patterns.

String.replaceAll is implemented as something like:
Pattern.compile(regex).matcher(this).replaceAll(replacement);

Which means that in your version, you compile the regexp every time in your
for-loop.

See
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7u40-b...

The same goes for String.split, but I think you hit the fast-path of split,
since you have a single-character split and it's not one of ".$|()[{^?*+\".

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/GtestListener.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/GtestListener.java:76:
System.err.println(f.getDescription());
Should this also use mLogger?

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/GtestListener.java:78:
f.getException().printStackTrace();
I guess you could do something along the lines of:

StringWriter sw = new StringWriter();
PrintWriter pw = new PrintWriter(sw);
t.printStackTrace(pw);
String stackTrace = sw.toString();

And then do something with the logger and stackTrace?

Or maybe provide printing stack trace as a method on the logger?

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/GtestLogger.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:17: private
PrintStream mOutputStream;
final

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/JunitTestArgParser.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestArgParser.java:15:
private Set<String> mPackageFilters;
All these can be final.

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestArgParser.java:71:
private JunitTestArgParser() {
Could you move the constructor above the public non-static methods (even though
it's private)?

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:69:
public static void main(String[] args) throws ClassNotFoundException {
Is ClassNotFoundException still thrown?

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:72:
Class[] classes = findClassesFromClasspath();
I don't think you use these until you call Request.classes. Care to move it down
to just above that?

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/RunnerFilter.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/RunnerFilter.java:41: return
"runner-filter: " + mRunnerClass.getName();
This is the only place you use .getName(). Do you want to do that for the others
too? Or possible getSimpleName()?

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
File
testing/android/javatests/src/org/chromium/testing/local/GtestFilterTest.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestFilterTest.java:83:
Filter filterUnderTest = new GtestFilter("TestClass.*-TestClass.testMethod");
the .*- part was very awkward to me. Is there supposed to be a : in there
somehow?

Or is just the - a magic marker as well? If this is just how gtest works, then
fine with me.

I would've thought something like .*:- from looking at the example below, but
that's only for separating positive an negatives then?

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
File
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java:25:
public class GtestLoggerTest {
Thanks! I think this is helpful, since in fact other code will parse this.

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java:29:
ByteArrayOutputStream actual = new ByteArrayOutputStream();
Optional: This is fine with me, but maybe just use StringStream for clarity? I
don't have a strong opinion though.

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java:40:
ByteArrayOutputStream actual = new ByteArrayOutputStream();
Optional: This seems to be repeated. Could you put these as members instead, and
add @Before for an initializer? It's only two simple lines though, so I'm fine
with keeping as is.

[jbudorick, 2014-09-20 00:11:29]

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
File testing/android/java/src/org/chromium/testing/local/GtestFilter.java
(right):

https://codereview.chromium.org/574433003/diff/60001/testing/android/java/src...
testing/android/java/src/org/chromium/testing/local/GtestFilter.java:33:
String[] filterStrings = filterString.split(":");
On 2014/09/19 22:21:45, nyquist wrote:
> On 2014/09/19 20:09:06, jbudorick wrote:
> > On 2014/09/19 01:26:02, nyquist wrote:
> > > Regexpes are better to compile up front instead of doing them within the
> > > for-loop, etc.
> > > 
> > > How about adding these:
> > >     private static final Pattern PERIOD = Pattern.compile("\\.");
> > >     private static final Pattern ASTERIX = Pattern.compile("\\*");
> > >     private static final Pattern COLON = Pattern.compile(":");
> > >     private static final Pattern DASH = Pattern.compile("-");
> > > 
> > > The code below would then read:
> > > String[] filterStrings = COLON.split(filterString);
> > > etc.
> > 
> > These are regular string operations, not regexes.
> 
> Well, that's the same thing though. Behind the scenes, they are  compiled into
> regex patterns.
> 

Didn't realize that. Switched to your suggestion.

> String.replaceAll is implemented as something like:
> Pattern.compile(regex).matcher(this).replaceAll(replacement);
> 
> Which means that in your version, you compile the regexp every time in your
> for-loop.
> 
> See
>
http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/7u40-b...
> 
> The same goes for String.split, but I think you hit the fast-path of split,
> since you have a single-character split and it's not one of ".$|()[{^?*+\".

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/GtestListener.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/GtestListener.java:76:
System.err.println(f.getDescription());
On 2014/09/19 22:21:45, nyquist wrote:
> Should this also use mLogger?

Yeah, I missed this function entirely with mLogger.

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/GtestListener.java:78:
f.getException().printStackTrace();
On 2014/09/19 22:21:45, nyquist wrote:
> I guess you could do something along the lines of:
> 
> StringWriter sw = new StringWriter();
> PrintWriter pw = new PrintWriter(sw);
> t.printStackTrace(pw);
> String stackTrace = sw.toString();
> 
> And then do something with the logger and stackTrace?
> 
> Or maybe provide printing stack trace as a method on the logger?

Added a method to the logger.

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/GtestLogger.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/GtestLogger.java:17: private
PrintStream mOutputStream;
On 2014/09/19 22:21:45, nyquist wrote:
> final

Done.

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/JunitTestArgParser.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestArgParser.java:15:
private Set<String> mPackageFilters;
On 2014/09/19 22:21:45, nyquist wrote:
> All these can be final.

Done.

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestArgParser.java:71:
private JunitTestArgParser() {
On 2014/09/19 22:21:45, nyquist wrote:
> Could you move the constructor above the public non-static methods (even
though
> it's private)?

Done.

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:69:
public static void main(String[] args) throws ClassNotFoundException {
On 2014/09/19 22:21:45, nyquist wrote:
> Is ClassNotFoundException still thrown?

Nope. Removed.

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:72:
Class[] classes = findClassesFromClasspath();
On 2014/09/19 22:21:45, nyquist wrote:
> I don't think you use these until you call Request.classes. Care to move it
down
> to just above that?

Done.

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/RunnerFilter.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/RunnerFilter.java:41: return
"runner-filter: " + mRunnerClass.getName();
On 2014/09/19 22:21:45, nyquist wrote:
> This is the only place you use .getName(). Do you want to do that for the
others
> too? Or possible getSimpleName()?

You mean the other Filters...? The other two are use Strings, while this uses a
Class.

If you mean something else, I'm lost on this comment.

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
File
testing/android/javatests/src/org/chromium/testing/local/GtestFilterTest.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestFilterTest.java:83:
Filter filterUnderTest = new GtestFilter("TestClass.*-TestClass.testMethod");
On 2014/09/19 22:21:45, nyquist wrote:
> the .*- part was very awkward to me. Is there supposed to be a : in there
> somehow?
> 
> Or is just the - a magic marker as well? If this is just how gtest works, then
> fine with me.
> 

It is awkward, but gtest does handle this:
https://code.google.com/p/googletest/wiki/AdvancedGuide#Running_a_Subset_of_t...

> I would've thought something like .*:- from looking at the example below, but
> that's only for separating positive an negatives then?

"TestClass.*-TestClass.testMethod" and "TestClass.*:-TestClass.testMethod"
behave the same way. I've added a test for that case.

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
File
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java:29:
ByteArrayOutputStream actual = new ByteArrayOutputStream();
On 2014/09/19 22:21:45, nyquist wrote:
> Optional: This is fine with me, but maybe just use StringStream for clarity? I
> don't have a strong opinion though.

discussed offline, sticking with this impl.

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java:40:
ByteArrayOutputStream actual = new ByteArrayOutputStream();
On 2014/09/19 22:21:45, nyquist wrote:
> Optional: This seems to be repeated. Could you put these as members instead,
and
> add @Before for an initializer? It's only two simple lines though, so I'm fine
> with keeping as is.

I thought about this too. I was on the fence, as leaving these lines in each
test makes the test a little more readable at the cost of more boilerplate.

I switched, but I'm still not sure about it.

[nyquist, 2014-09-22 23:32:09]

lgtm

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
File
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java:40:
ByteArrayOutputStream actual = new ByteArrayOutputStream();
On 2014/09/20 00:11:29, jbudorick wrote:
> On 2014/09/19 22:21:45, nyquist wrote:
> > Optional: This seems to be repeated. Could you put these as members instead,
> and
> > add @Before for an initializer? It's only two simple lines though, so I'm
fine
> > with keeping as is.
> 
> I thought about this too. I was on the fence, as leaving these lines in each
> test makes the test a little more readable at the cost of more boilerplate.
> 
> I switched, but I'm still not sure about it.

I see.
Now that I see it, I do agree it loses a bit of readability to do this as
members which is also why I marked it as optional. :-)

However, I would be totally OK if you change it back to what you had, but since
you've already done the work, feel free to keep what you have as well.

Given there were no other changes to these tests,
git checkout HEAD^ --
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java
for the win? :-p

https://codereview.chromium.org/574433003/diff/140001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
(right):

https://codereview.chromium.org/574433003/diff/140001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:32:
String[] jarPaths = System.getProperty("java.class.path").split(":");
Would you want to use pattern here as well? I'm fine without though since it
hits the optimized path.

[cjhopman, 2014-09-22 23:52:47]

https://codereview.chromium.org/574433003/diff/140001/build/host_jar.gypi
File build/host_jar.gypi (right):

https://codereview.chromium.org/574433003/diff/140001/build/host_jar.gypi#new...
build/host_jar.gypi:44: 'excluded_src_paths%': [],
no need for % on lists (lists are merged when combining dicts).

https://codereview.chromium.org/574433003/diff/140001/testing/android/junit_t...
File testing/android/junit_test.gyp (right):

https://codereview.chromium.org/574433003/diff/140001/testing/android/junit_t...
testing/android/junit_test.gyp:20: 'main_class':
'org.chromium.testing.local.JunitTestMain',
Hm, I'm not sure I like that this is injected into dependents.
all_dependent_settings is something that should generally be avoided. I think
it'd be better if the top-level jars just specified this on their own.

I'm not a huge fan of that either, another alternative would be to have another
.gypi that set main_class... but idk, for now I'd just stick with having to
manually specify it on top-level junit targets.

https://codereview.chromium.org/574433003/diff/140001/third_party/junit/junit...
File third_party/junit/junit.gyp (right):

https://codereview.chromium.org/574433003/diff/140001/third_party/junit/junit...
third_party/junit/junit.gyp:16: 'dest_path':
'<(PRODUCT_DIR)/lib.java/<(hamcrest_jar_name)',
I'd prefer that most of this be moved to a .gypi in build/ or build/android/
(like build/host_prebuilt_jar.gypi).

Having things outside those directories assume things about what the .gypis do
(like that jars go in lib.java or about how input_jars_paths is used), makes
maintenance harder.

Also, that would be consistent with how we do prebuilt jars for android
libraries.

[jbudorick, 2014-09-22 23:53:01]

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
File
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java
(right):

https://codereview.chromium.org/574433003/diff/100001/testing/android/javates...
testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java:40:
ByteArrayOutputStream actual = new ByteArrayOutputStream();
On 2014/09/22 23:32:09, nyquist wrote:
> On 2014/09/20 00:11:29, jbudorick wrote:
> > On 2014/09/19 22:21:45, nyquist wrote:
> > > Optional: This seems to be repeated. Could you put these as members
instead,
> > and
> > > add @Before for an initializer? It's only two simple lines though, so I'm
> fine
> > > with keeping as is.
> > 
> > I thought about this too. I was on the fence, as leaving these lines in each
> > test makes the test a little more readable at the cost of more boilerplate.
> > 
> > I switched, but I'm still not sure about it.
> 
> I see.
> Now that I see it, I do agree it loses a bit of readability to do this as
> members which is also why I marked it as optional. :-)
> 
> However, I would be totally OK if you change it back to what you had, but
since
> you've already done the work, feel free to keep what you have as well.
> 

Reverting to non-@Before version.

> Given there were no other changes to these tests,
> git checkout HEAD^ --
> testing/android/javatests/src/org/chromium/testing/local/GtestLoggerTest.java
> for the win? :-p

https://codereview.chromium.org/574433003/diff/140001/testing/android/java/sr...
File testing/android/java/src/org/chromium/testing/local/JunitTestMain.java
(right):

https://codereview.chromium.org/574433003/diff/140001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:32:
String[] jarPaths = System.getProperty("java.class.path").split(":");
On 2014/09/22 23:32:09, nyquist wrote:
> Would you want to use pattern here as well? I'm fine without though since it
> hits the optimized path.

I didn't think about using it here, but yeah, I'd rather be consistent and use
Pattern.

https://codereview.chromium.org/574433003/diff/140001/testing/android/java/sr...
testing/android/java/src/org/chromium/testing/local/JunitTestMain.java:43: cn =
cn.substring(0, cn.length() - CLASS_FILE_EXT.length()).replace('/', '.');
Also switched to Pattern here.

[jbudorick, 2014-09-23 01:00:19]

https://codereview.chromium.org/574433003/diff/140001/build/host_jar.gypi
File build/host_jar.gypi (right):

https://codereview.chromium.org/574433003/diff/140001/build/host_jar.gypi#new...
build/host_jar.gypi:44: 'excluded_src_paths%': [],
On 2014/09/22 23:52:47, cjhopman wrote:
> no need for % on lists (lists are merged when combining dicts).

Done.

https://codereview.chromium.org/574433003/diff/140001/testing/android/junit_t...
File testing/android/junit_test.gyp (right):

https://codereview.chromium.org/574433003/diff/140001/testing/android/junit_t...
testing/android/junit_test.gyp:20: 'main_class':
'org.chromium.testing.local.JunitTestMain',
On 2014/09/22 23:52:47, cjhopman wrote:
> Hm, I'm not sure I like that this is injected into dependents.
> all_dependent_settings is something that should generally be avoided. I think
> it'd be better if the top-level jars just specified this on their own.
> 
> I'm not a huge fan of that either, another alternative would be to have
another
> .gypi that set main_class... but idk, for now I'd just stick with having to
> manually specify it on top-level junit targets.

Done.

https://codereview.chromium.org/574433003/diff/140001/third_party/junit/junit...
File third_party/junit/junit.gyp (right):

https://codereview.chromium.org/574433003/diff/140001/third_party/junit/junit...
third_party/junit/junit.gyp:16: 'dest_path':
'<(PRODUCT_DIR)/lib.java/<(hamcrest_jar_name)',
On 2014/09/22 23:52:47, cjhopman wrote:
> I'd prefer that most of this be moved to a .gypi in build/ or build/android/
> (like build/host_prebuilt_jar.gypi).
> 
> Having things outside those directories assume things about what the .gypis do
> (like that jars go in lib.java or about how input_jars_paths is used), makes
> maintenance harder.
> 
> Also, that would be consistent with how we do prebuilt jars for android
> libraries.

Done.

[cjhopman, 2014-09-23 01:14:04]

lgtm

[jbudorick, 2014-09-23 01:23:38]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-09-23 01:24:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/574433003/200001

[commit-bot: I haz the power, 2014-09-23 01:41:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-23 01:41:33]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[jbudorick, 2014-09-23 02:34:31]

Currently, build/apk_test.gypi uses

  'java_in_dir': '<(DEPTH)/testing/android/java'

which means it tries to compile all of the code the junit code in
testing/android in this patch. I've resolved it by narrowing down java_in_dir
and specifying non-default values for java_in_dir_suffix and
android_manifest_path. There are a couple of other ways I could resolve this:
 - Add exclusion to build/java_apk.gypi, similar to what I did in
build/host_jar.gypi, or
 - Move the junit code somewhere else.

I'm open to the former but figure it could have more far-reaching effects than
what I have. I'm not a huge fan of the latter.

cjhopman, wdyt?

[jbudorick, 2014-09-24 01:03:21]

After discussion with cjhopman offline, I moved the junit code to
testing/android/junit.

fyi, the junit_unit_tests will run on the bots once I land
https://codereview.chromium.org/591333002/

[cjhopman, 2014-09-24 01:06:52]

lgtm w/ nits

https://codereview.chromium.org/574433003/diff/240001/testing/android/junit/j...
File testing/android/junit/junit_test.gyp (right):

https://codereview.chromium.org/574433003/diff/240001/testing/android/junit/j...
testing/android/junit/junit_test.gyp:15:
'../../../testing/android/junit/java/src',
nit: just 'java/src'

https://codereview.chromium.org/574433003/diff/240001/testing/android/junit/j...
testing/android/junit/junit_test.gyp:31:
'../../../testing/android/junit/javatests/src',
just 'javatests/src'

https://codereview.chromium.org/574433003/diff/240001/third_party/junit/junit...
File third_party/junit/junit.gyp (right):

https://codereview.chromium.org/574433003/diff/240001/third_party/junit/junit...
third_party/junit/junit.gyp:11: 'jar_path':
'../../third_party/junit/src/lib/hamcrest-core-1.3.jar',
nit: the paths here are relative to third_party/junit and so drop the leading
'../../third_party/junit'

[jbudorick, 2014-09-24 01:16:41]

https://codereview.chromium.org/574433003/diff/240001/testing/android/junit/j...
File testing/android/junit/junit_test.gyp (right):

https://codereview.chromium.org/574433003/diff/240001/testing/android/junit/j...
testing/android/junit/junit_test.gyp:15:
'../../../testing/android/junit/java/src',
On 2014/09/24 01:06:52, cjhopman wrote:
> nit: just 'java/src'

Done.

https://codereview.chromium.org/574433003/diff/240001/testing/android/junit/j...
testing/android/junit/junit_test.gyp:31:
'../../../testing/android/junit/javatests/src',
On 2014/09/24 01:06:52, cjhopman wrote:
> just 'javatests/src'

Done.

https://codereview.chromium.org/574433003/diff/240001/third_party/junit/junit...
File third_party/junit/junit.gyp (right):

https://codereview.chromium.org/574433003/diff/240001/third_party/junit/junit...
third_party/junit/junit.gyp:11: 'jar_path':
'../../third_party/junit/src/lib/hamcrest-core-1.3.jar',
On 2014/09/24 01:06:52, cjhopman wrote:
> nit: the paths here are relative to third_party/junit and so drop the leading
> '../../third_party/junit'

Done.

[jbudorick, 2014-09-24 01:16:51]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-09-24 01:17:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/574433003/260001

[commit-bot: I haz the power, 2014-09-24 01:21:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 01:21:49]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jbudorick, 2014-09-24 02:49:07]

The CQ bit was checked by jbudorick@chromium.org

[commit-bot: I haz the power, 2014-09-24 02:49:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/574433003/280001

[commit-bot: I haz the power, 2014-09-24 03:41:28]

Committed patchset #15 (id:280001) as ce7eb54c0ebb4aca9e591779850ff92dab1bed46

[commit-bot: I haz the power, 2014-09-24 03:42:09]

Patchset 15 (id:??) landed as
https://crrev.com/2e56d4508e33de5fc60bbbb41c5a5d5534e88174
Cr-Commit-Position: refs/heads/master@{#296340}

[phoglund_chromium, 2014-09-24 11:11:14]

A revert of this CL (patchset #15 id:280001) has been created in
https://codereview.chromium.org/597123002/ by phoglund@chromium.org.

The reason for reverting is: Speculative revert: Android test started flaking a
ton after this landed. Affected targets androidwebview_instrumentation_tests,
chromeshell_instrumentation_tests, contentshell_instrumentation_tests. This CL
seems the most likely in the blamelist of
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/1....

[jbudorick, 2014-09-24 13:51:46]

On 2014/09/24 11:11:14, phoglund wrote:
> A revert of this CL (patchset #15 id:280001) has been created in
> https://codereview.chromium.org/597123002/ by mailto:phoglund@chromium.org.
> 
> The reason for reverting is: Speculative revert: Android test started flaking
a
> ton after this landed. Affected targets androidwebview_instrumentation_tests,
> chromeshell_instrumentation_tests, contentshell_instrumentation_tests. This CL
> seems the most likely in the blamelist of
>
https://build.chromium.org/p/chromium.linux/builders/Android%20Tests/builds/1....

I'd be extremely surprised if this CL was the cause of those failures.

[kindrik, 2015-08-14 10:15:43]

I have wrote my comment already. Just use this "publish & mail" button to send
it. Or, what am I expected to write here?

https://codereview.chromium.org/574433003/diff/280001/testing/android/junit/j...
File testing/android/junit/java/src/org/chromium/testing/local/GtestFilter.java
(right):

https://codereview.chromium.org/574433003/diff/280001/testing/android/junit/j...
testing/android/junit/java/src/org/chromium/testing/local/GtestFilter.java:44:
int negIndex = sanitized.indexOf('-');
Hi. Sorry, I have to add a fly in the ointment. But as I see this, I think an
error creeped in there.
Your code first of all splits filterString to parts by colon (':').
Then it checks each part for dash '-'. 
This is fine for filters like this "--gtest_filter=FooTest.*-FooTest.Bar" but
more complicated filters like
"--gtest_filter=FooTest.*-FooTest.test1:FooTest.test2:FooTest.test3" will be
treated incorrectly.
FooTest.test2 and FooTest.test3 will be considered as positive parts. So, our
current blacklisting code is broken for JVM tests becauze it adds only one '-'
per filter.

To find out what is wrong, this code, or may be some other (our) code, I have
read
https://code.google.com/p/googletest/wiki/AdvancedGuide#Running_a_Subset_of_t...
again and again and I see that '-' may be only one in filter string.

It say : "The format of a filter is a ':'-separated list of wildcard patterns
(called the positive patterns) optionally followed by a '-' and another
':'-separated pattern list (called the negative patterns)".
So, as I understand it, the format looks like "test1:test2:test3" or
"test1:test2:test3-test4:test5:test6". This is how I understand it. Where's the
truth?

p.s. so I asked it here to make descision what to fix. Our blacklisting code, or
JVM filtering code.
Thanks.

[kindrik, 2015-08-14 10:18:49]

> So, as I understand it, the format looks like "test1:test2:test3" or
> "test1:test2:test3-test4:test5:test6". This is how I understand it. Where's
the
> truth?
> 

Where test4, test5, test6 are negative filters.

[jbudorick, 2015-08-14 15:43:56]

https://codereview.chromium.org/574433003/diff/280001/testing/android/junit/j...
File testing/android/junit/java/src/org/chromium/testing/local/GtestFilter.java
(right):

https://codereview.chromium.org/574433003/diff/280001/testing/android/junit/j...
testing/android/junit/java/src/org/chromium/testing/local/GtestFilter.java:44:
int negIndex = sanitized.indexOf('-');
On 2015/08/14 at 10:15:43, kindrik wrote:
> Hi. Sorry, I have to add a fly in the ointment. But as I see this, I think an
error creeped in there.
> Your code first of all splits filterString to parts by colon (':').
> Then it checks each part for dash '-'. 
> This is fine for filters like this "--gtest_filter=FooTest.*-FooTest.Bar" but
> more complicated filters like
"--gtest_filter=FooTest.*-FooTest.test1:FooTest.test2:FooTest.test3" will be
treated incorrectly.
> FooTest.test2 and FooTest.test3 will be considered as positive parts. So, our
current blacklisting code is broken for JVM tests becauze it adds only one '-'
per filter.
> 
> To find out what is wrong, this code, or may be some other (our) code, I have
read
https://code.google.com/p/googletest/wiki/AdvancedGuide#Running_a_Subset_of_t...
again and again and I see that '-' may be only one in filter string.
> 
> It say : "The format of a filter is a ':'-separated list of wildcard patterns
(called the positive patterns) optionally followed by a '-' and another
':'-separated pattern list (called the negative patterns)".
> So, as I understand it, the format looks like "test1:test2:test3" or
"test1:test2:test3-test4:test5:test6". This is how I understand it. Where's the
truth?
> 
> p.s. so I asked it here to make descision what to fix. Our blacklisting code,
or JVM filtering code.
> Thanks.

Thanks for the report. This is definitely a bug with this code. I've filed
https://code.google.com/p/chromium/issues/detail?id=520997.