Simplify skiatest framework.

dm/DM.cpp

 #include "CrashHandler.h"
 #include "DMJsonWriter.h"
 #include "DMSrcSink.h"
 #include "OverwriteLine.h"
 #include "ProcStats.h"
 #include "SkBBHFactory.h"
 #include "SkCommonFlags.h"
 #include "SkForceLinking.h"
 #include "SkGraphics.h"
 #include "SkMD5.h"
 #include "SkOSFile.h"
 #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.");
 DEFINE_string(src, "gm skp image subset", "Source types to test.");
 DEFINE_bool(nameByHash, false,
             "If true, write to FLAGS_writePath[0]/<hash>.png instead of "
             "to FLAGS_writePath[0]/<config>/<sourceType>/<name>.png");
 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_bool(gpu_threading, false, "Allow GPU work to run on multiple threads?");
 
 DEFINE_string(blacklist, "",
         "Space-separated config/src/name triples to blacklist.  '_' matches anything.  E.g. \n"
         "'--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;
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 
 SK_DECLARE_STATIC_MUTEX(gFailuresMutex);
 static SkTArray<SkString> gFailures;
 
 static void fail(ImplicitString err) {
     SkAutoMutexAcquire lock(gFailuresMutex);
@@ skipping 312 matching lines @@
 static void run_enclave(SkTArray<Task>* tasks) {
     for (int i = 0; i < tasks->count(); i++) {
         Task::Run(tasks->begin() + i);
     }
 }
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 
 // 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.

 
-static SkTArray<SkAutoTDelete<skiatest::Test>, kMemcpyOK> gCPUTests, gGPUTests;
+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<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);
 }
 
 /*~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
 
 int dm_main();
 int dm_main() {
     SetupCrashHandler();
     SkAutoGraphics ag;
     SkTaskGroup::Enabler enabled(FLAGS_threads);
 
@@ skipping 58 matching lines @@
     }
     return 0;
 }
 
 #if !defined(SK_BUILD_FOR_IOS) && !defined(SK_BUILD_FOR_NACL)
 int main(int argc, char** argv) {
     SkCommandLineFlags::Parse(argc, argv);
     return dm_main();
 }
 #endif