From 2baabb43c8f9b327b4b72da795ff43812c725208 Mon Sep 17 00:00:00 2001 From: Kevin Lubick Date: Wed, 21 Sep 2022 09:55:53 -0400 Subject: [PATCH] Change fNeedsGpu/fNeedsGraphite to TestType enum These two booleans were really representing a three-way state. This enum does so more explicitly. This also removes two tool binaries that appear unused, but were related to listing tests/etc for skqp. Change-Id: Ida7a4d4a4aabd8be55ebfc2e92e6b52815e823b0 Bug: skia:13758 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/583236 Reviewed-by: Derek Sollenberger Reviewed-by: Greg Daniel --- BUILD.gn | 18 ----- dm/DM.cpp | 7 +- modules/canvaskit/gm_bindings.cpp | 5 +- tests/Test.h | 105 +++++++++++++++--------------- tools/fm/fm.cpp | 2 +- tools/list_gms.cpp | 25 ------- tools/list_gpu_unit_tests.cpp | 31 --------- tools/skqp/src/skqp.cpp | 3 +- 8 files changed, 64 insertions(+), 132 deletions(-) delete mode 100644 tools/list_gms.cpp delete mode 100644 tools/list_gpu_unit_tests.cpp diff --git a/BUILD.gn b/BUILD.gn index aaddd44e583f..2271a1983dcb 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -2624,24 +2624,6 @@ if (skia_enable_tools) { } } - test_app("list_gms") { - sources = [ "tools/list_gms.cpp" ] - deps = [ - ":gm", - ":skia", - ] - } - test_app("list_gpu_unit_tests") { - sources = [ - "dm/DMGpuTestProcs.cpp", - "tools/list_gpu_unit_tests.cpp", - ] - deps = [ - ":skia", - ":tests", - ] - } - test_lib("sk_app") { public_deps = [ ":gpu_tool_utils", diff --git a/dm/DM.cpp b/dm/DM.cpp index 006d584c7565..91e030af3dc9 100644 --- a/dm/DM.cpp +++ b/dm/DM.cpp @@ -186,6 +186,7 @@ int RuntimeCheckErrorFunc(int errorType, const char* filename, int linenumber, using namespace DM; using sk_gpu_test::GrContextFactory; using sk_gpu_test::ContextInfo; +using skiatest::TestType; #ifdef SK_GL using sk_gpu_test::GLTestContext; #endif @@ -1473,11 +1474,11 @@ static void gather_tests() { if (CommandLineFlags::ShouldSkip(FLAGS_match, test.fName)) { continue; } - if (test.fNeedsGpu && FLAGS_gpu) { + if (test.fTestType == TestType::kGanesh && FLAGS_gpu) { gSerialTests->push_back(test); - } else if (test.fNeedsGraphite && FLAGS_graphite) { + } else if (test.fTestType == TestType::kGraphite && FLAGS_graphite) { gSerialTests->push_back(test); - } else if (!test.fNeedsGpu && !test.fNeedsGraphite && FLAGS_cpu) { + } else if (test.fTestType == TestType::kCPU && FLAGS_cpu) { gParallelTests->push_back(test); } } diff --git a/modules/canvaskit/gm_bindings.cpp b/modules/canvaskit/gm_bindings.cpp index 46c5974f70a0..4aa780f5f886 100644 --- a/modules/canvaskit/gm_bindings.cpp +++ b/modules/canvaskit/gm_bindings.cpp @@ -251,12 +251,15 @@ static JSObject RunTest(std::string name) { return result; } GrContextOptions grOpts; - if (test.fNeedsGpu) { + if (test.fTestType == skiatest::TestType::kGanesh) { result.set("result", "passed"); // default to passing - the reporter will mark failed. WasmReporter reporter(name, result); test.modifyGrContextOptions(&grOpts); test.run(&reporter, grOpts); return result; + } else if (test.fTestType == skiatest::TestType::kGraphite) { + SkDebugf("Graphite test %s not yet supported\n", name.c_str()); + return result; } result.set("result", "passed"); // default to passing - the reporter will mark failed. diff --git a/tests/Test.h b/tests/Test.h index 29284d160427..c48d9f4f599e 100644 --- a/tests/Test.h +++ b/tests/Test.h @@ -71,22 +71,21 @@ class ReporterContext : SkNoncopyable { typedef void (*TestProc)(skiatest::Reporter*, const GrContextOptions&); typedef void (*ContextOptionsProc)(GrContextOptions*); +enum class TestType : uint8_t { kCPU, kGanesh, kGraphite }; + struct Test { Test(const char* name, - bool needsGpu, - bool needsGraphite, + TestType testType, CtsEnforcement ctsEnforcement, TestProc proc, ContextOptionsProc optionsProc = nullptr) : fName(name) - , fNeedsGpu(needsGpu) - , fNeedsGraphite(needsGraphite) + , fTestType(testType) , fCTSEnforcement(ctsEnforcement) , fProc(proc) , fContextOptionsProc(optionsProc) {} const char* fName; - bool fNeedsGpu; - bool fNeedsGraphite; + TestType fTestType; CtsEnforcement fCTSEnforcement; TestProc fProc; ContextOptionsProc fContextOptionsProc; @@ -105,30 +104,20 @@ struct Test { using TestRegistry = sk_tools::Registry; -/* - Use the following macros to make use of the skiatest classes, e.g. - - #include "tests/Test.h" - - DEF_TEST(TestName, reporter) { - ... - REPORTER_ASSERT(reporter, x == 15); - ... - REPORTER_ASSERT(reporter, x == 15, "x should be 15"); - ... - if (x != 15) { - ERRORF(reporter, "x should be 15, but is %d", x); - return; - } - ... - } -*/ - using GrContextFactoryContextType = sk_gpu_test::GrContextFactory::ContextType; typedef void GrContextTestFn(Reporter*, const sk_gpu_test::ContextInfo&); typedef bool GrContextTypeFilterFn(GrContextFactoryContextType); +// We want to run the same test against potentially multiple GPU backends. Test runners should +// implement this function by calling the testFn with a fresh GPU context if that GPU backend +// matches the provided filter. If filter is nullptr, then all compiled-in GPU backends should +// be used. The reporter and opts arguments are piped in from Test::run. +void RunWithGPUTestContexts(GrContextTestFn* testFn, GrContextTypeFilterFn* filter, + Reporter* reporter, const GrContextOptions& opts); + +// These context filters should be implemented by test runners and return true if the backend was +// compiled in (i.e. is supported) and matches the criteria indicated by the name of the filter. extern bool IsGLContextType(GrContextFactoryContextType); extern bool IsVulkanContextType(GrContextFactoryContextType); extern bool IsMetalContextType(GrContextFactoryContextType); @@ -136,8 +125,6 @@ extern bool IsDawnContextType(GrContextFactoryContextType); extern bool IsDirect3DContextType(GrContextFactoryContextType); extern bool IsRenderingGLContextType(GrContextFactoryContextType); extern bool IsMockContextType(GrContextFactoryContextType); -void RunWithGPUTestContexts(GrContextTestFn*, GrContextTypeFilterFn*, Reporter*, - const GrContextOptions&); namespace graphite { @@ -169,6 +156,25 @@ class Timer { } // namespace skiatest +/* + Use the following macros to make use of the skiatest classes, e.g. + + #include "tests/Test.h" + + DEF_TEST(TestName, reporter) { + ... + REPORTER_ASSERT(reporter, x == 15); + ... + REPORTER_ASSERT(reporter, x == 15, "x should be 15"); + ... + if (x != 15) { + ERRORF(reporter, "x should be 15, but is %d", x); + return; + } + ... + } +*/ + #define REPORTER_ASSERT(r, cond, ...) \ do { \ if (!(cond)) { \ @@ -188,14 +194,13 @@ class Timer { } \ } while (0) -#define DEF_CONDITIONAL_TEST(name, reporter, condition) \ - static void test_##name(skiatest::Reporter*, const GrContextOptions&); \ - skiatest::TestRegistry name##TestRegistry(skiatest::Test(#name, \ - /*gpu=*/false, \ - /*graphite=*/false, \ - CtsEnforcement::kNever, \ - test_##name), \ - condition); \ +#define DEF_CONDITIONAL_TEST(name, reporter, condition) \ + static void test_##name(skiatest::Reporter*, const GrContextOptions&); \ + skiatest::TestRegistry name##TestRegistry(skiatest::Test(#name, \ + skiatest::TestType::kCPU, \ + CtsEnforcement::kNever, \ + test_##name), \ + condition); \ void test_##name(skiatest::Reporter* reporter, const GrContextOptions&) #define DEF_TEST(name, reporter) DEF_CONDITIONAL_TEST(name, reporter, true) @@ -209,17 +214,16 @@ class Timer { #endif // TODO update all the callsites to support CtsEnforcement -#define DEF_GRAPHITE_TEST(name, reporter) \ - static void test_##name(skiatest::Reporter*); \ - static void test_graphite_##name(skiatest::Reporter* reporter, \ - const GrContextOptions& /*unused*/) { \ - test_##name(reporter); \ - } \ - skiatest::TestRegistry name##TestRegistry(skiatest::Test(#name, \ - /*gpu=*/false, \ - /*graphite=*/true, \ - CtsEnforcement::kNever, \ - test_graphite_##name)); \ +#define DEF_GRAPHITE_TEST(name, reporter) \ + static void test_##name(skiatest::Reporter*); \ + static void test_graphite_##name(skiatest::Reporter* reporter, \ + const GrContextOptions& /*unused*/) { \ + test_##name(reporter); \ + } \ + skiatest::TestRegistry name##TestRegistry(skiatest::Test(#name, \ + skiatest::TestType::kGraphite, \ + CtsEnforcement::kNever, \ + test_graphite_##name)); \ void test_##name(skiatest::Reporter* reporter) // TODO update all the callsites to support CtsEnforcement @@ -230,17 +234,15 @@ class Timer { skiatest::graphite::RunWithGraphiteTestContexts(test_##name, _reporter); \ } \ skiatest::TestRegistry name##TestRegistry(skiatest::Test(#name, \ - /*gpu=*/false, \ - /*graphite=*/true, \ + skiatest::TestType::kGraphite, \ CtsEnforcement::kNever, \ test_graphite_contexts_##name)); \ void test_##name(skiatest::Reporter* reporter, skgpu::graphite::Context* graphite_context) -// TODO update all the callsites to pass real API values #define DEF_GPUTEST(name, reporter, options, ctsEnforcement) \ static void test_##name(skiatest::Reporter*, const GrContextOptions&); \ skiatest::TestRegistry name##TestRegistry(skiatest::Test( \ - #name, /*gpu=*/true, /*graphite=*/false, ctsEnforcement, test_##name, nullptr)); \ + #name, skiatest::TestType::kGanesh, ctsEnforcement, test_##name, nullptr)); \ void test_##name(skiatest::Reporter* reporter, const GrContextOptions& options) #define DEF_CONDITIONAL_GPUTEST_FOR_CONTEXTS( \ @@ -251,8 +253,7 @@ class Timer { skiatest::RunWithGPUTestContexts(test_##name, context_filter, reporter, options); \ } \ skiatest::TestRegistry name##TestRegistry(skiatest::Test(#name, \ - /*gpu=*/true, \ - /*graphite=*/false, \ + skiatest::TestType::kGanesh, \ ctsEnforcement, \ test_gpu_contexts_##name, \ options_filter), \ diff --git a/tools/fm/fm.cpp b/tools/fm/fm.cpp index 452a8e6da02b..5ec7fac50991 100644 --- a/tools/fm/fm.cpp +++ b/tools/fm/fm.cpp @@ -422,7 +422,7 @@ int main(int argc, char** argv) { SkTHashMap tests; for (const skiatest::Test& test : skiatest::TestRegistry::Range()) { - if (test.fNeedsGpu || test.fNeedsGraphite) { + if (test.fTestType != skiatest::TestType::kCPU) { continue; // TODO } if (FLAGS_listTests) { diff --git a/tools/list_gms.cpp b/tools/list_gms.cpp deleted file mode 100644 index 6c3ebc510dac..000000000000 --- a/tools/list_gms.cpp +++ /dev/null @@ -1,25 +0,0 @@ -/* - * Copyright 2018 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - -#include -#include -#include -#include - -#include "gm/gm.h" - -int main() { - std::vector gms; - for (skiagm::GMFactory factory : skiagm::GMRegistry::Range()) { - std::unique_ptr gm(factory()); - gms.push_back(std::string(gm->getName())); - } - std::sort(gms.begin(), gms.end()); - for (const std::string& gm : gms) { - std::cout << gm << '\n'; - } -} diff --git a/tools/list_gpu_unit_tests.cpp b/tools/list_gpu_unit_tests.cpp deleted file mode 100644 index 380a88fc1575..000000000000 --- a/tools/list_gpu_unit_tests.cpp +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright 2018 Google Inc. - * - * Use of this source code is governed by a BSD-style license that can be - * found in the LICENSE file. - */ - -#include -#include -#include -#include - -#include "tests/Test.h" -#include "tests/TestHarness.h" - -TestHarness CurrentTestHarness() { - return TestHarness::kListGpuUnitTests; -} - -int main() { - std::vector tests; - for (const skiatest::Test& test : skiatest::TestRegistry::Range()) { - if (test.fNeedsGpu) { - tests.push_back(std::string(test.fName)); - } - } - std::sort(tests.begin(), tests.end()); - for (const std::string& test : tests) { - std::cout << test << '\n'; - } -} diff --git a/tools/skqp/src/skqp.cpp b/tools/skqp/src/skqp.cpp index a0f228e373f4..f3f6c917b6b2 100644 --- a/tools/skqp/src/skqp.cpp +++ b/tools/skqp/src/skqp.cpp @@ -58,7 +58,8 @@ static std::vector get_unit_tests(int enforcedAndroidAPILevel) { for (const skiatest::Test& test : skiatest::TestRegistry::Range()) { const auto ctsMode = test.fCTSEnforcement.eval(enforcedAndroidAPILevel); if (ctsMode != CtsEnforcement::RunMode::kSkip) { - SkASSERTF(test.fNeedsGpu, "Non-GPU test was included in SkQP: %s\n", test.fName); + SkASSERTF(test.fTestType != skiatest::TestType::kCPU, + "Non-GPU test was included in SkQP: %s\n", test.fName); unitTests.push_back(&test); } }