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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2021-08-19 09:42 |
Thanks for reporting, Alexander, and Andrei for the patch! ✨ 🍰 ✨
|
msg399915 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:33 | admin | set | github: 85494 |
2021-08-22 20:43:31 | lukasz.langa | set | status: open -> closed resolution: fixed messages:
+ msg400105
stage: patch review -> resolved |
2021-08-22 18:32:54 | serhiy.storchaka | set | messages:
+ msg400087 |
2021-08-21 14:49:54 | xtreak | set | messages:
+ msg400031 |
2021-08-21 14:42:29 | andrei.avk | set | messages:
+ msg400030 |
2021-08-21 14:36:24 | andrei.avk | set | messages:
+ msg400029 |
2021-08-21 14:08:31 | xtreak | set | messages:
+ msg400028 |
2021-08-21 04:28:38 | rmast | set | messages:
+ msg400019 |
2021-08-21 04:09:29 | rmast | set | messages:
+ msg400017 |
2021-08-21 04:05:39 | andrei.avk | set | messages:
+ msg400016 |
2021-08-21 03:54:33 | andrei.avk | set | messages:
+ msg400015 |
2021-08-21 03:49:36 | rmast | set | messages:
+ msg400014 |
2021-08-21 03:45:27 | andrei.avk | set | messages:
+ msg400013 |
2021-08-21 03:13:09 | rmast | set | nosy:
+ rmast messages:
+ msg400010
|
2021-08-20 06:55:02 | serhiy.storchaka | set | messages:
+ msg399938 |
2021-08-20 01:12:59 | andrei.avk | set | stage: test needed -> patch review pull_requests:
+ pull_request26308 |
2021-08-19 17:43:12 | andrei.avk | set | messages:
+ msg399927 |
2021-08-19 17:02:34 | serhiy.storchaka | set | nosy:
+ terry.reedy, rbcollins, ezio.melotti, michael.foord, zach.ware, xtreak, defreng, andrei.avk
|
2021-08-19 16:54:56 | terry.reedy | set | nosy:
+ lukasz.langa, serhiy.storchaka, - fdrake, terry.reedy, stefan
versions:
+ Python 3.11, - Python 3.7 |
2021-08-19 16:24:58 | santhu_reddy12 | set | nosy:
+ fdrake, terry.reedy, stefan
versions:
+ Python 3.7, - Python 3.8 |
2021-08-19 16:24:05 | santhu_reddy12 | set | nosy:
+ 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:30 | serhiy.storchaka | set | status: closed -> open resolution: fixed -> (no value) messages:
+ msg399915
stage: resolved -> test needed |
2021-08-19 09:42:12 | lukasz.langa | set | versions:
+ Python 3.11, - Python 3.10 |
2021-08-19 09:42:07 | lukasz.langa | set | status: open -> closed resolution: fixed messages:
+ msg399902
stage: patch review -> resolved |
2021-08-19 09:41:12 | lukasz.langa | set | nosy:
+ lukasz.langa messages:
+ msg399901
|
2021-08-12 20:22:40 | andrei.avk | set | keywords:
+ patch nosy:
+ andrei.avk
pull_requests:
+ pull_request26226 stage: patch review |
2020-07-18 06:40:36 | serhiy.storchaka | set | messages:
+ msg373876 |
2020-07-18 06:29:19 | terry.reedy | set | nosy:
+ 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:35 | zach.ware | set | nosy:
+ zach.ware messages:
+ msg373850
|
2020-07-17 11:20:58 | defreng | set | messages:
+ msg373824 |
2020-07-17 10:54:03 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg373816
|
2020-07-17 10:43:52 | xtreak | set | messages:
+ msg373812 |
2020-07-17 10:17:58 | xtreak | set | nosy:
+ ezio.melotti, xtreak, michael.foord, rbcollins messages:
+ msg373810
|
2020-07-17 09:46:57 | defreng | create | |