This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: unittest: deprecate test methods returning non-None values
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.11
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: andrei.avk, defreng, ezio.melotti, lukasz.langa, michael.foord, rbcollins, rmast, santhu_reddy12, serhiy.storchaka, terry.reedy, xtreak, zach.ware
Priority: normal Keywords: patch

Created on 2020-07-17 09:46 by defreng, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 27748 merged andrei.avk, 2021-08-12 20:22
PR 27846 merged andrei.avk, 2021-08-20 01:12
Messages (27)
msg373807 - (view) Author: Alexander Hungenberg (defreng) Date: 2020-07-17 09:46
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?
msg373810 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-07-17 10:17
https://github.com/python/cpython/blob/8e836bb21ce73f0794fd769db5883c29680dfe47/Lib/unittest/case.py#L548 . _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)
msg373812 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2020-07-17 10:43
This is a duplicate of issue15551 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.
msg373816 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-07-17 10:54
I would raser raise error if the test method returns something suspicious, like generator or coroutine. Or maybe if it returns anything except None.
msg373824 - (view) Author: Alexander Hungenberg (defreng) Date: 2020-07-17 11:20
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.
msg373850 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2020-07-17 19:23
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).
msg373874 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2020-07-18 06:29
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.
msg373876 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-07-18 06:40
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)
msg399901 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-08-19 09:41
New changeset 3db42fc5aca320b0cac1895bc3cb731235ede794 by andrei kulakov in branch 'main':
bpo-41322: added deprecation warning for tests returning value!=None (GH-27748)
https://github.com/python/cpython/commit/3db42fc5aca320b0cac1895bc3cb731235ede794
msg399902 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-08-19 09:42
Thanks for reporting, Alexander, and Andrei for the patch! ✨ 🍰 ✨
msg399915 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-08-19 13:04
Tests are needed.
msg399923 - (view) Author: santhosh (santhu_reddy12) Date: 2021-08-19 16:24
def split_module_names(module):
    unnamed, named = set(), set()
    for name in dir(module):
        if not name.startswith('_'):
            attr = getattr(module, name)
            try:
                if hasattr(attr, '__name__'):
                    named.add(name)
                else:
                    unnamed.add(name)
            except TypeError:
                pass
    return named, unnamed
