Skip to content

Issue #562 - Ensure @nose.tools.istest is respected #921

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

Merged
merged 4 commits into from
Aug 9, 2015

Conversation

tomviner
Copy link
Contributor

@tomviner tomviner commented Aug 6, 2015

Fix for #562

@@ -335,6 +337,9 @@ class PyCollector(PyobjMixin, pytest.Collector):
def funcnamefilter(self, name):
return self._matches_prefix_or_glob_option('python_functions', name)

def isnosetestfunction(self, obj):
return getattr(obj, '__test__', False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_parsefactories_evil_objects_issue214 is failing, which shows that in order to be safe during collection (#214), this would be safer:

def isnosetestfunction(self, obj):
    try:
        return obj.__test__
    except Exception:
        return False

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not except Exception: to avoid catching KeyboardInterrupt and the like?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, brain fart, you're right of course. Thanks! (updated my example)

@nicoddemus
Copy link
Member

Thanks for the PR @tomviner! 😄

Besides the comment I made regarding the failing test, the patch looks good to me! Could you also please add yourself to AUTHORS and a note to the CHANGELOG?

@@ -248,8 +248,10 @@ def pytest_pycollect_makeitem(collector, name, obj):
if collector.classnamefilter(name):
Class = collector._getcustomclass("Class")
outcome.force_result(Class(name, parent=collector))
elif collector.funcnamefilter(name) and hasattr(obj, "__call__") and\
getfixturemarker(obj) is None:
elif (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That condition is starting to be waaay to complex, can you turn it into a method?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tomviner tomviner force-pushed the issue562-noseistest branch 2 times, most recently from 5210838 to b3e9df5 Compare August 8, 2015 16:47
@tomviner
Copy link
Contributor Author

tomviner commented Aug 8, 2015

This patch is now made up of:

  • test functions and classes are now selected if they match the naming rules or have @nose.tools.istest applied
    • including little refactor /cope with evil cases, as suggested above - thx
    • replacing a couple inspect.isclass with isclass which is local-namespaced at the top of python.py
  • tests for this and @nose.tools.nottest
  • changelog update
  • I'm already in authors

I'd have liked to have made these changes solely in nose.py (rather than python.py), does anyone think that would have been possible?

So we now match the nose functionality of allowing functions and classes (but not modules) to be selected regardless of name with the @istest / __test__ = True. The negative case was already supported by py.test and does cover modules too, so setting __test__ = False on a module will prevent all tests being selected.

Here's a final test on the command line, using these 3 test files:

==> check.py <==
# regardless of this, the filename mismatch precludes this having any effect
__test__ = True

def test_me__not_run():
    pass

==> test_false.py <==
# nothing gets run in this module
__test__ = False

def test_me__not_run():
    pass

==> test_things.py <==
import nose.tools

# will not get run

@nose.tools.nottest
def test_stuff__not_run():
    pass

@nose.tools.nottest
class TestPrefix():
    def test_stuff__not_run(self):
        pass


# will run

@nose.tools.istest
class AnyName:
    def test_stuff_method__will_run(self):
        pass

@nose.tools.istest
def stuff__will_run():
    pass

With nose (nosetests version 1.3.7), 2 tests run:

$ nosetests -v
test_things.AnyName.test_stuff_method__will_run ... ok
test_things.stuff__will_run ... ok

With latest py.test:

$ py.test -v
=== test session starts ===
platform linux2 -- Python 2.7.9 -- py-1.4.30 -- pytest-2.7.2 -- /home/tom/.virtualenvs/pytest-lib/bin/python 
collected 0 items 

With py.test of this branch:

$ py.test -v
=== test session starts ===
platform linux2 -- Python 2.7.9, pytest-2.8.0.dev4, py-1.4.30, pluggy-0.3.0 -- /home/tom/.virtualenvs/pytest/bin/python
collected 2 items 

test_things.py::AnyName::test_stuff_method__will_run PASSED
test_things.py::stuff__will_run PASSED

@tomviner tomviner force-pushed the issue562-noseistest branch 2 times, most recently from e841e73 to df44333 Compare August 8, 2015 17:22
@tomviner
Copy link
Contributor Author

tomviner commented Aug 8, 2015

Rebased.

def istestfunction(self, obj, name):
return (
(self.funcnamefilter(name) or self.isnosetest(obj))
and hasattr(obj, "__call__") and getfixturemarker(obj) is None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the __call__ attribute should be checked with safe_getattr as wel

@RonnyPfannschmidt
Copy link
Member

overall nicely done, great job :)

@tomviner tomviner force-pushed the issue562-noseistest branch 2 times, most recently from 9b2639c to a5c3c07 Compare August 8, 2015 17:35
@tomviner
Copy link
Contributor Author

tomviner commented Aug 8, 2015

cheers, and changed that to safe_getattr @RonnyPfannschmidt

@nicoddemus
Copy link
Member

Small flakes failure, could you please fix it? 😄

After this passes this is good to merge from my POV.

@tomviner
Copy link
Contributor Author

tomviner commented Aug 8, 2015

Ah yes. Currently out flying drone, will fix upon return.

@tomviner tomviner force-pushed the issue562-noseistest branch from a5c3c07 to 66d20a9 Compare August 8, 2015 20:07
@tomviner tomviner force-pushed the issue562-noseistest branch 2 times, most recently from 54bb63d to ad4dee3 Compare August 8, 2015 23:08
@tomviner tomviner force-pushed the issue562-noseistest branch from ad4dee3 to 1462590 Compare August 8, 2015 23:11
@tomviner
Copy link
Contributor Author

tomviner commented Aug 8, 2015

Flakes deflaked, couple docstrings clarified/detypoed too. All good?

@coveralls
Copy link

Coverage Status

Coverage decreased (-30.8%) to 61.556% when pulling 1462590 on tomviner:issue562-noseistest into 729b5e9 on pytest-dev:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants