Use DEFINE_TESTCLASS_SHORT macro in tests.

Use DEFINE_TESTCLASS_SHORT macro in tests.

The three version of DEFINE_TESTCLASS macro is deprecated and thus just
use the simple, short one.

BUG=None
TEST=out/Debug/tests
R=mtklein@google.com, bsalomon@google.com, robertphillips@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12653

[tfarina, 2013-12-07 03:16:29]

Found another cleanup task ;)

In the sound of skrillex...

I left these two tests/ImageFilterTest.cpp, tests/TileGridTest.cpp for now.
After they are fixed, we can remove the DEFINE_TESTCLASS macro.

Please, take a look.

Thanks!

[mtklein, 2013-12-07 10:30:46]

I like where this is going.  One naming request for continuity.  Please apply it
everywhere you can.

If you're still feeling like refactoring after this, I'd suggest breaking up
DEFINE_TESTCLASS_SHORT(Foo) into DEF_TEST(Foo_Bar, reporter) { ... },
DEF_TEST(Foo_Baz, reporter) { ... }, etc, where it makes sense.

E.g. AnnotationTest.cpp might look like

#include "Test.h"
#include "TestClassDef.h"

static bool ContainsString(...) { ... }

DEF_TEST(Annotation_NoDraw, reporter {
  SkBitmap bm;
  bm.setConfig(SkBitmap::kARGB_8888_Config, 10, 10);
  ...
  REPORTER_ASSERT(reporter, 0 == *bm.getAddr32(0, 0));
}

struct testCase { ... };

DEF_TEST(Annotation_PdfLink, reporter) {
   ...
}

DEF_TEST(Annotation_NamedDestination, reporter) {
   ...
}

And that's it.  No static void TestAnnotation, no DEFINE_TESTCLASS_SHORT.  (Also
might want to have Test.h #include TestClassDef.h or move the definitions into
there.)

https://codereview.chromium.org/100113004/diff/150001/tests/AnnotationTest.cpp
File tests/AnnotationTest.cpp (right):

https://codereview.chromium.org/100113004/diff/150001/tests/AnnotationTest.cp...
tests/AnnotationTest.cpp:105: DEFINE_TESTCLASS_SHORT(TestAnnotation)
Generally, for some continuity, it'd be nice to replace
  DEFINE_TESTCLASS("Foo", ...)
with
  DEFINE_TESTCLASS_SHORT(Foo).

You can see the effect by running tests before and after this change.  Many
names have changed.

[tfarina, 2013-12-07 13:20:23]

https://codereview.chromium.org/100113004/diff/150001/tests/AnnotationTest.cpp
File tests/AnnotationTest.cpp (right):

https://codereview.chromium.org/100113004/diff/150001/tests/AnnotationTest.cp...
tests/AnnotationTest.cpp:105: DEFINE_TESTCLASS_SHORT(TestAnnotation)
On 2013/12/07 10:30:46, mtklein wrote:
> Generally, for some continuity, it'd be nice to replace
>   DEFINE_TESTCLASS("Foo", ...)
> with
>   DEFINE_TESTCLASS_SHORT(Foo).
> 
> You can see the effect by running tests before and after this change.  Many
> names have changed.

Done.

[mtklein, 2013-12-10 15:04:57]

On 2013/12/07 13:20:23, tfarina wrote:
> https://codereview.chromium.org/100113004/diff/150001/tests/AnnotationTest.cpp
> File tests/AnnotationTest.cpp (right):
> 
>
https://codereview.chromium.org/100113004/diff/150001/tests/AnnotationTest.cp...
> tests/AnnotationTest.cpp:105: DEFINE_TESTCLASS_SHORT(TestAnnotation)
> On 2013/12/07 10:30:46, mtklein wrote:
> > Generally, for some continuity, it'd be nice to replace
> >   DEFINE_TESTCLASS("Foo", ...)
> > with
> >   DEFINE_TESTCLASS_SHORT(Foo).
> > 
> > You can see the effect by running tests before and after this change.  Many
> > names have changed.
> 
> Done.

Thanks Thiago.  This lgtm, modulo AtomicAdd's name.  

Before submitting, can I interest you in one more refactoring?  (If not, I'll do
it.)  I like the style you've got now in AnnotationTest the best, but I
recognize that moving to it everywhere is a pretty tedious and sometimes
non-trivial refactoring.  As a half-way point, most of the tests now look like
this:
  static void ThingWeWantToTest(skiatest::Reporter* reporter) {
     test_something(reporter);
     test_other_thing(reporter);
  }
  ...  (maybe many lines)
  DEFINE_TESTCLASS_SHORT(ThingWeWantToTest)

I was thinking it might be nice before submitting to convert these to:
  DEF_TEST(ThingWeWantToTest, reporter) {
      test_something(reporter);
      test_other_thing(reporter);
  }

What do you think?  It's sort of half-way closer to the goal of this:
  DEF_TEST(ThingWeWantToTest_something, reporter) {
      ...
  }

  DEF_TEST(ThingWeWantToTest_other_thing, reporter) {
      ...
  }

[tfarina, 2013-12-11 00:12:43]

If it wasn't vim... ;)

Thanks to vim + i3 keystrokes that was possible.

My left hand is hurting...heh

Mike, please, could you take another look?

Thanks!

[mtklein, 2013-12-11 19:32:31]

On 2013/12/11 00:12:43, tfarina wrote:
> If it wasn't vim... ;)
> 
> Thanks to vim + i3 keystrokes that was possible.
> 
> My left hand is hurting...heh
> 
> Mike, please, could you take another look?
> 
> Thanks!

Just saw two missed opportunities.  Please update them before submitting.  This
LGTM very much.

Whenever I find myself in a situation like this, I find myself thinking hard and
then mapping some very complicated string of vim commands to ",".  :)

[tfarina, 2013-12-12 08:49:48]

I fixed the two missed opportunities (AtomicTest.cpp and MathTest.cpp) and
posted here.

Thanks for pushing in the review!

We came to a better result in the end :)

After this lands I plan to go and fix the remaining DEFINE_TESTCLASS_SHORT
usage.

Cheers,

[mtklein, 2013-12-12 14:42:16]

On 2013/12/12 08:49:48, tfarina wrote:
> I fixed the two missed opportunities (AtomicTest.cpp and MathTest.cpp) and
> posted here.
> 
> Thanks for pushing in the review!
> 
> We came to a better result in the end :)
> 
> After this lands I plan to go and fix the remaining DEFINE_TESTCLASS_SHORT
> usage.
> 
> Cheers,

Whoops, so sorry, just noticed that I didn't send the comments tagging
AtomicTest and MathTest as those two places to fix.  Didn't mean to make you
hunt them down!

LGTM @26

[tfarina, 2013-12-12 21:13:03]

Committed patchset #27 manually as r12653 (presubmit successful).