unittest: deprecate test methods returning non-None values

[defreng]

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]

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?

[tirkarthi]

cpython/Lib/unittest/case.py

Line 548 in 8e836bb

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]

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]

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]

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]

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]

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.

[serhiy-storchaka]

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]

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

[ambv]

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

[akulakov]

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

[serhiy-storchaka]

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]

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]

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

[nightlark]

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]

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]

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]

Sure, I'll open a new issue.

[nightlark]

The new issue for the failing test is bpo-44968

[tirkarthi]

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]

Karthikeyan: I was looking into them yesterday, and:

• distutils ones can probably be left as is because distutils are set for removal in 3.12

• test_constructor should be fixed, I can do that today.

• test_null_strings in test_capi I'm not sure, I think it can also be fixed here:
  https://github.com/python/cpython/blob/main/Modules/_testcapimodule.c#L2388

  .. by replacing return line with Py_RETURN_NONE;

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]

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]

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]

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

[ambv]

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.