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
Add deprecations for tests written for nose
#9907
Conversation
src/_pytest/nose.py
Outdated
@@ -38,5 +41,6 @@ def call_optional(obj: object, name: str) -> bool: | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if we can unwind this into a user-space to be more helpful
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 True
which 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.
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.
Thanks @symonk!
Left a few suggestions, please take a look. 👍
src/_pytest/nose.py
Outdated
@@ -38,5 +41,6 @@ def call_optional(obj: object, name: str) -> bool: | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
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. 👍 |
nose
nose
[SQUASH]
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.
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...
testing/deprecated_test.py
Outdated
@@ -231,3 +231,31 @@ def test_importing_instance_is_deprecated(pytester: Pytester) -> None: | |||
match=re.escape("The pytest.Instance collector type is deprecated"), | |||
): | |||
from _pytest.python import Instance # noqa: F401 | |||
|
|||
|
|||
def test_nose_deprecated(pytester: Pytester) -> None: |
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.
Should also have a test using bare setup
and teardown
methods?
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.
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! |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Indeed, done. 👍
nose
[SQUASH]nose
Thanks @symonk and @The-Compiler! |
Adds deprecation warnings for tests written in
nose
, namely aroundsetup
andteardown
aspects; as we support limited functionality.I went with a
pytest 8.x
deprecation 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 docs
does not work in python3.10+
due to an upstream issue insphinx/util/typing.py
we may need a version bump there? I will chase that up in a secondcloses #9886
Todo:
call_optional(...)
@with_setup
etc.