Simplify skiatest framework.

Simplify skiatest framework.

skiatest::Test class is now a simple struct.  Some
functionalty, such as counting errors or timing is now
handled elsewhere.

skiatest:Reporter is now a simpler abstract class.  The two
implementations handle test errors.

DM and pathops_unittest updated.

Committed: https://skia.googlesource.com/skia/+/87f3ba4847aa575016eb3a21e944197d757df8c0

[hal.canary, 2015-01-15 19:52:55]

halcanary@google.com changed reviewers:
+ mtklein@google.com

[hal.canary, 2015-01-15 19:52:56]

ptal

[mtklein, 2015-01-15 23:46:46]

big fan, many nits

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode26
dm/DM.cpp:26: DEFINE_string2(tmpDir, t, NULL, "tmp directory for tests to
use.");
Let's just default this to "/tmp" so we don't have to be so conditional below?

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode340
dm/DM.cpp:340: DMTestReporter() : fErrorCount(0) {
One line for each of these?

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode342
dm/DM.cpp:342: int errorCount() {
Might as well const?

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode357
dm/DM.cpp:357: int fErrorCount;
Where does this change?

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode368
dm/DM.cpp:368: const skiatest::Test& test = r->factory();
factory returns a const&?  That's at least weird naming.

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode382
dm/DM.cpp:382: static void run_test(skiatest::Test* testPtr) {
Let's just call this test and make test->foo() calls.

https://codereview.chromium.org/830513004/diff/1/tests/Test.cpp
File tests/Test.cpp (right):

https://codereview.chromium.org/830513004/diff/1/tests/Test.cpp#newcode15
tests/Test.cpp:15: using namespace skiatest;
Remove this or remove all the skiatest:: below?

https://codereview.chromium.org/830513004/diff/1/tests/Test.cpp#newcode17
tests/Test.cpp:17: skiatest::Reporter::~Reporter() {
Any reason we don't just put this little nothing destructor in the header?

https://codereview.chromium.org/830513004/diff/1/tests/Test.cpp#newcode34
tests/Test.cpp:34: //                           Reporter* reporter,
Yeah, die die die.

https://codereview.chromium.org/830513004/diff/1/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/830513004/diff/1/tests/Test.h#newcode29
tests/Test.h:29: SkString FailureToString(const skiatest::Failure& failure);
I don't really see the harm of this being a method?  Isn't failure.toString()
nicer to read than FailureToString(failure)?

https://codereview.chromium.org/830513004/diff/1/tests/Test.h#newcode34
tests/Test.h:34: virtual void bumpTestCount() {
One line for all these trivial guys?  Just a suggestion.

https://codereview.chromium.org/830513004/diff/1/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/830513004/diff/1/tests/skia_test.cpp#newcode4
tests/skia_test.cpp:4: * Use of this source code is governed by a BSD-style
license that can be
Just FYI, I'm going to ignore this file.  If it compiles, that's good by me.

[hal.canary, 2015-01-15 23:55:14]

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode357
dm/DM.cpp:357: int fErrorCount;
On 2015/01/15 23:46:46, mtklein wrote:
> Where does this change?

oops

[hal.canary, 2015-01-16 15:31:58]

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode26
dm/DM.cpp:26: DEFINE_string2(tmpDir, t, NULL, "tmp directory for tests to
use.");
On 2015/01/15 23:46:46, mtklein wrote:
> Let's just default this to "/tmp" so we don't have to be so conditional below?

That's not cross-platform.  I've added complete tempdir logic to tools/.

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode340
dm/DM.cpp:340: DMTestReporter() : fErrorCount(0) {
On 2015/01/15 23:46:46, mtklein wrote:
> One line for each of these?

I just let clang-format do its magic.

Changing AllowShortFunctionsOnASingleLine to true.

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode342
dm/DM.cpp:342: int errorCount() {
On 2015/01/15 23:46:46, mtklein wrote:
> Might as well const?

Done.

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode368
dm/DM.cpp:368: const skiatest::Test& test = r->factory();
On 2015/01/15 23:46:46, mtklein wrote:
> factory returns a const&?  That's at least weird naming.

You can register anything with a SkTRegistry, but the function name to get the
thing assumes it's a factory.

https://codereview.chromium.org/830513004/diff/1/dm/DM.cpp#newcode382
dm/DM.cpp:382: static void run_test(skiatest::Test* testPtr) {
On 2015/01/15 23:46:46, mtklein wrote:
> Let's just call this test and make test->foo() calls.

Done.

https://codereview.chromium.org/830513004/diff/1/tests/Test.cpp
File tests/Test.cpp (right):

https://codereview.chromium.org/830513004/diff/1/tests/Test.cpp#newcode15
tests/Test.cpp:15: using namespace skiatest;
On 2015/01/15 23:46:46, mtklein wrote:
> Remove this or remove all the skiatest:: below?

Done.

https://codereview.chromium.org/830513004/diff/1/tests/Test.cpp#newcode17
tests/Test.cpp:17: skiatest::Reporter::~Reporter() {
On 2015/01/15 23:46:46, mtklein wrote:
> Any reason we don't just put this little nothing destructor in the header?

Done.

https://codereview.chromium.org/830513004/diff/1/tests/Test.cpp#newcode34
tests/Test.cpp:34: //                           Reporter* reporter,
On 2015/01/15 23:46:46, mtklein wrote:
> Yeah, die die die.

Done.

https://codereview.chromium.org/830513004/diff/1/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/830513004/diff/1/tests/Test.h#newcode29
tests/Test.h:29: SkString FailureToString(const skiatest::Failure& failure);
On 2015/01/15 23:46:46, mtklein wrote:
> I don't really see the harm of this being a method?  Isn't failure.toString()
> nicer to read than FailureToString(failure)?

Done.

https://codereview.chromium.org/830513004/diff/1/tests/Test.h#newcode34
tests/Test.h:34: virtual void bumpTestCount() {
On 2015/01/15 23:46:46, mtklein wrote:
> One line for all these trivial guys?  Just a suggestion.

Done (AllowShortFunctionsOnASingleLine: true).

https://codereview.chromium.org/830513004/diff/1/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/830513004/diff/1/tests/skia_test.cpp#newcode4
tests/skia_test.cpp:4: * Use of this source code is governed by a BSD-style
license that can be
On 2015/01/15 23:46:46, mtklein wrote:
> Just FYI, I'm going to ignore this file.  If it compiles, that's good by me.

Acknowledged.

[mtklein, 2015-01-18 22:57:55]

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode376
dm/DM.cpp:376: SkString getTmpDir() const SK_OVERRIDE { return
sk_tools::GetTmpDir(); }
Why don't we get this out of reporter and have the tests that need it call
GetTmpDir directly?  This doesn't seem like something any particular reporter
would care about.

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode379
dm/DM.cpp:379: int fErrorCount;
Seems like this wants to be bool fPassed / fFailed?  What's the count get us?

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode382
dm/DM.cpp:382: static SkTArray<skiatest::Test, kMemcpyOK> gCPUTests;
Go back to one line?  Having these as separate declarations makes it read like
they've got separate types.  Something like
  static SkTArray<skiatest::Test, kMemcpyOK> gCPUTests,
                                             gGPUTests;
seems fine too but pointlessly profligate with lines of code.

We probably don't need kMemcpyOK anymore now that skiatest::Tests are static
structs?  Let's switch to SkTDArray to make that stand out even more that it's
all POD?

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode391
dm/DM.cpp:391: // Badly named function in SkTRegistry.
Might mention you're talking about factory()?

// Despite its name, factory() is returning a reference to link-time static
const POD data.

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode408
dm/DM.cpp:408: DMTestReporter reporter;
I'd prefer

struct : public skiatest::Reporter {
...
} reporter;

here, to keep the code close together and to scope that DMTestReporter so finely
it doesn't even need a name.

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode410
dm/DM.cpp:410: if (0 != reporter.errorCount()) {
Why don't we drop this instead of having to track errorCount()?  The individual
unit test assertions will already have printed out (the call to fail() in
reportFailed, so having an extra summary "something in this test failed" seems
between negligible value and misleading.  If we're going through extra work to
support it, let's drop it instead.

https://codereview.chromium.org/830513004/diff/20001/gyp/tools.gyp
File gyp/tools.gyp (right):

https://codereview.chromium.org/830513004/diff/20001/gyp/tools.gyp#newcode622
gyp/tools.gyp:622: 'target_name': 'temp_directory',
If this sticks, let's call it temp_dir, tempdir or TempDir?  Something to match
the file names.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp
File tools/TempDir.cpp (right):

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode15
tools/TempDir.cpp:15: DEFINE_string2(tmpDir, t, NULL, "Temp directory to use.");
It doesn't look like anyone sets this flag.

So, at the very least, let's delete the flag.  I'd also strongly consider
deleting all this code and all the tests that appear to use the results of
GetTmpDir() today.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode20
tools/TempDir.cpp:20: //
https://docs.python.org/2/library/tempfile.html#tempfile.tempdir
Let's not do this.  This is way more complicated than it needs to be, which at
the very mildest is going to make someone confused over why we wrote this code.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode23
tools/TempDir.cpp:23: if (nonempty_string(tmpDir)) {
This seems like a weird way to write

if (!FLAGS_tmpDir.isEmpty()) {
  return SkString(FLAGS_tmpDir[0]);
}

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode27
tools/TempDir.cpp:27: const char* const kVariables[] = {"TMPDIR", "TEMP",
"TMP"};
Why are we getting so complicated with features no one will ever use?

#if SK_BUILD_FOR_WIN
  return "C:\\TEMP";  (Or whatever the normal Windows temp dir is.)
#else
  return "/tmp";
#endif

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode47
tools/TempDir.cpp:47: return SkString(".");
This seems like a bad idea.  I'd rather return no temporary directory than say
the cwd is a temporary directory.  If we write to ., no one will ever clean that
up.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.h
File tools/TempDir.h (right):

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.h#newcode14
tools/TempDir.h:14: SkString GetTmpDir();
I'm sort of skeptical that this is a feature worth writing code over.  Are you
sure the couple tests that ostensibly want this can't just get what they need
out of tmpfile() or tmpnam()?

[hal.canary, 2015-01-20 16:01:52]

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode376
dm/DM.cpp:376: SkString getTmpDir() const SK_OVERRIDE { return
sk_tools::GetTmpDir(); }
On 2015/01/18 22:57:55, mtklein wrote:
> Why don't we get this out of reporter and have the tests that need it call
> GetTmpDir directly?  This doesn't seem like something any particular reporter
> would care about.

Done.

No longer virtual on reporter.

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode379
dm/DM.cpp:379: int fErrorCount;
On 2015/01/18 22:57:55, mtklein wrote:
> Seems like this wants to be bool fPassed / fFailed?  What's the count get us?

Acknowledged.

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode382
dm/DM.cpp:382: static SkTArray<skiatest::Test, kMemcpyOK> gCPUTests;
On 2015/01/18 22:57:55, mtklein wrote:
> Go back to one line?  Having these as separate declarations makes it read like
> they've got separate types.  Something like
>   static SkTArray<skiatest::Test, kMemcpyOK> gCPUTests,
>                                              gGPUTests;
> seems fine too but pointlessly profligate with lines of code.
> 
> We probably don't need kMemcpyOK anymore now that skiatest::Tests are static
> structs?  Let's switch to SkTDArray to make that stand out even more that it's
> all POD?

Done.

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode391
dm/DM.cpp:391: // Badly named function in SkTRegistry.
On 2015/01/18 22:57:55, mtklein wrote:
> Might mention you're talking about factory()?
> 
> // Despite its name, factory() is returning a reference to link-time static
> const POD data.

Done.

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode408
dm/DM.cpp:408: DMTestReporter reporter;
On 2015/01/18 22:57:55, mtklein wrote:
> I'd prefer
> 
> struct : public skiatest::Reporter {
> ...
> } reporter;
> 
> here, to keep the code close together and to scope that DMTestReporter so
finely
> it doesn't even need a name.

Done.

https://codereview.chromium.org/830513004/diff/20001/dm/DM.cpp#newcode410
dm/DM.cpp:410: if (0 != reporter.errorCount()) {
On 2015/01/18 22:57:55, mtklein wrote:
> Why don't we drop this instead of having to track errorCount()?  The
individual
> unit test assertions will already have printed out (the call to fail() in
> reportFailed, so having an extra summary "something in this test failed" seems
> between negligible value and misleading.  If we're going through extra work to
> support it, let's drop it instead.

Done.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp
File tools/TempDir.cpp (right):

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode15
tools/TempDir.cpp:15: DEFINE_string2(tmpDir, t, NULL, "Temp directory to use.");
On 2015/01/18 22:57:55, mtklein wrote:
> It doesn't look like anyone sets this flag.
> 
> So, at the very least, let's delete the flag.  I'd also strongly consider
> deleting all this code and all the tests that appear to use the results of
> GetTmpDir() today.

Done.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode20
tools/TempDir.cpp:20: //
https://docs.python.org/2/library/tempfile.html#tempfile.tempdir
On 2015/01/18 22:57:55, mtklein wrote:
> Let's not do this.  This is way more complicated than it needs to be, which at
> the very mildest is going to make someone confused over why we wrote this
code.

Done.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode23
tools/TempDir.cpp:23: if (nonempty_string(tmpDir)) {
On 2015/01/18 22:57:55, mtklein wrote:
> This seems like a weird way to write
> 
> if (!FLAGS_tmpDir.isEmpty()) {
>   return SkString(FLAGS_tmpDir[0]);
> }

Acknowledged.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode27
tools/TempDir.cpp:27: const char* const kVariables[] = {"TMPDIR", "TEMP",
"TMP"};
On 2015/01/18 22:57:55, mtklein wrote:
> Why are we getting so complicated with features no one will ever use?
> 
> #if SK_BUILD_FOR_WIN
>   return "C:\\TEMP";  (Or whatever the normal Windows temp dir is.)
> #else
>   return "/tmp";
> #endif

Acknowledged.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.cpp#newcode47
tools/TempDir.cpp:47: return SkString(".");
On 2015/01/18 22:57:55, mtklein wrote:
> This seems like a bad idea.  I'd rather return no temporary directory than say
> the cwd is a temporary directory.  If we write to ., no one will ever clean
that
> up.

Acknowledged.

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.h
File tools/TempDir.h (right):

https://codereview.chromium.org/830513004/diff/20001/tools/TempDir.h#newcode14
tools/TempDir.h:14: SkString GetTmpDir();
On 2015/01/18 22:57:55, mtklein wrote:
> I'm sort of skeptical that this is a feature worth writing code over.  Are you
> sure the couple tests that ostensibly want this can't just get what they need
> out of tmpfile() or tmpnam()?

removed.

[mtklein, 2015-01-20 16:16:54]

lgtm

https://codereview.chromium.org/830513004/diff/40001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/830513004/diff/40001/dm/DM.cpp#newcode393
dm/DM.cpp:393: return FLAGS_pathOpsExtended;
One line?

https://codereview.chromium.org/830513004/diff/40001/dm/DM.cpp#newcode397
dm/DM.cpp:397: reporter;
usually I put these on the same line as the }

https://codereview.chromium.org/830513004/diff/40001/tests/Test.cpp
File tests/Test.cpp (right):

https://codereview.chromium.org/830513004/diff/40001/tests/Test.cpp#newcode24
tests/Test.cpp:24: const skiatest::Failure& failure = *this;
?

'this->' is shorter than 'failure.', and I think clearer too.

[hal.canary, 2015-01-20 16:21:51]

https://codereview.chromium.org/830513004/diff/40001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/830513004/diff/40001/dm/DM.cpp#newcode393
dm/DM.cpp:393: return FLAGS_pathOpsExtended;
On 2015/01/20 16:16:53, mtklein wrote:
> One line?

I'll defer to clang-format

https://codereview.chromium.org/830513004/diff/40001/dm/DM.cpp#newcode397
dm/DM.cpp:397: reporter;
On 2015/01/20 16:16:53, mtklein wrote:
> usually I put these on the same line as the }

Done.

https://codereview.chromium.org/830513004/diff/40001/tests/Test.cpp
File tests/Test.cpp (right):

https://codereview.chromium.org/830513004/diff/40001/tests/Test.cpp#newcode24
tests/Test.cpp:24: const skiatest::Failure& failure = *this;
On 2015/01/20 16:16:54, mtklein wrote:
> ?
> 
> 'this->' is shorter than 'failure.', and I think clearer too.

Done.

[hal.canary, 2015-01-20 16:21:58]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-20 16:22:25]

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

[commit-bot: I haz the power, 2015-01-20 16:23:46]

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

[commit-bot: I haz the power, 2015-01-20 16:23:47]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[hal.canary, 2015-01-20 16:50:13]

Patchset #5 (id:80001) has been deleted

[hal.canary, 2015-01-20 16:50:24]

Patchset #4 (id:60001) has been deleted

[hal.canary, 2015-01-20 17:27:03]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-01-20 17:27:36]

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

[commit-bot: I haz the power, 2015-01-20 17:30:24]

Committed patchset #4 (id:100001) as
https://skia.googlesource.com/skia/+/87f3ba4847aa575016eb3a21e944197d757df8c0