Skip to content

Commit

Permalink
Change fNeedsGpu/fNeedsGraphite to TestType enum
Browse files Browse the repository at this point in the history 
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 <[email protected]>
Reviewed-by: Greg Daniel <[email protected]>
  • Loading branch information
@kjlubick
kjlubick authored and SkCQ committed Sep 21, 2022
1 parent 6dc680b commit 2baabb4
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 132 deletions.
18 changes: 0 additions & 18 deletions BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
7 changes: 4 additions & 3 deletions dm/DM.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
}
Expand Down
5 changes: 4 additions & 1 deletion modules/canvaskit/gm_bindings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
105 changes: 53 additions & 52 deletions tests/Test.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -105,39 +104,27 @@ struct Test {

using TestRegistry = sk_tools::Registry<Test>;

/*
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);
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 {

Expand Down Expand Up @@ -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)) { \
Expand All @@ -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)
Expand All @@ -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
Expand All @@ -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( \
Expand All @@ -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), \
Expand Down
2 changes: 1 addition & 1 deletion tools/fm/fm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ int main(int argc, char** argv) {

SkTHashMap<SkString, const skiatest::Test*> 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) {
Expand Down
25 changes: 0 additions & 25 deletions tools/list_gms.cpp
View file

This file was deleted.

31 changes: 0 additions & 31 deletions tools/list_gpu_unit_tests.cpp
View file

This file was deleted.

3 changes: 2 additions & 1 deletion tools/skqp/src/skqp.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ static std::vector<SkQP::UnitTest> 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);
}
}
Expand Down

0 comments on commit 2baabb4

Please sign in to comment.