cleanup test flags

- We were using both caja.test.foo and test.foo properties. There
  isn't really a reason to require a caja. prefix, so I'm renaming
  them all to test.foo.

- I've renamed some of the existing test.foo properties for clarity.

- Added a typo check for test flags.

- test.filter.method is now really a glob. Before, 'x' would be
  interpreted as /^.*x.*$/ rather than /^x$/.

[kpreid2, 2013-04-22 16:49:30]

LGTM

https://codereview.appspot.com/8912043/diff/1/build.xml
File build.xml (right):

https://codereview.appspot.com/8912043/diff/1/build.xml#newcode364
build.xml:364: </javac>
Is there a reason this can't just be compiled by AllTests?

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
File tests/com/google/caja/util/TestFlag.java (right):

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlag.java:24: * See <a
href="https://code.google.com/p/google-caja/wiki/CajaTesting">CajaTesting</a>
long line -- you can break after the > without introducing whitespace in the
HTML interpretation

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlag.java:57: private static class Names {
Why is this an inner class? It doesn't appear to gain any laziness.

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlag.java:92: public boolean truthy() {
I'd like to see a finite set of values considered true, and others being an
error. This has the advantage that if we want to introduce non-true-or-false for
a formerly boolean flag we can do it soundly.

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
File tests/com/google/caja/util/TestFlagTypoCheck.java (right):

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlagTypoCheck.java:17: for (Object key :
getProject().getProperties().keySet()) {
I see we're checking these properties here, but not reading them as flags in
TestFlag. Do I correctly assume that what's going on is this is invoked as a
task, but the tests are run as a subprocess which gets the project properties
added to its system properties?

[felix8a, 2013-04-22 17:42:55]

https://codereview.appspot.com/8912043/diff/1/build.xml
File build.xml (right):

https://codereview.appspot.com/8912043/diff/1/build.xml#newcode364
build.xml:364: </javac>
On 2013/04/22 16:49:30, kpreid2 wrote:
> Is there a reason this can't just be compiled by AllTests?

it could, but I prefer putting related code together, and there's negligible
overhead in compiling it separately.

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
File tests/com/google/caja/util/TestFlag.java (right):

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlag.java:24: * See <a
href="https://code.google.com/p/google-caja/wiki/CajaTesting">CajaTesting</a>
On 2013/04/22 16:49:30, kpreid2 wrote:
> long line -- you can break after the > without introducing whitespace in the
> HTML interpretation

Done.

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlag.java:57: private static class Names {
On 2013/04/22 16:49:30, kpreid2 wrote:
> Why is this an inner class? It doesn't appear to gain any laziness.

Since the HashSet is used in the enum's constructor, the HashSet has to exist
before the first enum value is defined, and the simplest way to do that is to
use an inner class to hold the HashSet.

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlag.java:92: public boolean truthy() {
On 2013/04/22 16:49:30, kpreid2 wrote:
> I'd like to see a finite set of values considered true, and others being an
> error. This has the advantage that if we want to introduce non-true-or-false
for
> a formerly boolean flag we can do it soundly.

Done.

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
File tests/com/google/caja/util/TestFlagTypoCheck.java (right):

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlagTypoCheck.java:17: for (Object key :
getProject().getProperties().keySet()) {
On 2013/04/22 16:49:30, kpreid2 wrote:
> I see we're checking these properties here, but not reading them as flags in
> TestFlag. Do I correctly assume that what's going on is this is invoked as a
> task, but the tests are run as a subprocess which gets the project properties
> added to its system properties?

the ant junit task has a directive <sysproperty> that copies ant properties into
java system properties for the benefit of junit tests, which don't have an easy
way to get ant properties.

I'm kinda unhappy at how many places test flags come from and go to, but it's
hard to simplify it further.

[felix8a, 2013-04-22 17:43:24]

- We were using both caja.test.foo and test.foo properties. There
  isn't really a reason to require a caja. prefix, so I'm renaming
  them all to test.foo.

- I've renamed some of the existing test.foo properties for clarity.

- Added a typo check for test flags.

- test.filter.method is now really a glob. Before, 'x' would be
  interpreted as /^.*x.*$/ rather than /^x$/.

[kpreid_google, 2013-04-22 17:52:29]

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
File tests/com/google/caja/util/TestFlag.java (right):

https://codereview.appspot.com/8912043/diff/1/tests/com/google/caja/util/Test...
tests/com/google/caja/util/TestFlag.java:57: private static class Names {
On 2013/04/22 17:42:55, felix8a wrote:
> On 2013/04/22 16:49:30, kpreid2 wrote:
> > Why is this an inner class? It doesn't appear to gain any laziness.
> 
> Since the HashSet is used in the enum's constructor

...and the enum constructors are invoked before any other static fields, which
is the part I hadn't noticed.

> the HashSet has to exist
> before the first enum value is defined, and the simplest way to do that is to
> use an inner class to hold the HashSet.

First glance at research suggests that it's conventional to do the map
initialization as a separate static initializer:

http://stackoverflow.com/questions/443980/why-cant-enums-constructor-access-s...

But there's no strong reason to change it now.

[felix8a, 2013-04-22 19:17:06]

@r5364