Skip to content
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

unittest: deprecate test methods returning non-None values #85494

Closed
defreng mannequin opened this issue Jul 17, 2020 · 27 comments
Closed

unittest: deprecate test methods returning non-None values #85494

defreng mannequin opened this issue Jul 17, 2020 · 27 comments
Labels
3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@defreng
Copy link
Mannequin

defreng mannequin commented Jul 17, 2020

BPO 41322
Nosy @terryjreedy, @rbtcollins, @ezio-melotti, @voidspace, @ambv, @zware, @serhiy-storchaka, @tirkarthi, @akulakov, @softsol solutions, @nightlark
PRs
  • bpo-41322: added deprecation warning for tests returning value!=None #27748
  • bpo-41322: add two unit tests for deprecation of test return values #27846
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-08-22.20:43:31.515>
    created_at = <Date 2020-07-17.09:46:57.312>
    labels = ['type-feature', 'library', '3.11']
    title = 'unittest: deprecate test methods returning non-None values'
    updated_at = <Date 2021-08-22.20:43:31.514>
    user = 'https://bugs.python.org/defreng'

    bugs.python.org fields:

    activity = <Date 2021-08-22.20:43:31.514>
    actor = 'lukasz.langa'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-08-22.20:43:31.515>
    closer = 'lukasz.langa'
    components = ['Library (Lib)']
    creation = <Date 2020-07-17.09:46:57.312>
    creator = 'defreng'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41322
    keywords = ['patch']
    message_count = 27.0
    messages = ['373807', '373810', '373812', '373816', '373824', '373850', '373874', '373876', '399901', '399902', '399915', '399923', '399927', '399938', '400010', '400013', '400014', '400015', '400016', '400017', '400019', '400028', '400029', '400030', '400031', '400087', '400105']
    nosy_count = 12.0
    nosy_names = ['terry.reedy', 'rbcollins', 'ezio.melotti', 'michael.foord', 'lukasz.langa', 'zach.ware', 'serhiy.storchaka', 'xtreak', 'defreng', 'andrei.avk', 'santhu_reddy12', 'rmast']
    pr_nums = ['27748', '27846']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue41322'
    versions = ['Python 3.11']

    @defreng
    Copy link
    Mannequin Author

    defreng mannequin commented Jul 17, 2020

    The following testcase will always be marked as passed:

    from unittest import TestCase
    
    class BuggyTestCase(TestCase):
        def test_generator(self):
            self.assertTrue(False)
            yield None

    It happened to us that someone accidentally made the test method a generator function. That error was caught very late, because it always appears to have executed correctly.

    Maybe an additional check can be introduced in the unittest module, to check if a test_ method was executed correctly?

    @defreng defreng mannequin added 3.8 only security fixes type-bug An unexpected behavior, bug, or error stdlib Python modules in the Lib dir labels Jul 17, 2020
    @tirkarthi
    Copy link
    Member

    def _callTestMethod(self, method):
    . _callTestMethod just calls the test method and doesn't check for the method to be a generator function to be iterated through. In Python 3.8 the call to test method was separated out as _callTestMethod. So something like below added in the original method should work. I guess there is an existing issue for this.

    import unittest
    from unittest import TestCase
    
    class BuggyTestCase(TestCase):
    
        def _callTestMethod(self, method):
            import inspect
    
            if inspect.isgeneratorfunction(method):
                list(method())
            else:
                method()
    
        def test_generator(self):
            self.assertTrue(False)
            yield None
    
    if __name__ == "__main__":
        unittest.main()

    python3.8 test_foo.py
    F
    ======================================================================
    FAIL: test_generator (main.BuggyTestCase)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "test_foo.py", line 10, in _callTestMethod
        list(method())
      File "test_foo.py", line 15, in test_generator
        self.assertTrue(False)
    AssertionError: False is not true

    Ran 1 test in 0.000s

    FAILED (failures=1)

    @tirkarthi
    Copy link
    Member

    This is a duplicate of bpo-15551 and as per discussion in the thread my approach is incoherent with design of unittest to execute generators. I propose closing this as duplicate with same resolution as not fixed unless there is a change required here.

    @serhiy-storchaka
    Copy link
    Member

    I would raser raise error if the test method returns something suspicious, like generator or coroutine. Or maybe if it returns anything except None.

    @defreng
    Copy link
    Mannequin Author

    defreng mannequin commented Jul 17, 2020

    I would also strongly vote for raising an error if the test method is a generator - not even silently turn it into a list.

    It would be straight forward to implement various checks (like the ones you mentioned for generators, coroutines, ...) - and they will absolutely help to prevent unintended errors, especially by more junior developers.

    @zware
    Copy link
    Member

    zware commented Jul 17, 2020

    In the same vein as Serhiy's suggestion, I would like to see unittest raise a DeprecationWarning when a test method returns anything other than None, and eventually switch that to an error (probably TypeError or ValueError).

    @terryjreedy
    Copy link
    Member

    The current behavior is not a bug as it follows documented behavior. In the absence of any use for returning values from test functions, I agree that we should do the simple thing and raise ValueError (I checked and did not find anything like a UnittestError.) That would add minimal time to testing.

    @terryjreedy terryjreedy added 3.10 only security fixes and removed 3.8 only security fixes labels Jul 18, 2020
    @terryjreedy terryjreedy changed the title unittest: Generator test methods will always be marked as passed unittest: deprecate test methods returning non-None values Jul 18, 2020
    @terryjreedy terryjreedy added type-feature A feature request or enhancement 3.10 only security fixes and removed type-bug An unexpected behavior, bug, or error 3.8 only security fixes labels Jul 18, 2020
    @terryjreedy terryjreedy changed the title unittest: Generator test methods will always be marked as passed unittest: deprecate test methods returning non-None values Jul 18, 2020
    @terryjreedy terryjreedy added type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Jul 18, 2020
    @serhiy-storchaka
    Copy link
    Member

    It is also a good idea for linters to catch such kind of errors. It will help users of older Python versions.

    We cannot raise error without deprecation period or add warnings in old versions because it potentially can break existing code, e.g.:

        def test_ham(self, arg='ham'):
            ...
    
        def test_spam(self):
            res = test_ham('spam')
            self.assertTrue(res)

    @ambv
    Copy link
    Contributor

    ambv commented Aug 19, 2021

    New changeset 3db42fc by andrei kulakov in branch 'main':
    bpo-41322: added deprecation warning for tests returning value!=None (GH-27748)
    3db42fc

    @ambv
    Copy link
    Contributor

    ambv commented Aug 19, 2021

    Thanks for reporting, Alexander, and Andrei for the patch! ✨ 🍰 ✨

    @ambv ambv closed this as completed Aug 19, 2021
    @ambv ambv closed this as completed Aug 19, 2021
    @ambv ambv added 3.11 only security fixes and removed 3.10 only security fixes labels Aug 19, 2021
    @softsolsolutions softsolsolutions mannequin added 3.8 only security fixes 3.7 (EOL) end of life and removed 3.11 only security fixes 3.8 only security fixes labels Aug 19, 2021
    @terryjreedy terryjreedy added 3.11 only security fixes and removed 3.7 (EOL) end of life labels Aug 19, 2021
    @akulakov
    Copy link
    Contributor

    I will add the test in the next 1-2 days, I should have asked if it's needed in this case..

    @serhiy-storchaka
    Copy link
    Member

    Every new feature should be documented and covered by tests (unless it is very trivial or it is very difficult to write a test for it).

    @nightlark
    Copy link
    Mannequin

    nightlark mannequin commented Aug 21, 2021

    It looks like since #71935 got merged, test_subprocess_wait_no_same_group in test_asyncio has been failing for the Ubuntu SSL tests.

    @akulakov
    Copy link
    Contributor

    That test does have a yield statement in it, and does cause the deprecation warning to fire off. I'm looking into it..

    @nightlark
    Copy link
    Mannequin

    nightlark mannequin commented Aug 21, 2021

    Would rewriting the test so that it doesn't yield be the right fix?

        def test_subprocess_wait_no_same_group(self):
            # start the new process in a new session
            connect = self.loop.subprocess_shell(
                            functools.partial(MySubprocessProtocol, self.loop),
                            'exit 7', stdin=None, stdout=None, stderr=None,
                            start_new_session=True)
            _, proto = yield self.loop.run_until_complete(connect)
            self.assertIsInstance(proto, MySubprocessProtocol)
            self.loop.run_until_complete(proto.completed)
            self.assertEqual(7, proto.returncode)
    

    to

    def test_subprocess_wait_no_same_group(self):
            # start the new process in a new session
            connect = self.loop.subprocess_shell(
                            functools.partial(MySubprocessProtocol, self.loop),
                            'exit 7', stdin=None, stdout=None, stderr=None,
                            start_new_session=True)
            transp, proto = self.loop.run_until_complete(connect)
            self.assertIsInstance(proto, MySubprocessProtocol)
            self.loop.run_until_complete(proto.completed)
            self.assertEqual(7, proto.returncode)
            transp.close()
    

    @akulakov
    Copy link
    Contributor

    Yes, it seems like this deprecation detected a broken test that wasn't running.

    If I add a raise statement to it and rerun the test, it still passes on my local system:
    ./python.exe -m unittest -v test.test_asyncio.test_events.PollEventLoopTests.test_subprocess_wait_no_same_group
    warnings.warn(f'It is deprecated to return a value!=None from a '
    Ran 1 test in 0.022s

    OK

    (with last line of test being just raise).

    It seems to me it should be fixed in some way although I don't know if Ryan's suggestion will do or not as I haven't looked at asyncio tests before..

    @akulakov
    Copy link
    Contributor

    The test was refactored in this commit: 658103f

    the refactoring moved yield statement from inner func into the test.

    That was 8 years ago so it hasn't worked since then, and since asyncio changed a lot, I wonder if the test is still valid?

    Ryan, do you want to open a new issue for fixing / removing the test?

    @nightlark
    Copy link
    Mannequin

    nightlark mannequin commented Aug 21, 2021

    Sure, I'll open a new issue.

    @nightlark
    Copy link
    Mannequin

    nightlark mannequin commented Aug 21, 2021

    The new issue for the failing test is bpo-44968

    @tirkarthi
    Copy link
    Member

    There are few deprecation warnings in other tests :

    0:00:05 load avg: 3.20 [ 52/428] test_code passed
    /home/karthikeyan/stuff/python/cpython/Lib/unittest/case.py:550: DeprecationWarning: It is deprecated to return a value!=None from a test case (<bound method CodeTest.test_constructor of <test.test_code.CodeTest testMethod=test_constructor>>)
    warnings.warn(f'It is deprecated to return a value!=None from a '

    0:00:12 load avg: 3.74 [110/428] test_capi passed
    /home/karthikeyan/stuff/python/cpython/Lib/unittest/case.py:550: DeprecationWarning: It is deprecated to return a value!=None from a test case (<built-in function test_null_strings>)
    warnings.warn(f'It is deprecated to return a value!=None from a '

    0:00:13 load avg: 3.74 [114/428] test_distutils passed
    /home/karthikeyan/stuff/python/cpython/Lib/distutils/tests/test_unixccompiler.py:7: DeprecationWarning: The distutils.sysconfig module is deprecated, use sysconfig instead
    from distutils import sysconfig
    /home/karthikeyan/stuff/python/cpython/Lib/unittest/case.py:550: DeprecationWarning: It is deprecated to return a value!=None from a test case (<bound method _id of <distutils.tests.test_bdist_rpm.BuildRpmTestCase testMethod=test_no_optimize_flag>>)
    warnings.warn(f'It is deprecated to return a value!=None from a '
    /home/karthikeyan/stuff/python/cpython/Lib/unittest/case.py:550: DeprecationWarning: It is deprecated to return a value!=None from a test case (<bound method _id of <distutils.tests.test_bdist_rpm.BuildRpmTestCase testMethod=test_quiet>>)
    warnings.warn(f'It is deprecated to return a value!=None from a '

    @akulakov
    Copy link
    Contributor

    Karthikeyan: I was looking into them yesterday, and:

    but I haven't worked with C tests before and it looks like some other C api tests also return non-null and not None values, but they didn't trigger the deprecation when running a test PR I made yesterday here:

    https://github.com/python/cpython/pull/27869/checks?check_run_id=3387700689

    @akulakov
    Copy link
    Contributor

    I should just add that none of the these tests are actually a problem, i.e. they all do run properly and not running duplicate times (except that I'm not sure about the 2 distutils tests, but that shouldn't matter much as distutils is deprecated and up for removal).

    @tirkarthi
    Copy link
    Member

    Deprecation warnings are not on by default in tests. You can pass -Wall while running tests to turn on all warnings.

    ./python -Wall -m test test_capi
    0:00:00 load avg: 2.46 Run tests sequentially
    0:00:00 load avg: 2.46 [1/1] test_capi
    /home/karthikeyan/stuff/python/cpython/Lib/unittest/case.py:550: DeprecationWarning: It is deprecated to return a value!=None from a test case (<built-in function test_null_strings>)
    warnings.warn(f'It is deprecated to return a value!=None from a '

    == Tests result: SUCCESS ==

    1 test OK.

    Total duration: 5.2 sec
    Tests result: SUCCESS

    @serhiy-storchaka
    Copy link
    Member

    New changeset b1db308 by andrei kulakov in branch 'main':
    bpo-41322: Add unit tests for deprecation of test return values (GH-27846)
    b1db308

    @ambv
    Copy link
    Contributor

    ambv commented Aug 22, 2021

    This can be re-closed. Deprecation warnings raised by the test suite are being fixed in BPO-44980 which already has an open pull request.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants