Simplify skiatest framework.

dm/DM.cpp

Index: dm/DM.cpp
diff --git a/dm/DM.cpp b/dm/DM.cpp
index df7c32f9e7bc91d28608148f94bb50ffd1f5ddd7..9d4d2be53f6cc37e8a2aa06436b5e3516e12c0d2 100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -23,6 +23,7 @@ DEFINE_bool(nameByHash, false,
 DEFINE_bool2(pathOpsExtended, x, false, "Run extended pathOps tests.");
 DEFINE_string(matrix, "1 0 0 0 1 0 0 0 1",
               "Matrix to apply when using 'matrix' in config.");
+DEFINE_string2(tmpDir, t, NULL, "tmp directory for tests to use.");

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

Let's just default this to "/tmp" so we don't have to be so conditional below?

[hal.canary, 2015/01/16 15:31:57]

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

 
 __SK_FORCE_IMAGE_DECODER_LINKING;
 using namespace DM;
@@ -334,53 +335,64 @@ 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) {

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

One line for each of these?

[hal.canary, 2015/01/16 15:31:57]

I just let clang-format do its magic.

Changing AllowShortFunctionsOnASingleLine to true.

+    }
+    int errorCount() {

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

Might as well const?

[hal.canary, 2015/01/16 15:31:57]

Done.

+        return fErrorCount;
+    }
+    void reportFailed(const skiatest::Failure& failure) SK_OVERRIDE {
+        fail(skiatest::FailureToString(failure));
         JsonWriter::AddTestFailure(failure);
     }
     bool allowExtendedTest() const SK_OVERRIDE { return FLAGS_pathOpsExtended; }
     bool verbose()           const SK_OVERRIDE { return FLAGS_veryVerbose; }
-} gTestReporter;
+    SkString getTmpDir() SK_OVERRIDE {
+        const char* tmpDir = FLAGS_tmpDir.isEmpty() ? NULL : FLAGS_tmpDir[0];
+        return SkString(tmpDir);
+    }
+
+private:
+    int fErrorCount;

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

Where does this change?

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

oops

+};
 
-static SkTArray<SkAutoTDelete<skiatest::Test>, kMemcpyOK> gTests;
+static SkTArray<skiatest::Test, kMemcpyOK> gTests;
 
 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()) {
+        const skiatest::Test& test = r->factory();

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

factory returns a const&?  That's at least weird naming.

[hal.canary, 2015/01/16 15:31:57]

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

+        if (SkCommandLineFlags::ShouldSkip(FLAGS_match, test.name)) {
             continue;
         }
-        if (test->isGPUTest() /*&& !gpu_supported()*/) {  // TEMPORARILY DISABLED
+        if (test.needsGpu /*&& !gpu_supported()*/) {  // TEMPORARILY DISABLED
             continue;
         }
-        if (!test->isGPUTest() && !FLAGS_cpu) {
+        if (!test.needsGpu && !FLAGS_cpu) {
             continue;
         }
-        test->setReporter(&gTestReporter);
-        gTests.push_back().reset(test.detach());
+        gTests.push_back(test);
     }
 }
 
-static void run_test(SkAutoTDelete<skiatest::Test>* t) {
+static void run_test(skiatest::Test* testPtr) {

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

Let's just call this test and make test->foo() calls.

[hal.canary, 2015/01/16 15:31:57]

Done.

+    const skiatest::Test& test = *testPtr;
     WallTimer timer;
     timer.start();
-    skiatest::Test* test = t->get();
     if (!FLAGS_dryRun) {
         GrContextFactory grFactory;
-        test->setGrContextFactory(&grFactory);
-        test->run();
-        if (!test->passed()) {
-            fail(SkStringPrintf("test %s failed", test->getName()));
+        DMTestReporter reporter;
+        test.proc(&reporter, &grFactory);
+        if (0 != reporter.errorCount()) {
+            fail(SkStringPrintf("test %s failed", test.name));
         }
     }
     timer.end();
-    done(timer.fWall, "unit", "test", test->getName());
+    done(timer.fWall, "unit", "test", test.name);
 }
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/