Generate list of GPU contexts outside SurfaceTest tests

Generate list of GPU contexts outside SurfaceTest tests

Add support for feeding the tests with contexts directly to the unit
test framework.

This fixes the problem where tests are more complex than needed just in
order to run the test code with multiple backends.

Also makes it possible to change the logic how contexts are
created. Instead of direct numbering, the different testable contexts
may be generated from filtered cross-product of context options. For
example: currently NVPR is a type of context. However, it could be also
an on/off feature of any context. In order to test this kind of context,
the enumeration can not be just of context type. It's simpler
to move the enumeration out of the tests.

A test targeting both normal and GPU backends would look like:

static void test_obj_behavior(skiatest::Reporter* reporter,
    SkObj* obj, [other params] ) {
    ... test with obj and param ..
}
DEF_TEST(ObjBehavior, reporter) {
    for (auto& object : generate_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
#if SK_SUPPORT_GPU
DEF_GPUTEST_FOR_ALL_CONTEXTS(ObjBehavior_Gpu, reporter, context) {
    for (auto& object : generate_gpu_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
#endif

Uses the feature in SurfaceTests as an example.

Moves SkSurface -related tests from ImageTest to SurfaceTest.

BUG=skia:2992
Committed: https://skia.googlesource.com/skia/+/179a8f5f7feab052e24596d0d04ab5cf2ccab5e0

[Kimmo Kinnunen, 2015-11-13 13:31:25]

kkinnunen@nvidia.com changed reviewers:
+ bsalomon@google.com, mtklein@google.com

[Kimmo Kinnunen, 2015-11-13 13:31:25]

Would this clarify or complexify current tests?

[Kimmo Kinnunen, 2015-11-13 13:34:31]

Description was changed from

==========
Generate list of GPU contexts outside SurfaceTest tests

Add support for feeding the tests with contexts directly to the unit
test framework.

This fixes the problem where tests are more complex than needed just in
order to run the test code with multiple backends.

Also makes it possible to change the logic how contexts are
created. Instead of direct numbering, the different testable contexts
may be generated from filtered cross-product of context options. For
example: currently NVPR is a type of context. However, it could be also
an on/off feature of any context. In order to test this kind of context,
the enumeration can not be just of context type. It's simpler
to move the enumeration out of the tests.

A test targeting both normal and GPU backends would look like:

static void test_obj_behavior(skiatest::Reporter* reporter,
    SkObj* obj, [other params] ) {
    ... test with obj and param ..
}
DEF_TEST(reporter, ObjBehavior) {
    for (auto& object : generate_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #if SK_SUPPORT_GPU
DEF_GPUTEST_FOR_ALL_CONTEXTS(reporter, ObjBehavior, context) {
    for (auto& object : generate_gpu_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #endif
Uses the feature in SurfaceTests as an example.

Moves SkImage -related tests from SurfaceTest to ImageTest.

Fixes build with "GYP_DEFINES=skia_gpu=0 ./gyp_skia"

BUG=skia:
==========

to

==========
Generate list of GPU contexts outside SurfaceTest tests

Add support for feeding the tests with contexts directly to the unit
test framework.

This fixes the problem where tests are more complex than needed just in
order to run the test code with multiple backends.

Also makes it possible to change the logic how contexts are
created. Instead of direct numbering, the different testable contexts
may be generated from filtered cross-product of context options. For
example: currently NVPR is a type of context. However, it could be also
an on/off feature of any context. In order to test this kind of context,
the enumeration can not be just of context type. It's simpler
to move the enumeration out of the tests.

A test targeting both normal and GPU backends would look like:

static void test_obj_behavior(skiatest::Reporter* reporter,
    SkObj* obj, [other params] ) {
    ... test with obj and param ..
}
DEF_TEST(ObjBehavior, reporter) {
    for (auto& object : generate_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #if SK_SUPPORT_GPU
DEF_GPUTEST_FOR_ALL_CONTEXTS(ObjBehavior_Gpu, reporter, context) {
    for (auto& object : generate_gpu_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #endif
Uses the feature in SurfaceTests as an example.

Moves SkImage -related tests from SurfaceTest to ImageTest.

Fixes build with "GYP_DEFINES=skia_gpu=0 ./gyp_skia"

BUG=skia:
==========

[Kimmo Kinnunen, 2015-11-16 10:51:42]

Description was changed from

==========
Generate list of GPU contexts outside SurfaceTest tests

Add support for feeding the tests with contexts directly to the unit
test framework.

This fixes the problem where tests are more complex than needed just in
order to run the test code with multiple backends.

Also makes it possible to change the logic how contexts are
created. Instead of direct numbering, the different testable contexts
may be generated from filtered cross-product of context options. For
example: currently NVPR is a type of context. However, it could be also
an on/off feature of any context. In order to test this kind of context,
the enumeration can not be just of context type. It's simpler
to move the enumeration out of the tests.

A test targeting both normal and GPU backends would look like:

static void test_obj_behavior(skiatest::Reporter* reporter,
    SkObj* obj, [other params] ) {
    ... test with obj and param ..
}
DEF_TEST(ObjBehavior, reporter) {
    for (auto& object : generate_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #if SK_SUPPORT_GPU
DEF_GPUTEST_FOR_ALL_CONTEXTS(ObjBehavior_Gpu, reporter, context) {
    for (auto& object : generate_gpu_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #endif
Uses the feature in SurfaceTests as an example.

Moves SkImage -related tests from SurfaceTest to ImageTest.

Fixes build with "GYP_DEFINES=skia_gpu=0 ./gyp_skia"

BUG=skia:
==========

to

==========
Generate list of GPU contexts outside SurfaceTest tests

Add support for feeding the tests with contexts directly to the unit
test framework.

This fixes the problem where tests are more complex than needed just in
order to run the test code with multiple backends.

Also makes it possible to change the logic how contexts are
created. Instead of direct numbering, the different testable contexts
may be generated from filtered cross-product of context options. For
example: currently NVPR is a type of context. However, it could be also
an on/off feature of any context. In order to test this kind of context,
the enumeration can not be just of context type. It's simpler
to move the enumeration out of the tests.

A test targeting both normal and GPU backends would look like:

static void test_obj_behavior(skiatest::Reporter* reporter,
    SkObj* obj, [other params] ) {
    ... test with obj and param ..
}
DEF_TEST(ObjBehavior, reporter) {
    for (auto& object : generate_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #if SK_SUPPORT_GPU
DEF_GPUTEST_FOR_ALL_CONTEXTS(ObjBehavior_Gpu, reporter, context) {
    for (auto& object : generate_gpu_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #endif
Uses the feature in SurfaceTests as an example.

Moves SkImage -related tests from SurfaceTest to ImageTest.

Fixes build with "GYP_DEFINES=skia_gpu=0 ./gyp_skia"

BUG=skia:2992
==========

[bsalomon, 2015-11-16 14:12:08]

On 2015/11/13 13:31:25, Kimmo Kinnunen wrote:
> Would this clarify or complexify current tests?

I like the idea of removing the context loops from the tests. Is it possible to
simplify the initial CL. It looks like 100s of lines of test code were moved and
modified and it'd be simpler to review if we can keep those
relocations/modifications separate from the macros/context-type aspects.

[Kimmo Kinnunen, 2015-11-16 17:48:26]

On 2015/11/16 14:12:08, bsalomon wrote:
> On 2015/11/13 13:31:25, Kimmo Kinnunen wrote:
> > Would this clarify or complexify current tests?
> 
> I like the idea of removing the context loops from the tests. Is it possible
to
> simplify the initial CL. It looks like 100s of lines of test code were moved
and
> modified and it'd be simpler to review if we can keep those
> relocations/modifications separate from the macros/context-type aspects.

Not really, but I can of course try. It takes significant amount of time
splitting the patch in less modifications.

The original code typically used a hack where nullptr was pushed to context list
when it wanted to run non-gpu codepath.
Thus if the context list enumeration is lifted out of the test, the test
structure must change.

Other typical hack was that routines accepted many parameters, where for example
context was one. Then if context was null, the routine changed behavior.
Essentially a function implemented two different functions, and context
parameter was part of the function name.

[bsalomon, 2015-11-17 05:03:27]

On 2015/11/16 17:48:26, Kimmo Kinnunen wrote:
> On 2015/11/16 14:12:08, bsalomon wrote:
> > On 2015/11/13 13:31:25, Kimmo Kinnunen wrote:
> > > Would this clarify or complexify current tests?
> > 
> > I like the idea of removing the context loops from the tests. Is it possible
> to
> > simplify the initial CL. It looks like 100s of lines of test code were moved
> and
> > modified and it'd be simpler to review if we can keep those
> > relocations/modifications separate from the macros/context-type aspects.
> 
> Not really, but I can of course try. It takes significant amount of time
> splitting the patch in less modifications.
> 
> The original code typically used a hack where nullptr was pushed to context
list
> when it wanted to run non-gpu codepath.
> Thus if the context list enumeration is lifted out of the test, the test
> structure must change.
> 
> Other typical hack was that routines accepted many parameters, where for
example
> context was one. Then if context was null, the routine changed behavior.
> Essentially a function implemented two different functions, and context
> parameter was part of the function name.

Gotcha, is it possible to leave the tests in the original files so that the diff
lines up? Or move the tests and then make the change?

[Kimmo Kinnunen, 2015-11-17 10:23:11]

On 2015/11/17 05:03:27, bsalomon wrote:
> Gotcha, is it possible to leave the tests in the original files so that the
diff
> lines up? Or move the tests and then make the change?

I moved the tests.
Fix build:          https://codereview.chromium.org/1454653002/
Move:               https://codereview.chromium.org/1452123002/
SurfaceTest (this): https://codereview.chromium.org/1446453003/
ImageTest:          https://codereview.chromium.org/1454663002/
Other tests:        https://codereview.chromium.org/1448873002/

[Kimmo Kinnunen, 2015-11-17 10:23:49]

Description was changed from

==========
Generate list of GPU contexts outside SurfaceTest tests

Add support for feeding the tests with contexts directly to the unit
test framework.

This fixes the problem where tests are more complex than needed just in
order to run the test code with multiple backends.

Also makes it possible to change the logic how contexts are
created. Instead of direct numbering, the different testable contexts
may be generated from filtered cross-product of context options. For
example: currently NVPR is a type of context. However, it could be also
an on/off feature of any context. In order to test this kind of context,
the enumeration can not be just of context type. It's simpler
to move the enumeration out of the tests.

A test targeting both normal and GPU backends would look like:

static void test_obj_behavior(skiatest::Reporter* reporter,
    SkObj* obj, [other params] ) {
    ... test with obj and param ..
}
DEF_TEST(ObjBehavior, reporter) {
    for (auto& object : generate_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #if SK_SUPPORT_GPU
DEF_GPUTEST_FOR_ALL_CONTEXTS(ObjBehavior_Gpu, reporter, context) {
    for (auto& object : generate_gpu_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
 #endif
Uses the feature in SurfaceTests as an example.

Moves SkImage -related tests from SurfaceTest to ImageTest.

Fixes build with "GYP_DEFINES=skia_gpu=0 ./gyp_skia"

BUG=skia:2992
==========

to

==========
Generate list of GPU contexts outside SurfaceTest tests

Add support for feeding the tests with contexts directly to the unit
test framework.

This fixes the problem where tests are more complex than needed just in
order to run the test code with multiple backends.

Also makes it possible to change the logic how contexts are
created. Instead of direct numbering, the different testable contexts
may be generated from filtered cross-product of context options. For
example: currently NVPR is a type of context. However, it could be also
an on/off feature of any context. In order to test this kind of context,
the enumeration can not be just of context type. It's simpler
to move the enumeration out of the tests.

A test targeting both normal and GPU backends would look like:

static void test_obj_behavior(skiatest::Reporter* reporter,
    SkObj* obj, [other params] ) {
    ... test with obj and param ..
}
DEF_TEST(ObjBehavior, reporter) {
    for (auto& object : generate_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
#if SK_SUPPORT_GPU
DEF_GPUTEST_FOR_ALL_CONTEXTS(ObjBehavior_Gpu, reporter, context) {
    for (auto& object : generate_gpu_object) {
        for (auto& other_param : generate_other_variant) {
	   test_obj_behavior(reporter, object, other_param);
	}
    }
}
#endif

Uses the feature in SurfaceTests as an example.

Moves SkSurface -related tests from ImageTest to SurfaceTest.

BUG=skia:2992
==========

[bsalomon, 2015-11-17 14:49:37]

https://codereview.chromium.org/1446453003/diff/40001/dm/DM.cpp
File dm/DM.cpp (right):

https://codereview.chromium.org/1446453003/diff/40001/dm/DM.cpp#newcode1171
dm/DM.cpp:1171: test(reporter, context);
I'm in the process of adding tests that would want to also access the
SkGLContext that backs the GrContext. Do you think we could pass a struct here
rather than GrContext*? Something like

struct GrContextInfo {
    GrContext*    fContext;
    SkGLContext*  fGLContext;
};

? That way if we decide to extend the set of things passed we don't need to
update each test.

https://codereview.chromium.org/1446453003/diff/40001/tests/SurfaceTest.cpp
File tests/SurfaceTest.cpp (right):

https://codereview.chromium.org/1446453003/diff/40001/tests/SurfaceTest.cpp#n...
tests/SurfaceTest.cpp:306:
DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceBackendHandleAccessCopyOnWrite_Gpu,
reporter, context) {
Should we bake the #if SK_SUPPORT_GPU into the macros?

[Kimmo Kinnunen, 2015-11-17 17:55:38]

On 2015/11/17 14:49:37, bsalomon wrote:
> https://codereview.chromium.org/1446453003/diff/40001/dm/DM.cpp
> File dm/DM.cpp (right):
> 
> https://codereview.chromium.org/1446453003/diff/40001/dm/DM.cpp#newcode1171
> dm/DM.cpp:1171: test(reporter, context);
> I'm in the process of adding tests that would want to also access the
> SkGLContext that backs the GrContext. Do you think we could pass a struct here
> rather than GrContext*? Something like
> 
> struct GrContextInfo {
>     GrContext*    fContext;
>     SkGLContext*  fGLContext;
> };
> 
> ? That way if we decide to extend the set of things passed we don't need to
> update each test.

I can try that..

Though I don't know what can you add there -- it's loop over contexts. If you
loop over something else,
it'd be logical to add a different define.

for p in prop1: for c in contexts: contextinfo{ p, c.gr, c.gl } (how is p a
property in context info?)

 
> https://codereview.chromium.org/1446453003/diff/40001/tests/SurfaceTest.cpp
> File tests/SurfaceTest.cpp (right):
> 
>
https://codereview.chromium.org/1446453003/diff/40001/tests/SurfaceTest.cpp#n...
> tests/SurfaceTest.cpp:306:
> DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceBackendHandleAccessCopyOnWrite_Gpu,
> reporter, context) {
> Should we bake the #if SK_SUPPORT_GPU into the macros?

It would need an end tag, which is a bit unnatural?
...CONTEXTS(.., {

})

I think bigger issue would be the includes, which still would require guards.
Then if you are allowed to write the code without guards, it appears
inconsistent.
(personally I'd prefer no guards anywhere, but then again I don't know all the
requirements.)

[Kimmo Kinnunen, 2015-11-18 09:56:32]

On 2015/11/17 14:49:37, bsalomon wrote:
> https://codereview.chromium.org/1446453003/diff/40001/dm/DM.cpp
> File dm/DM.cpp (right):
> 
> https://codereview.chromium.org/1446453003/diff/40001/dm/DM.cpp#newcode1171
> dm/DM.cpp:1171: test(reporter, context);
> I'm in the process of adding tests that would want to also access the
> SkGLContext that backs the GrContext. Do you think we could pass a struct here
> rather than GrContext*? Something like
> 
> struct GrContextInfo {
>     GrContext*    fContext;
>     SkGLContext*  fGLContext;
> };
> 
> ? That way if we decide to extend the set of things passed we don't need to
> update each test.

I added the SkGLContext as an optional parameter to DEF_GPUTEST(name, reporter,
context, glcontext).
(on the grounds that passing the plain struct would create noise in majority of
the cases).


> DEF_GPUTEST_FOR_RENDERING_CONTEXTS(SurfaceBackendHandleAccessCopyOnWrite_Gpu,
> reporter, context) {
> Should we bake the #if SK_SUPPORT_GPU into the macros?

I did not add this one.

PTAL.

[Kimmo Kinnunen, 2015-11-18 13:12:24]

https://codereview.chromium.org/1446453003/diff/140001/tests/TestTest.cpp
File tests/TestTest.cpp (right):

https://codereview.chromium.org/1446453003/diff/140001/tests/TestTest.cpp#new...
tests/TestTest.cpp:69:
DEF_GPUTEST_FOR_ALL_CONTEXTS(TestGpuGrContextAndGLContext, reporter, context,
glContext) {
so would this work for the use-case you mentioned?

[bsalomon, 2015-11-18 15:02:17]

lgtm

[Kimmo Kinnunen, 2015-11-19 06:36:30]

The CQ bit was checked by kkinnunen@nvidia.com

[commit-bot: I haz the power, 2015-11-19 06:36:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1446453003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1446453003/140001

[commit-bot: I haz the power, 2015-11-19 06:37:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 06:37:56]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[Kimmo Kinnunen, 2015-11-19 09:02:07]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-11-19 09:02:07]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1446453003/#ps160001
(title: "msvc fixes")

[commit-bot: I haz the power, 2015-11-19 09:02:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1446453003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1446453003/160001

[commit-bot: I haz the power, 2015-11-19 09:03:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-19 09:03:42]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[Kimmo Kinnunen, 2015-11-19 10:13:43]

Patchset #10 (id:180001) has been deleted

[Kimmo Kinnunen, 2015-11-20 07:09:38]

The CQ bit was checked by kkinnunen@nvidia.com

[Kimmo Kinnunen, 2015-11-20 07:09:39]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1446453003/#ps160001
(title: "msvc fixes")

[commit-bot: I haz the power, 2015-11-20 07:09:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1446453003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1446453003/160001

[commit-bot: I haz the power, 2015-11-20 21:32:29]

Committed patchset #9 (id:160001) as
https://skia.googlesource.com/skia/+/179a8f5f7feab052e24596d0d04ab5cf2ccab5e0