Make test_events more reliable, avoid run_briefly

Make test_events more reliable, avoid run_briefly

[GvR, 2014-03-03 18:15:14]

I like this approach!

https://codereview.appspot.com/67870052/diff/20001/asyncio/test_utils.py
File asyncio/test_utils.py (right):

https://codereview.appspot.com/67870052/diff/20001/asyncio/test_utils.py#newc...
asyncio/test_utils.py:55: def run_until(loop, pred, timeout=5 * 60):
It looks ugly with the space around the '*' but not around the '='. I recommend
writing either 5*60 or 300.

However, no tests should even wait more than a second, and when a test fails on
a manual run it's annoying to wait 5 minutes. Maybe set it to 5 seconds?

https://codereview.appspot.com/67870052/diff/20001/asyncio/test_utils.py#newc...
asyncio/test_utils.py:61: raise TimeoutError()
I don't think this is the exception you think it is. It's a subclass of OSError
meant for errno.ETIMEDOUT.

https://codereview.appspot.com/67870052/diff/20001/asyncio/test_utils.py#newc...
asyncio/test_utils.py:62: if loop._ready:
I'm uncomfortable using an internal variable here. Perhaps you can just wait for
1 msec since it's a test loop anyway?

https://codereview.appspot.com/67870052/diff/20001/asyncio/test_utils.py#newc...
asyncio/test_utils.py:65: loop.run_until_complete(tasks.sleep(0.100, loop=loop))
1/10th of a second is an eternity for most unittests. Since you're doing it in a
loop anyway, why not use 1 msec?

https://codereview.appspot.com/67870052/diff/20001/tests/test_events.py
File tests/test_events.py (right):

https://codereview.appspot.com/67870052/diff/20001/tests/test_events.py#newco...
tests/test_events.py:62: def __init__(self, loop=None):
(Are there actually any calls where loop is None? Consider making this a
required argument?)

https://codereview.appspot.com/67870052/diff/20001/tests/test_events.py#newco...
tests/test_events.py:367: # FIXME: use something more reliable
We spell that 'TODO'. :-)

Are you planning to fix it in this patch or are you just reminding yourself that
another patch is needed later?

https://codereview.appspot.com/67870052/diff/20001/tests/test_events.py#newco...
tests/test_events.py:643: self.loop.run_until_complete(proto.connected)
Wow. The previous version here was broken. :-(

https://codereview.appspot.com/67870052/diff/20001/tests/test_events.py#newco...
tests/test_events.py:656: self.loop.run_until_complete(proto.done)
So did you test this still passes on Windows?

[haypo_gmail, 2014-03-04 17:36:39]

https://codereview.appspot.com/67870052/diff/20001/tests/test_events.py
File tests/test_events.py (right):

https://codereview.appspot.com/67870052/diff/20001/tests/test_events.py#newco...
tests/test_events.py:62: def __init__(self, loop=None):
On 2014/03/03 18:15:15, GvR wrote:
> (Are there actually any calls where loop is None? Consider making this a
> required argument?)

Yes, sometimes you don't need the waiters (futures) and loop is not set. It's
convinient to be able to pass the class instead of having to write lambda:
MyBaseProto(loop=self.loop).

I don't think that we need to make it mandatory.

https://codereview.appspot.com/67870052/diff/20001/tests/test_events.py#newco...
tests/test_events.py:656: self.loop.run_until_complete(proto.done)
On 2014/03/03 18:15:15, GvR wrote:
> So did you test this still passes on Windows?

I didn't run tests on Windows yet.

[haypo_gmail, 2014-03-04 23:56:39]

On 2014/03/04 17:36:39, haypo_gmail wrote:
> I didn't run tests on Windows yet.

Hum, the patch set 3 doesn't work because it calls w.send() multiple times and
w.send() may fail with BlockingIOError(10035). It should be fixed in patch set 4
which only calls w.send() once (stop the loop immediatly). The new tests ensure
also that the second call to remove_writer() returns False.

[GvR, 2014-03-05 19:18:57]

On 2014/03/04 23:56:39, haypo_gmail wrote:
> On 2014/03/04 17:36:39, haypo_gmail wrote:
> > I didn't run tests on Windows yet.
> 
> Hum, the patch set 3 doesn't work because it calls w.send() multiple times and
> w.send() may fail with BlockingIOError(10035). It should be fixed in patch set
4
> which only calls w.send() once (stop the loop immediatly). The new tests
ensure
> also that the second call to remove_writer() returns False.

I think the original test was testing something more. It was calling send() 256K
bytes, which might cause a partial write (the OS buffer for the socket pair is
probably smaller) and it was only expecting to read *some* data (at least 200
bytes, for some reason). Your new test just writes 1000 bytes (which should
always fit) and expects to receive all of that.

Thinking about it I don't think the OS behavior with large writes is what we're
trying to test here though, so I'm OK with this version if you are. I.e. LGTM if
you still think this is right.

[haypo_gmail, 2014-03-06 00:12:01]

On 2014/03/05 19:18:57, GvR wrote:
> I think the original test was testing something more. It was calling send()
256K
> bytes, which might cause a partial write (the OS buffer for the socket pair is
> probably smaller) and it was only expecting to read *some* data (at least 200
> bytes, for some reason). Your new test just writes 1000 bytes (which should
> always fit) and expects to receive all of that.

Yes, the old test was testing something different, but it didn't work on
Windows.

Sorry, I don't have time right now to write a new test for partial send. Windows
behaviour is weird, writing such test is not trivial.

I applied my test, I believe that it makes test_events.py better (I found at
least one bug ;-)).