Responding to three separate parts of the thread (too bad we aren't using
Wave :P).
On Tue, Aug 4, 2009 at 7:35 AM, Darin Fisher <da...@chromium.org> wrote:
> I don't think we want a HasPendingTasks method. Consider that the
> MessageLoop may be used by multiple threads. Any code depending on
> HasPendingTasks is likely to be fragile. Also, a MessageLoop may have work
> to do that is not task related.
I think adding inspection functions isn't a bad solution, assuming they're
marked for testing (eg., Making the name HasPendingTasksForTest()).
Using these functions in a multithreaded is not safe, but in unittests,
we've been using a pattern of creating a message loop, then executing
message_loop.RunAllPending() to simulate one iteration of the loop. This
setup doesn't execute a new thread and gives us a "paused" loop that would
actually be quite suitable for inspection.
This might actually be one of the least invasive methods of allowing for a
test point to be inserted.
> On Tue, Aug 4, 2009 at 6:41 AM, Andrew Scherkus <scher...@chromium.org>wrote:
>
>> I think it'd interesting to try. I imagine we'd need some helper gmock
>> actions to take care of executing/deleting tasks.
>> Out of curiosity, would adding a HasPendingTasks() method solve your
>> current testing issue?
>>
>>
>> On Tue, Aug 4, 2009 at 6:28 AM, Marc-Antoine Ruel <mar...@chromium.org>wrote:
>>
>>>
>>> I'm slightly against. No real reason :) except that it'll definitely
>>> bloat the WPO build.
>>
>>
How bad would the bloat be?
Another possible solution that avoids a virtual is to add a constructor that
takes a mock delegate for just the for functions. Then in the message loop
code, add a shim into each API that can be intercepted. Something like
MessgaeLoop(PostTaskDelegate *mock_delegate)
: mock_delegate_(mock_delegate) {
}
void PostTask(...) {
if (mock_delegate_) {
mock_delegate_->PostTask(....);
return;
}
// do real work.
}
This is a bit ugly since it will take a bit of inspection to know which
methods are mockable, and the shim needs to be put into each mockable
function. However, it has the niceness of not really disturbing the current
API structure and avoiding virtual.
>
>>> M-A
>>>
>>> On Tue, Aug 4, 2009 at 2:37 AM, Darin Fisher<da...@chromium.org> wrote:
>>> > MessageLoop is not designed to be subclassed. Call me a minimalist,
>>> but I
>>> > think it damages slightly the readability of the code to have methods
>>> marked
>>> > virtual that do not need to be. That said, I love mocking. Since a
>>> lot of
>>> > code doesn't actually need a MessageLoop so much as a place to post
>>> tasks,
>>> > maybe it would be better to define an interface for posting tasks that
>>> > MessageLoop can implement. Then a lot of code could be switched over
>>> to
>>> > that interface, making the code a bit more abstract. Think of
>>> > IPC::Message::Sender as an example of this kind of abstraction.
>>>
>>
That'd work and give a pretty clear separation. However, I also have a
couple of slight hesitations:
(a) It adds yet another interface + complexity. We already have
MessagePump,
MessagePump::Delegate, MessageLoop, etc., which at least when
starting out, can get really confusing.
(b) It still makes the methods virtual, which has the effect of making
them
feel subclassable (like you said), and bloating the build (like M-A
said)
Of these three choices, I'm tempted to go with the shim approach, with the
mock being passed during construction.
-Albert
>
>>> > On Mon, Aug 3, 2009 at 8:23 PM, Albert J. Wong (王重傑) <
>>> ajw...@chromium.org>
>>> > wrote:
>>> >>
>>> >> I've noticed that most public functions on MessageLoop are
>>> non-virtual.
>>> >> How bad would it be to make PostTask, and its variants, virtual? Are
>>> the
>>> >> perf implications or similar that would be bad?
>>> >> I'd like to be able to use gmock to mock out a message loop so I can
>>> test
>>> >> if my code knows to stop posting tasks. However, not having the
>>> message
>>> >> loop be virtual makes this hard.
>>> >> Thanks,
>>> >> Albert
>>> >>
>>> >
>>> >
>>> > >
>>> >
>>>
>>> >>>
>>>
>>
>
--~--~---------~--~----~------------~-------~--~----~
Chromium Developers mailing list: chromium-dev@googlegroups.com
View archives, change email options, or unsubscribe:
http://groups.google.com/group/chromium-dev
-~----------~----~----~----~------~----~------~--~---
