Add deprecations for tests written for nose#9907
Conversation
| return False | ||
| # If there are any problems allow the exception to raise rather than | ||
| # silently ignoring it. | ||
| warnings.warn(NOSE_SUPPORT, stacklevel=2) |
There was a problem hiding this comment.
not sure if we can unwind this into a user-space to be more helpful
There was a problem hiding this comment.
Indeed, but I think we should at least display the method name (and module), as well as the test that triggered this, if possible.
There was a problem hiding this comment.
any preferred way to handle this one @nicoddemus; we have the item in scope when called, could just pass the nodeid into a formattable warning? however some unit tests don't pass that obviously; I'm not sure how many edge cases there can be for this nose stuff; I've everything else fixed up I think but as-is its something like:
def call_optional(obj: object, name: str, nodeid: str) -> bool:
method = getattr(obj, name, None)
if method is None:
return False
is_fixture = getfixturemarker(method) is not None
if is_fixture:
return False
if not callable(method):
return False
# If there are any problems allow the exception to raise rather than
# silently ignoring it.
warnings.warn(NOSE_SUPPORT.format(nodeid=nodeid, nose_method=name), stacklevel=2)
method()
return Truewhich I don't really like; ok to use inspect here to derive some of that info? or perhaps __qualname__ / __module__ would suffice on the existing function obj passed?
Will push what I have for now as I'm out of time today; not happy with it tho so i'm hoping to change the call_optional here still based on your feedback. Please have a look at see what you think the cleanest way to handle that is; will update when I get back, thanks 👍 (I'm expecting some unit test failures now, on the call_optional(...) specifics.
nicoddemus
left a comment
There was a problem hiding this comment.
Thanks @symonk!
Left a few suggestions, please take a look. 👍
| return False | ||
| # If there are any problems allow the exception to raise rather than | ||
| # silently ignoring it. | ||
| warnings.warn(NOSE_SUPPORT, stacklevel=2) |
There was a problem hiding this comment.
Indeed, but I think we should at least display the method name (and module), as well as the test that triggered this, if possible.
|
Does this only affect |
Good point. In that case they will get a warning, and we should mention in the deprecation docs how to port it to use something pytest-native. |
|
Moved to 7.2 milestone. 👍 |
|
Sorry folks, I will get this one wrapped up shortly |
|
@symonk gentle ping. 😁 |
will get a look, thanks! |
This comment was marked as spam.
This comment was marked as spam.
nicoddemus
left a comment
There was a problem hiding this comment.
Thanks @symonk!
Went ahead and fixed the tests and added information on how to port code in the deprecation docs. 👍
|
@The-Compiler would appreciate a review if possible. 👍 |
nosenose [SQUASH]
There was a problem hiding this comment.
Seems fine implementation-wise other than a few doc nitpicks (feel free to ignore if you disagree), and perhaps a missing test.
I'm still somewhat lukewarm about dropping bare setup and teardown methods... Unfortunately, I see no easy way to find out how much usage of those there is, as searching for it will of course also pick up fixtures (only limited to repos containing conftest.py to try and avoid unittest.py testsuites).
Maybe we'll just need to do it and see how many people complain...
edit: The more I look at it, the more I seem to see genuine cases of this being used in numpy, celery, ceph, and similar high-profile projects...
| from _pytest.python import Instance # noqa: F401 | ||
|
|
||
|
|
||
| def test_nose_deprecated(pytester: Pytester) -> None: |
There was a problem hiding this comment.
Should also have a test using bare setup and teardown methods?
There was a problem hiding this comment.
Glad you asked this, it was not emitting a warning for setup and teardown 😬
I see, thanks for looking into it. I strongly believe we should deprecate
Implementing 2) should be trivial, we already support |
Hi! Sopel dev here, and here is our story. For the next major release of our IRC Bot, we try to support Python 3.10 and the latest version of We were very happy when we understood the problem, and the solution: we deactivated the nose plugin, and all our problems went away. So, from our perspective, we are fine with the following options:
Basically: we really need to make sure we can still run pytest without having to change our codebase, or our user's codebase, to accommodate for something that was not, as far as I know, officially documented (at least until now). |
Just to make it clear, my proposal is to support |
Oh! You're right, I was worried about functions, not methods. In that case, and since pytest has a configuration for class naming convention, we would not have any problem with that. Thanks for the clarification. 👍 |
|
Thanks @Exirel for the feedback! 👍 I updated my original post to make it clearer too. |
|
@The-Compiler what do you think regarding #9907 (comment)? I'm leaning towards 1), mostly because we are showing a deprecation, and the fix is trivial by doing a simple find/replace. |
|
The warning message now includes the text |
Seems fine to me, though I see one problem: Once the deprecation period is over, it turns into a silent failure, depending on what the user does in those methods. |
Yeah I agree it is not perfect, unfortunately. We should make a point to add a big note to the release notes when we drop the functionality, I think is the best we can do. |
|
Btw @The-Compiler could you give this another pass over? Thanks! |
The-Compiler
left a comment
There was a problem hiding this comment.
Looks great! Just a few small doc comments. Don't feel strongly about them, except the changelog one.
| @@ -0,0 +1 @@ | |||
| The functionality for running tests written for ``nose`` has been officially deprecated. | |||
There was a problem hiding this comment.
This should be a bit more verbose: It should point out that this also includes setup and teardown methods (since users don't seem to be aware of that being a nose thing), and it should have a link to the detailed deprecation docs.
nose [SQUASH]nose
|
Thanks @symonk and @The-Compiler! |
Adds deprecation warnings for tests written in
nose, namely aroundsetupandteardownaspects; as we support limited functionality.I went with a
pytest 8.xdeprecation as I think that is the norm here. I also notice a few things when reviewing docs locally:my version was showingwas my fault;6.3.x-dev*tox -e docsdoes not work in python3.10+due to an upstream issue insphinx/util/typing.pywe may need a version bump there? I will chase that up in a secondcloses #9886
Todo:
call_optional(...)@with_setupetc.