TestBrowserThreadBundle -- new way to setup fake BrowserThreads in tests

59 views
Skip to first unread message

Albert J. Wong (王重傑)

unread,
Jun 6, 2013, 7:03:51 PM6/6/13
to Chromium-dev

If you don’t write tests that use BrowserThreads, you can stop reading now.


tl;dr -- Use TestBrowserThreadBundle instead of TestBrowserThread.


In r204603, we’ve added TestBrowserThreadBundle. Tests that post to BrowserThreads should instantiate one TestBrowserThreadBundle in their test fixture rather than creating individual TestBrowserThreads.  If your test fixture inherits from RenderViewHostTestHarness, then this is already done for you.


TestBrowserThreadBundle is a simple class that instantiates one TestBrowserThread for each of the named BrowserThreads (UI, DB, FILE, IO, etc.).  By default all the “BrowserThreads” are serviced by a single thread with one MessageLoopForUI.  See the comments in test_browser_thread_bundle.h for configuration options, including how to use a MessageLoopForIO.



== TestBrowerThreadBundle FAQ ==

If I’m just trying to pass the threading DCHECKs, should I use this?

Yes. Passing these DCHECKs is one of the primary use cases.


I only use one or two BrowserThreads, should I use the bundle?

Yes!  Use TestBrowserThreadBundle!


Creating a single TestBrowserThread is an anti-pattern.


When a particular named BrowserThread doesn’t exist, BrowserThread::PostTask() helpfully discards the posted task and returns false.  In production, this is necessary for shutdown.  However, In tests, this becomes an implicit dependency cut which causes 2 problems:


 (1) You unwittingly change your testing surface.

 (2) If someone adds the missing thread later, the cut disappears,

innervating code in a seemingly unrelated area which is hard to debug.


In general, tests should create all BrowserThreads. Dependencies should be cut by adding correct APIs to disable the posting of unwanted tasks.


Is there ever a reason to directly create a TestBrowserThread?

You know...I actually can’t think of any.


You said the bundle only uses one thread earlier. I want real threads. How do I do that?

You must be thirsty. In the next town over is an Ivory Tower of Unit Testing. There is a fountain at the top. Go there and have a drink.


Okay, I drank from the fountain; flavor was too bland for me. How do I get real threads?

What are you trying to assert in your test? Are you sure you want really want parallel execution with threads blocking on each other, or do you just want to tests what happens when posted tasks are run in any order?


For example, let's say I want to test interaction between UpdateData() and GetResult() below:


 class Foo {

  public:

   Foo() : result_(0) {}

   void UpdateData() {

         int* update = new int();

         BrowserThread::PostTaskAndReply(IO, FROM_HERE,

           Bind(&Foo::GetDataOnIO, Unretained(this), update),

           Bind(&Foo::OnNewData, Unretained(this), Owned(update)));

   }


   int GetResult() { return result_ };


  private:

   void GetDataOnIO(int * update) { *update = 3; }

   void OnNewData(int* update) { result_ += *update; }


   int result_;

 }


The assertion is likely that GetResult() will have defined behavior regardless of whether the task and reply in UpdateData() have completed running.  With all BrowserThreads being serviced by a single thread, I can directly script the concurrency as follows:


 TEST_F(FooTest, GetResultConcurrencySafety) {

   Foo foo;


   // GetResult() called before posted tasks run.

   foo.UpdateData();

   EXPECT_EQ(0, foo.GetResult());

   

   // GetResult() called after Update has completed.

   base::RunLoop().RunUntilIdle();

   EXPECT_EQ(3, foo.GetResult());

 }


