Update Kernel Gateway test base class to be compatible with Tornado 5.0 #285
Conversation
|
Thanks @akchinSTC . In general, I'm OK with saying the next release of KG is only compatible with tornado 5.0 and up to avoid having multiple code paths to maintain and keeping the requirement as |
|
@parente - I'm a little hesitant to move the floor to Tornado 5.0 at this time. Of course, on fresh systems this really makes no difference since the latest versions of everything are installed. But for upgrade situations - where the jupyter framework is already in place albeit JKG may or may not - this could introduce some issues. I believe I found a conflict between Tornado (5) and pyzmq versions where 5.0 required an updated pyzmg. Also, I think it best to leave some fallback wiggle room in case new issues arise related to Tornado 5.0. The reason I bring up the concern of upgrade situations is that Enterprise Gateway is literally hours away from publishing its 0.9 release and have not moved Tornado's floor. I was also hoping we could publish a 2.0.3 JKG release shortly and was going to ask for that following the merge of this PR. Enterprise Gateway is not planning on moving the floor for its JKG dependency at this time (will likely do so for its 1.0 release), but would recommend that customers upgrade its JKG to 2.0.3 in order to take advantage of some of the changes (e.g., the ability for custom error messages to return to the client #284; #280 looks useful as well). As a result, JKG's Tornado floor would essentially force the issue for JEG upgrades. I hope this makes sense. |
|
Would it suffice to run the unit tests with Python 5.0, without raising the runtime prerequisite? |
| @@ -381,13 +382,21 @@ def test_concurrent_request_should_not_be_blocked(self): | |||
| method='GET', | |||
| raise_error=False | |||
| ) | |||
| self.assertTrue(response_long_running.running(), 'Long HTTP Request is not running') | |||
| if tornado.version.startswith("4"): | |||
There was a problem hiding this comment.
Could we avoid the version check and instead duck-type the object? Something like:
if callable(getattr(response_long_running, 'done', Null)):
# Tornado 5 or higher
else:
# Tornado 4Found the idea on Stack Overflow.
By the way, the check as it is now will yield a wrong result for Tornado 40 to 49 ;-)
There was a problem hiding this comment.
Thanks @rolweber ! Made the changes you suggested.
|
Go ahead and merge it if you're OK with it. Please don't wait on me. |
No description provided.