Simplify skiatest framework.

dm/DM.cpp

Index: dm/DM.cpp
diff --git a/dm/DM.cpp b/dm/DM.cpp
index dad1f8b33fd9af1a0436586eaee35c06e3c6c863..7b9d3c4b7415f10148f43b43e535272dca9938e9 100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -12,6 +12,7 @@
 #include "SkTaskGroup.h"
 #include "Test.h"
 #include "Timer.h"
+#include "TempDir.h"
 
 DEFINE_bool(tests, true, "Run tests?");
 DEFINE_string(images, "resources", "Images to decode.");
@@ -29,7 +30,6 @@ DEFINE_string(blacklist, "",
         "'--blacklist gpu skp _' will blacklist all SKPs drawn into the gpu config.\n"
         "'--blacklist gpu skp _ 8888 gm aarects' will also blacklist the aarects GM on 8888.");
 
-
 __SK_FORCE_IMAGE_DECODER_LINKING;
 using namespace DM;
 
@@ -362,51 +362,57 @@ static void run_enclave(SkTArray<Task>* tasks) {
 
 // Unit tests don't fit so well into the Src/Sink model, so we give them special treatment.
 
-static struct : public skiatest::Reporter {
-    void onReportFailed(const skiatest::Failure& failure) SK_OVERRIDE {
-        SkString s;
-        failure.getFailureString(&s);
-        fail(s);
+class DMTestReporter : public skiatest::Reporter {
+public:
+    DMTestReporter() : fErrorCount(0) {}
+    int errorCount() const { return fErrorCount; }
+    void reportFailed(const skiatest::Failure& failure) SK_OVERRIDE {
+        ++fErrorCount;
+        fail(failure.toString());
         JsonWriter::AddTestFailure(failure);
     }
     bool allowExtendedTest() const SK_OVERRIDE { return FLAGS_pathOpsExtended; }
-    bool verbose()           const SK_OVERRIDE { return FLAGS_veryVerbose; }
-} gTestReporter;
+    bool verbose() const SK_OVERRIDE { return FLAGS_veryVerbose; }
+    SkString getTmpDir() const SK_OVERRIDE { return sk_tools::GetTmpDir(); }

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

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.

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

Done.

No longer virtual on reporter.

+
+private:
+    int fErrorCount;

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

Seems like this wants to be bool fPassed / fFailed?  What's the count get us?

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

Acknowledged.

+};
 
-static SkTArray<SkAutoTDelete<skiatest::Test>, kMemcpyOK> gCPUTests, gGPUTests;
+static SkTArray<skiatest::Test, kMemcpyOK> gCPUTests;

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

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?

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

Done.

+static SkTArray<skiatest::Test, kMemcpyOK> gGPUTests;
 
 static void gather_tests() {
     if (!FLAGS_tests) {
         return;
     }
-    for (const skiatest::TestRegistry* r = skiatest::TestRegistry::Head(); r; r = r->next()) {
-        SkAutoTDelete<skiatest::Test> test(r->factory()(NULL));
-        if (SkCommandLineFlags::ShouldSkip(FLAGS_match, test->getName())) {
+    for (const skiatest::TestRegistry* r = skiatest::TestRegistry::Head(); r;
+         r = r->next()) {
+        // Badly named function in SkTRegistry.

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

Might mention you're talking about factory()?

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

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

Done.

+        const skiatest::Test& test = r->factory();
+        if (SkCommandLineFlags::ShouldSkip(FLAGS_match, test.name)) {
             continue;
         }
-
-        test->setReporter(&gTestReporter);
-        if (test->isGPUTest() && gpu_supported()) {
-            gGPUTests.push_back().reset(test.detach());
-        } else if (!test->isGPUTest() && FLAGS_cpu) {
-            gCPUTests.push_back().reset(test.detach());
+        if (test.needsGpu && gpu_supported()) {
+            gGPUTests.push_back(test);
+        } else if (!test.needsGpu && FLAGS_cpu) {
+            gCPUTests.push_back(test);
         }
     }
 }
 
-static void run_test(SkAutoTDelete<skiatest::Test>* t) {
+static void run_test(skiatest::Test* test) {
     WallTimer timer;
     timer.start();
-    skiatest::Test* test = t->get();
     if (!FLAGS_dryRun) {
-        test->setGrContextFactory(GetThreadLocalGrContextFactory());
-        test->run();
-        if (!test->passed()) {
-            fail(SkStringPrintf("test %s failed", test->getName()));
+        DMTestReporter reporter;

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

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.

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

Done.

+        test->proc(&reporter, GetThreadLocalGrContextFactory());
+        if (0 != reporter.errorCount()) {

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

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.

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

Done.

+            fail(SkStringPrintf("test %s failed", test->name));
         }
     }
     timer.end();
-    done(timer.fWall, "unit", "test", test->getName());
+    done(timer.fWall, "unit", "test", test->name);
 }
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/