Add --skip_cpu and --skip_gpu options to tests

Add --skip_cpu and --skip_gpu options to tests

BUG=skia:2074
Committed: http://code.google.com/p/skia/source/detail?r=13223

[borenet, 2014-01-27 21:31:11]

There may be a nicer way to do this.  Ideally, I'd like to have a
--skip_(cpu|gpu) or --(cpu|gpu)_only flag for EVERY Skia tool.

[mtklein, 2014-01-27 22:35:29]

https://codereview.chromium.org/144343004/diff/1/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/144343004/diff/1/tests/Test.h#newcode69
tests/Test.h:69: virtual bool isGPUTest() const { return false; }
Can you fold isThreadsafe and this together?  isThreadSafe() = !isGpuTest()
universally right now.  I like your name better.

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp#newcode38
tests/skia_test.cpp:38: DEFINE_bool(skip_cpu, false, "skip CPU tests.");
Would somewhat prefer to keep the flags positively oriented:

DEFINE_bool(cpu, true, "Run CPU tests.");
DEFINE_bool(gpu, true, "Run GPU tests.");

You get the negation for free from the flags: --nocpu, --nogpu

(Or run_cpu/run_gpu.  I just like it positive.)

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp#newcode179
tests/skia_test.cpp:179: } else if (test->isGPUTest() && !FLAGS_skip_gpu){
I think this is going to double-count?

Shouldn't it be

if (!ShouldSkip) {
  if ( test->isGPUTest() && !FLAGS_skip_gpu) toRun++;
  if (!test->isGPUTest() && !FLAGS_skip_cpu) toRun++;
}
?

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp#newcode200
tests/skia_test.cpp:200: } else if (test->isGPUTest() && FLAGS_skip_gpu) {
Same deal down here.

[djsollen, 2014-01-28 14:21:01]

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp#newcode38
tests/skia_test.cpp:38: DEFINE_bool(skip_cpu, false, "skip CPU tests.");
I'm a fan of this as well.

On 2014/01/27 22:35:29, mtklein wrote:
> Would somewhat prefer to keep the flags positively oriented:
> 
> DEFINE_bool(cpu, true, "Run CPU tests.");
> DEFINE_bool(gpu, true, "Run GPU tests.");
> 
> You get the negation for free from the flags: --nocpu, --nogpu
> 
> (Or run_cpu/run_gpu.  I just like it positive.)

[borenet, 2014-01-28 18:42:09]

Uploaded patch set 3.

https://codereview.chromium.org/144343004/diff/1/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/144343004/diff/1/tests/Test.h#newcode69
tests/Test.h:69: virtual bool isGPUTest() const { return false; }
On 2014/01/27 22:35:29, mtklein wrote:
> Can you fold isThreadsafe and this together?  isThreadSafe() = !isGpuTest()
> universally right now.  I like your name better.

Done.

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp#newcode38
tests/skia_test.cpp:38: DEFINE_bool(skip_cpu, false, "skip CPU tests.");
On 2014/01/28 14:21:02, djsollen wrote:
> I'm a fan of this as well.
> 
> On 2014/01/27 22:35:29, mtklein wrote:
> > Would somewhat prefer to keep the flags positively oriented:
> > 
> > DEFINE_bool(cpu, true, "Run CPU tests.");
> > DEFINE_bool(gpu, true, "Run GPU tests.");
> > 
> > You get the negation for free from the flags: --nocpu, --nogpu
> > 
> > (Or run_cpu/run_gpu.  I just like it positive.)
> 

Done.

https://codereview.chromium.org/144343004/diff/1/tests/skia_test.cpp#newcode179
tests/skia_test.cpp:179: } else if (test->isGPUTest() && !FLAGS_skip_gpu){
On 2014/01/27 22:35:29, mtklein wrote:
> I think this is going to double-count?
> 
> Shouldn't it be
> 
> if (!ShouldSkip) {
>   if ( test->isGPUTest() && !FLAGS_skip_gpu) toRun++;
>   if (!test->isGPUTest() && !FLAGS_skip_cpu) toRun++;
> }
> ?

Moved to its own function for clarity.

[djsollen, 2014-01-28 18:50:22]

lgtm with one nit

https://codereview.chromium.org/144343004/diff/30001/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/144343004/diff/30001/tests/skia_test.cpp#newc...
tests/skia_test.cpp:129: bool should_run(const char* testName, bool isGPUTest) {
this function should be static

[borenet, 2014-01-28 19:03:41]

Uploaded patch set 4.  For the record, here's what I get when using the new
flags.  Unfortunately, I no longer get each test on its own line for some
reason, but it seems clear that --cpu and --gpu have no effect, since the
default is to run everything, and the union of --nocpu and --nogpu is the same
as providing no flags.

$ out/Debug/tests 
[212/212] GrMemoryPool
$ out/Debug/tests --cpu
[212/212] GrMemoryPool
$ out/Debug/tests --gpu
[212/212] GrMemoryPool
$ out/Debug/tests --nogpu
[195/195] GrMemoryPool
$ out/Debug/tests --nocpu
[ 17/ 17] Blur

https://codereview.chromium.org/144343004/diff/30001/tests/skia_test.cpp
File tests/skia_test.cpp (right):

https://codereview.chromium.org/144343004/diff/30001/tests/skia_test.cpp#newc...
tests/skia_test.cpp:129: bool should_run(const char* testName, bool isGPUTest) {
On 2014/01/28 18:50:22, djsollen wrote:
> this function should be static

Done.

[mtklein, 2014-01-28 19:46:57]

lgtm

-v will show each tests each on its own line as it finishes.

https://codereview.chromium.org/144343004/diff/50001/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/144343004/diff/50001/tests/Test.h#newcode67
tests/Test.h:67: virtual bool isThreadsafe() const { return !this->isGPUTest();
}
Before submitting, can you just remove this whole method and call
test->isGpuTest() instead of !test->isThreadsafe() in skia_test.cpp?

[mtklein, 2014-01-28 19:50:04]

On 2014/01/28 19:46:57, mtklein wrote:
> lgtm
> 
> -v will show each tests each on its own line as it finishes.
> 
> https://codereview.chromium.org/144343004/diff/50001/tests/Test.h
> File tests/Test.h (right):
> 
> https://codereview.chromium.org/144343004/diff/50001/tests/Test.h#newcode67
> tests/Test.h:67: virtual bool isThreadsafe() const { return
!this->isGPUTest();
> }
> Before submitting, can you just remove this whole method and call
> test->isGpuTest() instead of !test->isThreadsafe() in skia_test.cpp?

I can't English.  "each tests each" -> "each test"

[borenet, 2014-01-28 19:53:18]

Ah, good to know.  Uploaded patch set 5

https://codereview.chromium.org/144343004/diff/50001/tests/Test.h
File tests/Test.h (right):

https://codereview.chromium.org/144343004/diff/50001/tests/Test.h#newcode67
tests/Test.h:67: virtual bool isThreadsafe() const { return !this->isGPUTest();
}
On 2014/01/28 19:46:57, mtklein wrote:
> Before submitting, can you just remove this whole method and call
> test->isGpuTest() instead of !test->isThreadsafe() in skia_test.cpp?

Done. I guess we don't currently (or plan to) have any mechanism for marking a
particular test as not threadsafe?

[commit-bot: I haz the power, 2014-01-28 19:53:43]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/borenet@google.com/144343004/70001

[mtklein, 2014-01-28 19:54:38]

On 2014/01/28 19:53:18, borenet wrote:
> Ah, good to know.  Uploaded patch set 5
> 
> https://codereview.chromium.org/144343004/diff/50001/tests/Test.h
> File tests/Test.h (right):
> 
> https://codereview.chromium.org/144343004/diff/50001/tests/Test.h#newcode67
> tests/Test.h:67: virtual bool isThreadsafe() const { return
!this->isGPUTest();
> }
> On 2014/01/28 19:46:57, mtklein wrote:
> > Before submitting, can you just remove this whole method and call
> > test->isGpuTest() instead of !test->isThreadsafe() in skia_test.cpp?
> 
> Done. I guess we don't currently (or plan to) have any mechanism for marking a
> particular test as not threadsafe?

Yeah, not for now.  If anything, I've got a better idea about how to make the
GPU tests threadsafe too, and then all the threadsafety nonsense in skia_test.cc
goes away.

[commit-bot: I haz the power, 2014-01-28 20:02:53]

Change committed as 13223

[borenet, 2014-01-28 20:21:12]

A revert of this CL has been created in
https://codereview.chromium.org/148173010/ by borenet@google.com.

The reason for reverting is: Broke tests on Win7 and Mac.