Or if you go the whitebox route, you can even do:

 TEST_F(FooTest, GetResultWhiteboxConcurrencySafety) {

   // GetResult() before everything else.

   Foo foo1;

   int update = -1;

   EXPECT_EQ(0, foo1.GetResult());

   foo1.GetDataOnIO(&update);

   foo1.OnNewData(&update);


   // GetResult() called mid update.   

   Foo foo2;

   update = -1

   foo2.GetDataOnIO(&update);

   EXPECT_EQ(0, foo2.GetResult());

   foo2.OnNewData(&update);


   // GetResult() called after Update has completed.

   Foo foo3;

   update = -1

   foo3.GetDataOnIO(&update);

   foo3.OnNewData(&update);

   EXPECT_EQ(3, foo3.GetResult());

 }


When all BrowserThreads are serviced by one MessageLoop, you can enumerate the races you care to check for by directly scripting the order in which functions are invoked.  This can either be done by directly calling the functions (whitebox) or at a higher level by pumping a RunLoop as appropriate. All this can be done without any true parallelism.


...you still want real threads? Fine. Fetch me the Rat Tail from the Feymarch and then we’ll talk.


I got the Rat Tail.  Now tell me about real threads or next time I’m bringing a Bomb Ring.

Okay okay, fine fine.


If you truly need real threads because some underlying API synchronously waits on another BrowserThead, or because your test truly is making assertions about parallel behavior, then pass the appropriate bitwise OR of the REAL_XX_THREAD options to the TestBrowserThread constructor.  For example, you could write


  thread_bundle_(

      REAL_WEBKIT_DEPRECATED_THREAD |

      REAL_CACHE_THREAD)


and the bundle will use real threads to service the WEBKIT_DEPRECATED and CACHE BrowserThreads.


Again though, most test really don’t need parallelism. If you find yourself using a bunch of WaitableEvents or RunLoop::QuitClosures to make your test non-flaky, you almost certainly aren’t testing true parallelism and should remove the threads.


Doesn’t making this single-threaded reduce the usefulness of TSAN?

You really like threads don’t you.


In practice, all the tests I saw either already didn’t use real threads, or when they did, they added so many WaitableEvents to control ordering that there was no actual parallelism. In the latter situation, the only parallelism TSAN would really be catching bugs in was the extra test logic that forced serialization.


Also, browser tests still use real threads so TSAN will have plenty of opportunity to catch bugs there.


What’s the right way to pump a MessageLoop?

Use base::RunLoop for unit tests, and content::RunAllPendingInMessageLoop() for browser tests.


The content version, when used within the browser test framework, has special handling for Views which is required to avoid hangs.  That will go away with Aura, but until then we’re stuck with it.


In unittests, base::RunLoop suffices.


DO NOT USE THE METHODS ON MessageLoop ITSELF!


I need an IO MessageLoop.  How do I do that?

