Fix run_until_complete

Additional related tests in this previous review: https://codereview.appspot.com/7312046/

[GvR, 2013-02-11 21:35:11]

https://codereview.appspot.com/7301076/diff/1/tulip/base_events.py
File tulip/base_events.py (right):

https://codereview.appspot.com/7301076/diff/1/tulip/base_events.py#newcode97
tulip/base_events.py:97: def _stop():
No need to name it with an underscore -- but perhaps give it a longer name, e.g.
stop_running.

https://codereview.appspot.com/7301076/diff/1/tulip/base_events.py#newcode100
tulip/base_events.py:100: _raise_stop_error()
No need to use that -- you can just raise _StopError.

https://codereview.appspot.com/7301076/diff/1/tulip/base_events.py#newcode101
tulip/base_events.py:101: handler_called = False
I'd put this before the definition of _stop().

https://codereview.appspot.com/7301076/diff/1/tulip/base_events.py#newcode102
tulip/base_events.py:102: future.add_done_callback(lambda _: self.stop())
If you give _raise_stop_error an optional (ignored) argument you can get rid of
the lambda.

https://codereview.appspot.com/7301076/diff/1/tulip/tasks_test.py
File tulip/tasks_test.py (right):

https://codereview.appspot.com/7301076/diff/1/tulip/tasks_test.py#newcode144
tulip/tasks_test.py:144: x = 0
Move this before the task.

https://codereview.appspot.com/7301076/diff/1/tulip/tasks_test.py#newcode146
tulip/tasks_test.py:146: self.event_loop.run_until_complete(t)
Ought to test that the time elapsed roughly matches the expected (0.5 seconds,
but see below).

https://codereview.appspot.com/7301076/diff/1/tulip/tasks_test.py#newcode148
tulip/tasks_test.py:148: self.assertEqual(x, 5)
Can we make the count or the sleep a little smaller? half a second for one test
is an eternity.

[saghul, 2013-02-11 21:54:04]

I (hopefully) addressed all your comments on patch set 2.

[GvR, 2013-02-11 22:27:45]

See also my musings on python-ideas about the proper return value of
run_until_complete()...

https://codereview.appspot.com/7301076/diff/5001/tulip/tasks_test.py
File tulip/tasks_test.py (right):

https://codereview.appspot.com/7301076/diff/5001/tulip/tasks_test.py#newcode140
tulip/tasks_test.py:140: yield from tasks.sleep(0.01)
Hm. I think 0.01 is a little too small (only 10 msec). How about sleeping 0.1
but waiting only for x == 2?

https://codereview.appspot.com/7301076/diff/5001/tulip/tasks_test.py#newcode150
tulip/tasks_test.py:150: self.assertTrue(t1-t0 < 0.1)
Should check that it is in a certain range (there are several other tests you
can use as an example).

https://codereview.appspot.com/7301076/diff/5001/tulip/tasks_test.py#newcode163
tulip/tasks_test.py:163: self.assertFalse(t.done())
I think this should also assert that time elapsed is close to 0.1, e.g.

self.assertTrue(0.08 <= t1-t0 <= 0.12)

And ditto for the next test function.

[saghul, 2013-02-11 22:40:04]

Updated addressing the comments above and those on the mailing list, if the loop
is stopped prematurely futures.InvalidStateError will be raised since
future.result() is always called if there was no timeout.

[GvR, 2013-02-11 22:57:35]

LGTM -- you can just fix the one nit below and submit. Thanks!!

https://codereview.appspot.com/7301076/diff/7002/tulip/tasks_test.py
File tulip/tasks_test.py (right):

https://codereview.appspot.com/7301076/diff/7002/tulip/tasks_test.py#newcode152
tulip/tasks_test.py:152: self.assertTrue(0.2 <= t1-t0 <= 0.5)
Shouldn't this be 0.18 <= t1-t0 <= 0.22?

[saghul, 2013-02-11 23:03:20]

On 2013/02/11 22:57:35, GvR wrote:
> LGTM -- you can just fix the one nit below and submit. Thanks!!
> 
> https://codereview.appspot.com/7301076/diff/7002/tulip/tasks_test.py
> File tulip/tasks_test.py (right):
> 
>
https://codereview.appspot.com/7301076/diff/7002/tulip/tasks_test.py#newcode152
> tulip/tasks_test.py:152: self.assertTrue(0.2 <= t1-t0 <= 0.5)
> Shouldn't this be 0.18 <= t1-t0 <= 0.22?

Committed in 7ec755b2196b, I also fixed the timeout check, Thanks!