Rewrite CancelableTaskTracker unit tests

Rewrite CancelableTaskTracker unit tests

Make it use TaskRunners and Closures instead of low-level synchronization
primitives.

Clean up death tests a bit.  In particular, remove a misleading one that was never fired (the one asserting that cancelling a non-existent ID should crash).

Add some ThreadChecker annotations to CancelableTaskTracker.

BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169488

[akalin, 2012-11-20 01:39:57]

+kai, +brett for review

[kaiwang, 2012-11-21 00:11:43]

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
File chrome/common/cancelable_task_tracker_unittest.cc (right):

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:28: class
FakeNonThreadSafeTaskRunner : public base::TaskRunner {
What we have gain to have this fake runner? Is it possible to do the same thing
with Thread and maybe RunLoop?

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:36: }
Group OVERRIDE functions together and annotate OVERRIDE

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:40: }
The only usage is to get num of tasks. Why not just have a TaskCount function?

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:44: }
not used, delete?

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:75: // TODO(akalin): Fix.
Fix what?

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:83: protected:
public -> protected -> private
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:217:
base::Bind(&base::DoNothing),
why not MakeExpectedRunClosure?

And what's the difference between this test and the above one? Just try on
multi-threads?

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:223: worker_thread_.Stop();
Will Stop() run all existing closures?

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:303: virtual
~CancelableTaskTrackerDeathTest() {}
What about adding a new thread here?
Using worker_thread_ to test thread violation seems a bit odd, because it's not
"working on tasks"

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:307: // good citizens there
and undef the macro.
Still don't understand why duplicated define is needed...

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:351:
MakeExpectedNotRunClosure(FROM_HERE));
I suggest to just post a DoNothing.

If you add a fake_task_runner_.RunUntilEmpty() below. This task actually will
run, no matter in release/debug binary.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:363:
ASSERT_TRUE(thread.Start());
Remove?

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:369:
MakeExpectedNotRunClosure(FROM_HERE));
same here

[kaiwang, 2012-11-21 00:34:51]

Could you also add a test of using IsCanceledCallback on different thread? It
should be thread safe

[akalin, 2012-11-22 00:14:58]

PTAL

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
File chrome/common/cancelable_task_tracker_unittest.cc (right):

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:28: class
FakeNonThreadSafeTaskRunner : public base::TaskRunner {
On 2012/11/21 00:11:43, kaiwang wrote:
> What we have gain to have this fake runner? Is it possible to do the same
thing
> with Thread and maybe RunLoop?

As discussed offline, this is to test the basic functionality of
CancelableTaskTracker separately from its multi-thread guarantees.  Also,
eventually this will be moved out into a separate class.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:36: }
On 2012/11/21 00:11:43, kaiwang wrote:
> Group OVERRIDE functions together and annotate OVERRIDE

Done.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:40: }
On 2012/11/21 00:11:43, kaiwang wrote:
> The only usage is to get num of tasks. Why not just have a TaskCount function?

Done.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:44: }
On 2012/11/21 00:11:43, kaiwang wrote:
> not used, delete?

Done.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:75: // TODO(akalin): Fix.
On 2012/11/21 00:11:43, kaiwang wrote:
> Fix what?

Oops, removed.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:83: protected:
On 2012/11/21 00:11:43, kaiwang wrote:
> public -> protected -> private
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Unfortunately, we're forced to put it in this order; MessageLoop needs to be
instantitated before task_tracker_.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:217:
base::Bind(&base::DoNothing),
On 2012/11/21 00:11:43, kaiwang wrote:
> why not MakeExpectedRunClosure?

Because depending on the thread ordering, the closure may or may not be
cancelled before it's run.

> And what's the difference between this test and the above one? Just try on
> multi-threads?

Yes, basically to provide multi-threaded coverage (for ThreadSanitizer bots,
etc.).

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:223: worker_thread_.Stop();
On 2012/11/21 00:11:43, kaiwang wrote:
> Will Stop() run all existing closures?

Yes, it will block until the other thread runs all closures and quits.  Starting
a thread, posting a task, and stopping it is a quick way to guarantee order
across multiple threads.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:303: virtual
~CancelableTaskTrackerDeathTest() {}
On 2012/11/21 00:11:43, kaiwang wrote:
> What about adding a new thread here?
> Using worker_thread_ to test thread violation seems a bit odd, because it's
not
> "working on tasks"