Pass in the IO_MAINLOOP bitmask to the TestBrowserThreadBundle constructor. Example:

 class FooTest : public testing::Test {

  protected:

   FooTest() : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {

  

  private:

   TestBrowserThreadBundle thread_bundle_;

 }


I need an IO MessageLoop AND a UI MessageLoop.  How do I do that?

We don’t do Chimera MessageLoops here. That’s black black magic.


In this situation, your only solution is to create a REAL_IO_THREAD. The main loop for all other threads will use a MessageLoopForUI, and the IO thread will have an MessageLoopForIO.


Fine. You got me to recommend using real threads.  I hope that makes you happy.


Why ya always gotta be so weird in your FAQs?

唔話你知 ^.^


John Abd-El-Malek

unread,
Jun 6, 2013, 7:28:52 PM6/6/13
to Albert J. Wong (王重傑), Chromium-dev
This is great, thanks.


On Thu, Jun 6, 2013 at 4:03 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

If you don’t write tests that use BrowserThreads, you can stop reading now.


tl;dr -- Use TestBrowserThreadBundle instead of TestBrowserThread.


In r204603, we’ve added TestBrowserThreadBundle. Tests that post to BrowserThreads should instantiate one TestBrowserThreadBundle in their test fixture rather than creating individual TestBrowserThreads.  If your test fixture inherits from RenderViewHostTestHarness, then this is already done for you.


TestBrowserThreadBundle is a simple class that instantiates one TestBrowserThread for each of the named BrowserThreads (UI, DB, FILE, IO, etc.).  By default all the “BrowserThreads” are serviced by a single thread with one MessageLoopForUI.  See the comments in test_browser_thread_bundle.h for configuration options, including how to use a MessageLoopForIO.



== TestBrowerThreadBundle FAQ ==

If I’m just trying to pass the threading DCHECKs, should I use this?

Yes. Passing these DCHECKs is one of the primary use cases.


I only use one or two BrowserThreads, should I use the bundle?

Yes!  Use TestBrowserThreadBundle!


Creating a single TestBrowserThread is an anti-pattern.


When a particular named BrowserThread doesn’t exist, BrowserThread::PostTask() helpfully discards the posted task and returns false.  In production, this is necessary for shutdown.  However, In tests, this becomes an implicit dependency cut which causes 2 problems:


 (1) You unwittingly change your testing surface.

 (2) If someone adds the missing thread later, the cut disappears,

innervating code in a seemingly unrelated area which is hard to debug.


In general, tests should create all BrowserThreads. Dependencies should be cut by adding correct APIs to disable the posting of unwanted tasks.


Is there ever a reason to directly create a TestBrowserThread?

You know...I actually can’t think of any.


This begs the question: can we remove this class altogether after all the existing usages are removed? I'm a huge fan of having only way to do something.
 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev
 
 
 

Albert J. Wong (王重傑)

unread,
Jun 6, 2013, 7:59:09 PM6/6/13
to John Abd-El-Malek, Chromium-dev
On Thu, Jun 6, 2013 at 4:28 PM, John Abd-El-Malek <j...@chromium.org> wrote:
This is great, thanks.


On Thu, Jun 6, 2013 at 4:03 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

If you don’t write tests that use BrowserThreads, you can stop reading now.


tl;dr -- Use TestBrowserThreadBundle instead of TestBrowserThread.


In r204603, we’ve added TestBrowserThreadBundle. Tests that post to BrowserThreads should instantiate one TestBrowserThreadBundle in their test fixture rather than creating individual TestBrowserThreads.  If your test fixture inherits from RenderViewHostTestHarness, then this is already done for you.


TestBrowserThreadBundle is a simple class that instantiates one TestBrowserThread for each of the named BrowserThreads (UI, DB, FILE, IO, etc.).  By default all the “BrowserThreads” are serviced by a single thread with one MessageLoopForUI.  See the comments in test_browser_thread_bundle.h for configuration options, including how to use a MessageLoopForIO.



== TestBrowerThreadBundle FAQ ==

If I’m just trying to pass the threading DCHECKs, should I use this?

Yes. Passing these DCHECKs is one of the primary use cases.


I only use one or two BrowserThreads, should I use the bundle?

Yes!  Use TestBrowserThreadBundle!


Creating a single TestBrowserThread is an anti-pattern.


When a particular named BrowserThread doesn’t exist, BrowserThread::PostTask() helpfully discards the posted task and returns false.  In production, this is necessary for shutdown.  However, In tests, this becomes an implicit dependency cut which causes 2 problems:


 (1) You unwittingly change your testing surface.

 (2) If someone adds the missing thread later, the cut disappears,

innervating code in a seemingly unrelated area which is hard to debug.


In general, tests should create all BrowserThreads. Dependencies should be cut by adding correct APIs to disable the posting of unwanted tasks.


Is there ever a reason to directly create a TestBrowserThread?

You know...I actually can’t think of any.


This begs the question: can we remove this class altogether after all the existing usages are removed? I'm a huge fan of having only way to do something.

Yes, I think we can.  Looks like there are currently 490 uses over 270 files right now.

$ git grep --cached 'TestBrowserThread ' | wc -l
490
$ git grep --cached 'TestBrowserThread ' | cut -d ':' -f1 | sort -u | wc -l
270

Cleaning it up won't be purely mechanical though because of the innervation I mentioned previously.

-Albert
Reply all
Reply to author
Forward
0 new messages