-
Notifications
You must be signed in to change notification settings - Fork 544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fir(test/__init__.py): Ignoring DeprecationWarning from "cassandra.io.asyncioreactor" #1118
Conversation
@absurdfarce Can you review this PR? |
@Orenef11 So I'm not sure I quite understand the intent here. Are the DeprecationWarnings coming from cassandra.io.asyncioreactor causing tests to fail? Or is the goal here merely to suppress any output that might result from these warnings? If we're actually seeing tests fail because of this problem I agree something needs to be done, but if we're only trying to limit some output logging that might be a different story. How much noise do these exceptions actually generate? And is there a case for fixing the root cause of the deprecations rather than suppressing the errors overall? I guess I'm just a bit nervous about the idea of explicitly suppressing errors reported by tests. :( Your comments mentioned that these errors were only visible on Python 3.8. I haven't tried a test run with that version of Cython yet... suppose I could just run through that exercise rather than being lazy and asking you all these questions. :) |
@absurdfarce If you want, I can try to fix it for Python3.8. |
@Orenef11 If you'd like to try to resolve them I'm certainly fine with that. I'm trying to repro myself in order to see which deprecation messages are causing the most trouble.. but I don't think I'm doing the same thing you are:
Is there a cleaner way to repro the DeprecationWarnings you're seeing? |
@absurdfarce Run the test with
|
Ah, thanks for that @Orenef11 , that was extremely useful! I see the errors you're referencing now. Please let me know if you run into any trouble implementing a fix. It looks like you'll have to fix cassandra.io.asyncioreactor and likely update the asynctest dependency as well (since some of the error messages are coming from there as well). We also need to make sure any changes will also run without issue for older Python runtimes, specifically 3.5 (the oldest version we currently support in the 3.x line) as well as 2.7. Thanks, and again please let me know if you need need any help! |
2283cc6
to
8974779
Compare
@absurdfarce |
@absurdfarce |
ping @absurdfarce |
cassandra/io/asyncioreactor.py
Outdated
@@ -9,24 +14,16 @@ | |||
|
|||
|
|||
log = logging.getLogger(__name__) | |||
is_min_python_version_3_5 = (sys.version_info.major, sys.version_info.micro) >= (3, 5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ill change it to system.version_info >= (3, 5)
@Orenef11 Apologies, everyone here has been out for holiday/end-of-year reasons. I'll try to take another look at this soon! Thanks for your patience! |
NP, when you can please review it. |
8974779
to
19394c3
Compare
…yncio.coroutine * Replace all places that use old async node (`@asyncio.coroutine` and `yield from`) and use a new Python syntax (Only for Python v3.5+) * Save the old code to allow users with Python v2+ to run our tests. * requirements.txt: * Install `futures` package only for Python v2 * Use the latest `six` package version
…arning: invalid escape sequence \" warning
…Warning: invalid escape sequence \" warning
19394c3
to
39fcfbb
Compare
@Orenef11 My sincerest apologies for the extended delay in getting back to you on this. I've been swamped with work for some of the other drivers supported by DataStax but (as will hopefully become apparent soon) there's also been a fair amount of work going on behind the scenes for this effort as well. I was a bit nervous about the extensive modifications to the test framework (like those outlined in this PR) just to remove some deprecation warnings. Perhaps even more importantly I couldn't shake the feeling that the deprecation warning really was trying to tell us something. In the not-too-distant future I'd like to catch the Python driver up with more modern Python runtimes and deprecation warnings such as these were indicating an area that would need to be addressed as part of any such effort. So I got to wondering... what's actually going on here and is it possible to just fix the underlying condition? The results of that investigation are documented in PYTHON-1290. My conclusion was that given the set of Python versions we're tasked with supporting now it does make sense to fix the underlying issue directly. I've opened another PR to do exactly that. At this point I think I'd like to close this PR in favor of the underlying fix. What do you think? I would certainly welcome your review of the other PR as well. |
No problem, my goal is to improve the driver as much as I can :-) The current PR contains an additional commitment, can I open it in another PR? Last thing, is it possible to use |
Thanks (again) @Orenef11! I certainly appreciate all the effort you've put in so far! Just to make sure I have this right: #1120 is the other PR you mentioned, right? On the question of changing to pytest... where would you like to make this change? In the tox.ini setup for running tests (i.e. in favor of nose)? Or in the actual test classes themselves? |
its is other issue...
The short fix it change only the |
This issue has been addressed with the changes in #1119. Closing this one out. Re: changing how the unittests are run... I'm open to the idea of converting the process running the tests to pytest. I do not want to change the tests themselves to use the pytest API. unittest is part of the Python standard lib so I'm extremely reluctant to move away from that. |
@asyncio.coroutine
andyield from
) and use a new Python syntax (Only for Python v3.5+)futures
package only for Python v2sex
package version