remove remaining run_sync calls from tests#6196
Conversation
20fab69 to
0d8ffca
Compare
| try: | ||
| res = thread_loop.run_sync(partial(func, *args, **kwargs), timeout=10) | ||
| except Exception: | ||
| main_loop.add_callback(fut.set_exc_info, sys.exc_info()) |
There was a problem hiding this comment.
the set_exc_info method is not available since tornado v5
0d8ffca to
75fbbd2
Compare
75fbbd2 to
aa0c493
Compare
test failures seem unrelated |
sjperkins
left a comment
There was a problem hiding this comment.
This PR replaces the use of loop fixture and associated pristine_loop function with higher-level @gen_test decorators and cleanup fixtures. As I read the code, these higher-level constructs create pristine loops which are available for use by the asyncio interface, avoiding the need to call loop.run_sync in the test cases.
| be used to determine if it exited correctly.""" | ||
|
|
||
| async def parent_process_coroutine(): | ||
| IOLoop.current() |
There was a problem hiding this comment.
I understand this is to ensure that the IOLoop has been created in this process? This then replaces the former pristine_loop test construct lower down and standard asyncio is used from that point on?
I think this is fine because the process is explicitly spawned as part of the test and won't have it's own IOLoop yet.
There was a problem hiding this comment.
Yeah, AsyncProcess uses IOLoop.current(instance=False) which fails if a tornado loop hasn't been created.
Calling IOLoop.current() is only deprecated when there isn't a running asyncio loop
|
|
||
|
|
||
| def test_All(loop): | ||
| @gen_test() |
There was a problem hiding this comment.
@gen_test should take care of providing a pristine IOLoop because it invokes test_All within a clean() context?
|
|
||
|
|
||
| def test_retry_no_exception(loop): | ||
| def test_retry_no_exception(cleanup): |
There was a problem hiding this comment.
I see the cleanup fixture also yields a pristine loop for sync test functions. It seems gen_test is the counterpart for async functions?
A nice to have might be to have gen_test_async and gen_test_sync decorators, but I realise I don't have a lot of context here. This shouldn't block the PR.
There was a problem hiding this comment.
I'll be removing the call to pristine_loop in cleanup in my next PR
|
I just re-triggered the failing job and we can merge if this turns out to be green |
followup to #6170
This PR removes the tornado run_sync calls from the body of tests
pre-commit run --all-files