|
|
|
Created:
11 years, 10 months ago by haypo_gmail Modified:
11 years, 10 months ago Reviewers:
GvR Visibility:
Public. |
Make test_events more reliable, avoid run_briefly
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : #Patch Set 4 : Fix Windows #Total messages: 5
|
GvR
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#newcode55 asyncio/test_utils.py:55: def run_until(loop, pred, timeout=5 * ...
|
11 years, 10 months ago (2014年03月03日 18:15:14 UTC) #1 |
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?
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.
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.
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.
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 ;-)).