msg399927 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-08-19 17:43
I will add the test in the next 1-2 days, I should have asked if it's needed in this case..
msg399938 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-08-20 06:55
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).
msg400010 - (view) Author: Ryan Mast (nightlark) (rmast) * Date: 2021-08-21 03:13
It looks like since GH-27748 got merged, `test_subprocess_wait_no_same_group` in `test_asyncio` has been failing for the Ubuntu SSL tests.
msg400013 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-08-21 03:45
That test does have a yield statement in it, and does cause the deprecation warning to fire off. I'm looking into it..
msg400014 - (view) Author: Ryan Mast (nightlark) (rmast) * Date: 2021-08-21 03:49
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()
```
msg400015 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-08-21 03:54
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..
msg400016 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-08-21 04:05
The test was refactored in this commit: https://github.com/python/cpython/commit/658103f84ea860888f8dab9615281ea64fee31b9

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?
msg400017 - (view) Author: Ryan Mast (nightlark) (rmast) * Date: 2021-08-21 04:09
Sure, I'll open a new issue.
msg400019 - (view) Author: Ryan Mast (nightlark) (rmast) * Date: 2021-08-21 04:28
The new issue for the failing test is bpo-44968
msg400028 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2021-08-21 14:08
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 '
msg400029 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-08-21 14:36
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
msg400030 - (view) Author: Andrei Kulakov (andrei.avk) * (Python triager) Date: 2021-08-21 14:42
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).
msg400031 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python committer) Date: 2021-08-21 14:49
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
msg400087 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-08-22 18:32
New changeset b1db308c6114bb18c06656c94c0ffa73ba954977 by andrei kulakov in branch 'main':
bpo-41322: Add unit tests for deprecation of test return values (GH-27846)
https://github.com/python/cpython/commit/b1db308c6114bb18c06656c94c0ffa73ba954977
msg400105 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2021-08-22 20:43
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.
History
Date User Action Args
2022-04-11 14:59:33adminsetgithub: 85494
2021-08-22 20:43:31lukasz.langasetstatus: open -> closed
resolution: fixed
messages: + msg400105

stage: patch review -> resolved
2021-08-22 18:32:54serhiy.storchakasetmessages: + msg400087
2021-08-21 14:49:54xtreaksetmessages: + msg400031
2021-08-21 14:42:29andrei.avksetmessages: + msg400030
2021-08-21 14:36:24andrei.avksetmessages: + msg400029
2021-08-21 14:08:31xtreaksetmessages: + msg400028
2021-08-21 04:28:38rmastsetmessages: + msg400019
2021-08-21 04:09:29rmastsetmessages: + msg400017
2021-08-21 04:05:39andrei.avksetmessages: + msg400016
2021-08-21 03:54:33andrei.avksetmessages: + msg400015
2021-08-21 03:49:36rmastsetmessages: + msg400014
2021-08-21 03:45:27andrei.avksetmessages: + msg400013
2021-08-21 03:13:09rmastsetnosy: + rmast
messages: + msg400010
2021-08-20 06:55:02serhiy.storchakasetmessages: + msg399938
2021-08-20 01:12:59andrei.avksetstage: test needed -> patch review
pull_requests: + pull_request26308
2021-08-19 17:43:12andrei.avksetmessages: + msg399927
2021-08-19 17:02:34serhiy.storchakasetnosy: + terry.reedy, rbcollins, ezio.melotti, michael.foord, zach.ware, xtreak, defreng, andrei.avk
2021-08-19 16:54:56terry.reedysetnosy: + lukasz.langa, serhiy.storchaka, - fdrake, terry.reedy, stefan

versions: + Python 3.11, - Python 3.7
2021-08-19 16:24:58santhu_reddy12setnosy: + fdrake, terry.reedy, stefan

versions: + Python 3.7, - Python 3.8
2021-08-19 16:24:05santhu_reddy12setnosy: + santhu_reddy12, - terry.reedy, rbcollins, ezio.melotti, michael.foord, lukasz.langa, zach.ware, serhiy.storchaka, xtreak, defreng, andrei.avk

messages: + msg399923
versions: + Python 3.8, - Python 3.11
2021-08-19 13:04:30serhiy.storchakasetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg399915

stage: resolved -> test needed
2021-08-19 09:42:12lukasz.langasetversions: + Python 3.11, - Python 3.10
2021-08-19 09:42:07lukasz.langasetstatus: open -> closed
resolution: fixed
messages: + msg399902

stage: patch review -> resolved
2021-08-19 09:41:12lukasz.langasetnosy: + lukasz.langa
messages: + msg399901
2021-08-12 20:22:40andrei.avksetkeywords: + patch
nosy: + andrei.avk

pull_requests: + pull_request26226
stage: patch review
2020-07-18 06:40:36serhiy.storchakasetmessages: + msg373876
2020-07-18 06:29:19terry.reedysetnosy: + terry.reedy
title: unittest: Generator test methods will always be marked as passed -> unittest: deprecate test methods returning non-None values
messages: + msg373874

versions: + Python 3.10, - Python 3.8
type: behavior -> enhancement
2020-07-17 19:23:35zach.waresetnosy: + zach.ware
messages: + msg373850
2020-07-17 11:20:58defrengsetmessages: + msg373824
2020-07-17 10:54:03serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg373816
2020-07-17 10:43:52xtreaksetmessages: + msg373812
2020-07-17 10:17:58xtreaksetnosy: + ezio.melotti, xtreak, michael.foord, rbcollins
messages: + msg373810
2020-07-17 09:46:57defrengcreate