A bunch of IDE warning fixes

I got cranky with too many warnings in Eclipse.

I probably have a different set of warnings on than others,
so if this is too much to review just let me know.

It should have no effect on program semantics.

---

Submitted @ r5539

[felix8a, 2013-08-07 20:12:38]

lgtm, but I'm a little confused by the random static-ifying of test methods.

I mean, clearly it's not possible to make all test methods static, but the mix
of static and non-static ends up looking random, and it's not particularly
obvious which test methods can't be static, since various assertFoo methods
might require instance setup.

What warning is that addressing?

[kpreid_google, 2013-08-07 20:24:51]

Generally, I'm not sure that making everything static just because it _can_ be
static is most appropriate.

Below are a couple of cases where I think there's actually a problem:

https://codereview.appspot.com/12626043/diff/1/src/com/google/caja/parser/css...
File src/com/google/caja/parser/css/CssTree.java (right):

https://codereview.appspot.com/12626043/diff/1/src/com/google/caja/parser/css...
src/com/google/caja/parser/css/CssTree.java:1173: public static String suffix()
{
This looks like it's intended to be an override point.

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
File tests/com/google/caja/ancillary/linter/ErrorReporterTest.java (right):

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
tests/com/google/caja/ancillary/linter/ErrorReporterTest.java:52: @Test
This is mixing JUnit 4 and JUnit 3 style — are you sure this test actually runs,
now? Last I looked, anything which is a TestCase and not otherwise annotated at
the class level is run in JUnit 3 mode, and the method-level annotations are
then irrelevant.

I'm not sure this is a good idea even if it works.

[MikeSamuel, 2013-08-07 20:24:59]

2013/8/7  <felix8a@gmail.com>:
> lgtm, but I'm a little confused by the random static-ifying of test
> methods.
>
> I mean, clearly it's not possible to make all test methods static, but
> the mix of static and non-static ends up looking random, and it's not
> particularly obvious which test methods can't be static, since various
> assertFoo methods might require instance setup.
>
> What warning is that addressing?

Yeah.  I really wish Eclipse had a sane/usable mechanism for filtering
warnings.
That's addressing a warning

    The method ... from the type ... can be declared static

which can be enabled via

    Eclipse :: Preferences :: Java :: Compiler :: Errors/Warnings ::
Code Style :: Method can be static

and the similarly named

    ... :: Method can potentially be static

I find it useful, because sometimes making one method static leads to
a cascade of similar changes, and then you realize you didn't need an
instance.

But for methods that are entry points reached via reflection, it's a
big source of garbage.

> https://codereview.appspot.com/12626043/

[MikeSamuel, 2013-08-07 20:31:26]

https://codereview.appspot.com/12626043/diff/1/src/com/google/caja/parser/css...
File src/com/google/caja/parser/css/CssTree.java (right):

https://codereview.appspot.com/12626043/diff/1/src/com/google/caja/parser/css...
src/com/google/caja/parser/css/CssTree.java:1173: public static String suffix()
{
On 2013/08/07 20:24:51, kpreid_google wrote:
> This looks like it's intended to be an override point.

I thought thats true, but suffixed identifiers are just placeholders in the code
that are used by an earlier pass to make clear to a later pass, which parts
really need to be changed.

This class is never actually overridden.

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
File tests/com/google/caja/ancillary/linter/ErrorReporterTest.java (right):

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
tests/com/google/caja/ancillary/linter/ErrorReporterTest.java:52: @Test
On 2013/08/07 20:24:51, kpreid_google wrote:
> This is mixing JUnit 4 and JUnit 3 style — are you sure this test actually
runs,
> now? Last I looked, anything which is a TestCase and not otherwise annotated
at
> the class level is run in JUnit 3 mode, and the method-level annotations are
> then irrelevant.

It runs for me in Eclipse and via the ant testrunner.  JUnit's code is such an
agile-development spaghetti minefield that I have no confidence that it would
run under all test runners.

It occurs to me now that since static methods do not mask one another, it's
possible that making all test methods static that could be static could cause
extra tests to run.

> I'm not sure this is a good idea even if it works.

[kpreid_google, 2013-08-07 20:34:57]

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
File tests/com/google/caja/ancillary/linter/ErrorReporterTest.java (right):

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
tests/com/google/caja/ancillary/linter/ErrorReporterTest.java:52: @Test
On 2013/08/07 20:31:26, MikeSamuel wrote:
> It runs for me in Eclipse and via the ant testrunner.  

Good enough.

> It occurs to me now that since static methods do not mask one another, it's
> possible that making all test methods static that could be static could cause
> extra tests to run.

My understanding is that it should be the case that
- all *TestCase contain no test*
- all *Test extend a *TestCase, not another *Test
so such shadowing should never occur in a properly structured test suite.

[felix8a, 2013-08-07 20:36:30]

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
File tests/com/google/caja/ancillary/linter/ErrorReporterTest.java (right):

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
tests/com/google/caja/ancillary/linter/ErrorReporterTest.java:52: @Test
On 2013/08/07 20:31:26, MikeSamuel wrote:
> It occurs to me now that since static methods do not mask one another, it's
> possible that making all test methods static that could be static could cause
> extra tests to run.

I don't see how that could happen. Any sane testrunner enumerates all test
methods once and then runs each test from that list once.

[MikeSamuel, 2013-08-07 20:45:23]

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
File tests/com/google/caja/ancillary/linter/ErrorReporterTest.java (right):

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
tests/com/google/caja/ancillary/linter/ErrorReporterTest.java:52: @Test
On 2013/08/07 20:36:30, felix8a wrote:
> On 2013/08/07 20:31:26, MikeSamuel wrote:
> > It occurs to me now that since static methods do not mask one another, it's
> > possible that making all test methods static that could be static could
cause
> > extra tests to run.
> 
> I don't see how that could happen. Any sane testrunner enumerates all test
> methods once and then runs each test from that list once.

Consider

    abstract class BaseTest { public void testFoo() { .. .} }

    class ConcreteTest extends BaseTest {
      public void testBar() { ... }
    }

A sane test runner might enumerate testFoo when inspecting ConcreteTest since it
is inherited.
But static methods aren't inherited, so would the same test runner come to the
same conclusion if testFoo were static?

[felix8a, 2013-08-07 21:29:01]

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
File tests/com/google/caja/ancillary/linter/ErrorReporterTest.java (right):

https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
tests/com/google/caja/ancillary/linter/ErrorReporterTest.java:52: @Test
On 2013/08/07 20:45:24, MikeSamuel wrote:
> On 2013/08/07 20:36:30, felix8a wrote:
> > On 2013/08/07 20:31:26, MikeSamuel wrote:
> > > It occurs to me now that since static methods do not mask one another,
it's
> > > possible that making all test methods static that could be static could
> cause
> > > extra tests to run.
> > 
> > I don't see how that could happen. Any sane testrunner enumerates all test
> > methods once and then runs each test from that list once.
> 
> Consider
> 
>     abstract class BaseTest { public void testFoo() { .. .} }
> 
>     class ConcreteTest extends BaseTest {
>       public void testBar() { ... }
>     }
> 
> A sane test runner might enumerate testFoo when inspecting ConcreteTest since
it
> is inherited.
> But static methods aren't inherited, so would the same test runner come to the
> same conclusion if testFoo were static?

hm, ok, if BaseTest is not a testsuite class but ConcreteTest is, then making
testFoo static will effectively remove it from a testrun, and that wouldn't be
noticeable without careful examination of the test results.

ok, so maybe @SuppressWarnings("static-method") to the test classes?

[MikeSamuel, 2013-08-07 22:08:12]

On 2013/08/07 21:29:01, felix8a wrote:
>
https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
> File tests/com/google/caja/ancillary/linter/ErrorReporterTest.java (right):
> 
>
https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
> tests/com/google/caja/ancillary/linter/ErrorReporterTest.java:52: @Test
> On 2013/08/07 20:45:24, MikeSamuel wrote:
> > On 2013/08/07 20:36:30, felix8a wrote:
> > > On 2013/08/07 20:31:26, MikeSamuel wrote:
> > > > It occurs to me now that since static methods do not mask one another,
> it's
> > > > possible that making all test methods static that could be static could
> > cause
> > > > extra tests to run.
> > > 
> > > I don't see how that could happen. Any sane testrunner enumerates all test
> > > methods once and then runs each test from that list once.
> > 
> > Consider
> > 
> >     abstract class BaseTest { public void testFoo() { .. .} }
> > 
> >     class ConcreteTest extends BaseTest {
> >       public void testBar() { ... }
> >     }
> > 
> > A sane test runner might enumerate testFoo when inspecting ConcreteTest
since
> it
> > is inherited.
> > But static methods aren't inherited, so would the same test runner come to
the
> > same conclusion if testFoo were static?
> 
> hm, ok, if BaseTest is not a testsuite class but ConcreteTest is, then making
> testFoo static will effectively remove it from a testrun, and that wouldn't be
> noticeable without careful examination of the test results.
> 
> ok, so maybe @SuppressWarnings("static-method") to the test classes?

This is such an excellent suggestion that I'm kicking myself for not thinking of
it.
I'll just blanket add it to all of the *Test classes, rollback all the static
changes to test classes and re-snapshot.

[MikeSamuel, 2013-08-07 22:35:43]

I got cranky with too many warnings in Eclipse.

I probably have a different set of warnings on than others,
so if this is too much to review just let me know.

It should have no effect on program semantics.

[MikeSamuel, 2013-08-07 22:35:48]

On 2013/08/07 22:08:12, MikeSamuel wrote:
> On 2013/08/07 21:29:01, felix8a wrote:
> >
>
https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
> > File tests/com/google/caja/ancillary/linter/ErrorReporterTest.java (right):
> > 
> >
>
https://codereview.appspot.com/12626043/diff/1/tests/com/google/caja/ancillar...
> > tests/com/google/caja/ancillary/linter/ErrorReporterTest.java:52: @Test
> > On 2013/08/07 20:45:24, MikeSamuel wrote:
> > > On 2013/08/07 20:36:30, felix8a wrote:
> > > > On 2013/08/07 20:31:26, MikeSamuel wrote:
> > > > > It occurs to me now that since static methods do not mask one another,
> > it's
> > > > > possible that making all test methods static that could be static
could
> > > cause
> > > > > extra tests to run.
> > > > 
> > > > I don't see how that could happen. Any sane testrunner enumerates all
test
> > > > methods once and then runs each test from that list once.
> > > 
> > > Consider
> > > 
> > >     abstract class BaseTest { public void testFoo() { .. .} }
> > > 
> > >     class ConcreteTest extends BaseTest {
> > >       public void testBar() { ... }
> > >     }
> > > 
> > > A sane test runner might enumerate testFoo when inspecting ConcreteTest
> since
> > it
> > > is inherited.
> > > But static methods aren't inherited, so would the same test runner come to
> the
> > > same conclusion if testFoo were static?
> > 
> > hm, ok, if BaseTest is not a testsuite class but ConcreteTest is, then
making
> > testFoo static will effectively remove it from a testrun, and that wouldn't
be
> > noticeable without careful examination of the test results.
> > 
> > ok, so maybe @SuppressWarnings("static-method") to the test classes?
> 
> This is such an excellent suggestion that I'm kicking myself for not thinking
of
> it.
> I'll just blanket add it to all of the *Test classes, rollback all the static
> changes to test classes and re-snapshot.

snapshotted.  Will add suppress annotations in a separate (and much more
formulaic) CL.

[felix8a, 2013-08-07 23:32:58]

lgtm except for one you missed

https://codereview.appspot.com/12626043/diff/14001/tests/com/google/caja/rend...
File tests/com/google/caja/render/CssPrettyPrinterTest.java (right):

https://codereview.appspot.com/12626043/diff/14001/tests/com/google/caja/rend...
tests/com/google/caja/render/CssPrettyPrinterTest.java:23: public static final
void testRender() {
undo this

https://codereview.appspot.com/12626043/diff/14001/tests/com/google/caja/rend...
tests/com/google/caja/render/CssPrettyPrinterTest.java:31: public static final
void testDeclarationGroup() {
and this

[MikeSamuel, 2013-08-08 00:35:31]

On 2013/08/07 23:32:58, felix8a wrote:
> lgtm except for one you missed
> 
>
https://codereview.appspot.com/12626043/diff/14001/tests/com/google/caja/rend...
> File tests/com/google/caja/render/CssPrettyPrinterTest.java (right):
> 
>
https://codereview.appspot.com/12626043/diff/14001/tests/com/google/caja/rend...
> tests/com/google/caja/render/CssPrettyPrinterTest.java:23: public static final
> void testRender() {
> undo this

Done

>
https://codereview.appspot.com/12626043/diff/14001/tests/com/google/caja/rend...
> tests/com/google/caja/render/CssPrettyPrinterTest.java:31: public static final
> void testDeclarationGroup() {
> and this

Done

[MikeSamuel, 2013-08-08 00:36:54]

I got cranky with too many warnings in Eclipse.

I probably have a different set of warnings on than others,
so if this is too much to review just let me know.

It should have no effect on program semantics.