Fix failed apk unit test case MessagePumpLibeventTest.* on Android.

Fix failed apk unit test case MessagePumpLibeventTest.* on Android.

The MessagePumpLibeventTest.StopWatcher and MessagePumpLibeventTest.DeleteWatcher
use fd 0, which is not permitted in apk test while calling 'epoll_ctl', and cause
failure on Android. Fix this by using fd created from pipe.

Also remove them from Android disabled case list.

BUG=None
TEST=base_unittests


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

[Shouqun Liu, 2012-07-06 06:01:12]

This patch fixes failed apk unit test case  MessagePumpLibeventTest.* on
Android. Since fd 0 is not permitted to be used in apk for epoll_ctl call, use
fd created from pipe instead.

[nilesh, 2012-07-09 17:35:59]

On 2012/07/06 06:01:12, Shouqun Liu wrote:
> This patch fixes failed apk unit test case  MessagePumpLibeventTest.* on
> Android. Since fd 0 is not permitted to be used in apk for epoll_ctl call, use
> fd created from pipe instead.

LG. Thanks for fixing this. You still need owner approvals.
Please try the change on try bots (including android_test)

[jar (doing other things), 2012-07-09 21:12:23]

http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
File base/message_pump_libevent_unittest.cc (right):

http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
base/message_pump_libevent_unittest.cc:67: TEST_F(MessagePumpLibeventTest,
TestWatchingFromBadThread) {
Where is TestWatchingFromBadThread defined?

http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
base/message_pump_libevent_unittest.cc:149: int fd = pipefds[1];
I hate seeing growth of replicated code... 

WDYT of:

Put this creation/closing into the constructor and destructor of the test class.
 This way it is harder to "not clean up" in the test.

Given that DeleteWatcher and StopWatcher are almost identical (and grow larger
and yet more identical with this constructor/destructor addition), combine them
into one Watcher class, with an enum arg to the constructor indicating if it
should DELETE or just STOP.

You can then have two separately named derived clases StopWatcher and
DeleteWatcher that pass in the two choice DELETE vs STOP, and use them in the
test (as they are now).

Code gets bloated more and more as folks make these changes... and it would be
nice to clean as you fix.

WDYT?

[Shouqun Liu, 2012-07-10 05:42:41]

Hi jar,  I have updated the patch according to your suggestion, comments are
below, thanks for review

http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
File base/message_pump_libevent_unittest.cc (right):

http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
base/message_pump_libevent_unittest.cc:67: TEST_F(MessagePumpLibeventTest,
TestWatchingFromBadThread) {
This is a death test, I think we don't need to define TestWatchingFromBadThread
On 2012/07/09 21:12:23, jar wrote:
> Where is TestWatchingFromBadThread defined?

http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
base/message_pump_libevent_unittest.cc:149: int fd = pipefds[1];
Yes, agree with you.  I have made the following changes:
1, move the creation/deletion of pipe into the SetUp/TearDown of test case class
MessagePumpLibeventTest to reducing replicated code.
2, Use a base class BaseWatcher to merge StopWatcher/DeleteWatcher, and
StopWatcher/DeleteWatcher derived from BaseWatcher, TYPE_STOP/TYPE_DELETE in
constructor to control which one should be used.
On 2012/07/09 21:12:23, jar wrote:
> I hate seeing growth of replicated code... 
> 
> WDYT of:
> 
> Put this creation/closing into the constructor and destructor of the test
class.
>  This way it is harder to "not clean up" in the test.
> 
> Given that DeleteWatcher and StopWatcher are almost identical (and grow larger
> and yet more identical with this constructor/destructor addition), combine
them
> into one Watcher class, with an enum arg to the constructor indicating if it
> should DELETE or just STOP.
> 
> You can then have two separately named derived clases StopWatcher and
> DeleteWatcher that pass in the two choice DELETE vs STOP, and use them in the
> test (as they are now).
> 
> Code gets bloated more and more as folks make these changes... and it would be
> nice to clean as you fix.
> 
> WDYT?

[jar (doing other things), 2012-07-10 17:02:10]

Changes to test class are very good. I think making the fd there is much better.
Thanks.

Looking at the implementation of the watcher classes (and contrary to my
previous thoughts) I realized you don't actually need/use |type_| in the base
class, so it is a waste (you correctly overrode the functions you needed to in
the derived classes).

I'd suggest killing the enum, and the type_ slot and its initialization.

http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
File base/message_pump_libevent_unittest.cc (right):

http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
base/message_pump_libevent_unittest.cc:67: TEST_F(MessagePumpLibeventTest,
TestWatchingFromBadThread) {
<doh>  For some reason I was confusing this test name with the class name...
sorry.  I don't know what I was thinking. <sigh>

Nevermind. 

On 2012/07/10 05:42:41, Shouqun Liu wrote:
> This is a death test, I think we don't need to define
TestWatchingFromBadThread
> On 2012/07/09 21:12:23, jar wrote:
> > Where is TestWatchingFromBadThread defined?
>

http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
File base/message_pump_libevent_unittest.cc (right):

http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
base/message_pump_libevent_unittest.cc:96: explicit BaseWatcher(
[if you don't kill type_]...

nit: don't "explicit" any more... and you might then fit line 97 onto this line.

http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
base/message_pump_libevent_unittest.cc:98: : controller_(controller),
type_(type) {
[if you don't kill type_]...

nit: one initialization per line, once you start wrapping.

[Shouqun Liu, 2012-07-11 00:49:11]

On 2012/07/10 17:02:10, jar wrote:
> Changes to test class are very good. I think making the fd there is much
better.
> Thanks.
> 
> Looking at the implementation of the watcher classes (and contrary to my
> previous thoughts) I realized you don't actually need/use |type_| in the base
> class, so it is a waste (you correctly overrode the functions you needed to in
> the derived classes).
> 
> I'd suggest killing the enum, and the type_ slot and its initialization.
> 
>
http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
> File base/message_pump_libevent_unittest.cc (right):
> 
>
http://codereview.chromium.org/10698112/diff/3001/base/message_pump_libevent_...
> base/message_pump_libevent_unittest.cc:67: TEST_F(MessagePumpLibeventTest,
> TestWatchingFromBadThread) {
> <doh>  For some reason I was confusing this test name with the class name...
> sorry.  I don't know what I was thinking. <sigh>
> 
> Nevermind. 
> 
> On 2012/07/10 05:42:41, Shouqun Liu wrote:
> > This is a death test, I think we don't need to define
> TestWatchingFromBadThread
> > On 2012/07/09 21:12:23, jar wrote:
> > > Where is TestWatchingFromBadThread defined?
> >
> 
>
http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
> File base/message_pump_libevent_unittest.cc (right):
> 
>
http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
> base/message_pump_libevent_unittest.cc:96: explicit BaseWatcher(
> [if you don't kill type_]...
> 
> nit: don't "explicit" any more... and you might then fit line 97 onto this
line.
> 
>
http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
> base/message_pump_libevent_unittest.cc:98: : controller_(controller),
> type_(type) {
> [if you don't kill type_]...
> 
> nit: one initialization per line, once you start wrapping.

Hi jar, patch updated,  seems type is not useful,  so I removed them and refined
the patch

[Shouqun Liu, 2012-07-11 00:49:37]

http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
File base/message_pump_libevent_unittest.cc (right):

http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
base/message_pump_libevent_unittest.cc:96: explicit BaseWatcher(
On 2012/07/10 17:02:10, jar wrote:
> [if you don't kill type_]...
> 
> nit: don't "explicit" any more... and you might then fit line 97 onto this
line.

Done.

http://codereview.chromium.org/10698112/diff/12001/base/message_pump_libevent...
base/message_pump_libevent_unittest.cc:98: : controller_(controller),
type_(type) {
On 2012/07/10 17:02:10, jar wrote:
> [if you don't kill type_]...
> 
> nit: one initialization per line, once you start wrapping.

Remove the type, done.

[jar (doing other things), 2012-07-11 01:18:46]

lgtm

[commit-bot: I haz the power, 2012-07-11 01:29:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shouqun.liu@intel.com/10698112/17001

[commit-bot: I haz the power, 2012-07-11 03:25:41]

Change committed as 146041