I removed worker_thread_, instead putting a separate thread in each test, and
renamed it "bad_thread" for these death tests.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:307: // good citizens there
and undef the macro.
On 2012/11/21 00:11:43, kaiwang wrote:
> Still don't understand why duplicated define is needed...

Just for clarity; it's clear that we're checking NDEBUG etc. for ThreadChecker,
and not for DCHECKs or some other reason.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:351:
MakeExpectedNotRunClosure(FROM_HERE));
On 2012/11/21 00:11:43, kaiwang wrote:
> I suggest to just post a DoNothing.
> 
> If you add a fake_task_runner_.RunUntilEmpty() below. This task actually will
> run, no matter in release/debug binary.

Done.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:363:
ASSERT_TRUE(thread.Start());
On 2012/11/21 00:11:43, kaiwang wrote:
> Remove?

Done.

https://codereview.chromium.org/11417077/diff/4/chrome/common/cancelable_task...
chrome/common/cancelable_task_tracker_unittest.cc:369:
MakeExpectedNotRunClosure(FROM_HERE));
On 2012/11/21 00:11:43, kaiwang wrote:
> same here

Done.

[akalin, 2012-11-22 00:17:38]

On 2012/11/21 00:34:51, kaiwang wrote:
> Could you also add a test of using IsCanceledCallback on different thread? It
> should be thread safe

Done.  Also added to comment in cancelable_task_tracker.h to mention that the
callback can be called from any thread.

[kaiwang, 2012-11-22 02:05:54]

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
File chrome/common/cancelable_task_tracker_unittest.cc (right):

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:33: // Stores posted tasks in
a FIFO, ignoring |delay|.p
Remove trailing p

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:73: // Needs to be initialized
before |task_tracker_|.
Why?

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:147: run_loop.RunUntilIdle();
already have this in Test destructor, necessary to duplicate?

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:212: run_loop.RunUntilIdle();
same here

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:362: #endif
I suggest to directly use ENABLE_THREAD_CHECKER and add comment about where it's
defined.

If the macro is modified in thread_checker.h, this piece of code will cause
problems.

[akalin, 2012-11-22 07:00:26]

Addressed comments, PTAL

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
File chrome/common/cancelable_task_tracker_unittest.cc (right):

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:33: // Stores posted tasks in
a FIFO, ignoring |delay|.p
On 2012/11/22 02:05:54, kaiwang wrote:
> Remove trailing p

Done.

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:73: // Needs to be initialized
before |task_tracker_|.
On 2012/11/22 02:05:54, kaiwang wrote:
> Why?

Oops, looks like that's wrong.  Only the methods need it.  Moved it down.

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:147: run_loop.RunUntilIdle();
On 2012/11/22 02:05:54, kaiwang wrote:
> already have this in Test destructor, necessary to duplicate?

This one is necessary (since the EXPECT below depends on it) but I removed the
other ones that are at the very ends of the test.

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:212: run_loop.RunUntilIdle();
On 2012/11/22 02:05:54, kaiwang wrote:
> same here

Done.

https://codereview.chromium.org/11417077/diff/5004/chrome/common/cancelable_t...
chrome/common/cancelable_task_tracker_unittest.cc:362: #endif
On 2012/11/22 02:05:54, kaiwang wrote:
> I suggest to directly use ENABLE_THREAD_CHECKER and add comment about where
it's
> defined.

Can't; note that ENABLE_THREAD_CHECKER is #undef'ed at the end of
thread_checker.h.

> If the macro is modified in thread_checker.h, this piece of code will cause
> problems.

Yeah.  But the same pattern is used in thread_checker_unittest.cc, so I think if
the macro is changed, it'll be easy to find this use.

[kaiwang, 2012-11-23 04:20:02]

LGTM

You still need brettw's approval

[brettw, 2012-11-26 19:09:34]

LGTM rubberstamp

[commit-bot: I haz the power, 2012-11-26 19:10:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/11417077/9002

[commit-bot: I haz the power, 2012-11-26 21:13:39]

Change committed as 169488