bpo-41322: add two unit tests for deprecation of test return values #27846
Conversation
|
While we are here, please document this change in the unittest module documentation and add an entry in the What's New document. |
|
I would also add with self.assertWarns(DeprecationWarning) as w:
Foo('test_func').run()
self.assertIn('It is deprecated to return a value!=None', w.warnings[0].message)
self.assertIn('test_func', w.warnings[0].message)
self.assertEqual(w.warnings[0].filename, __file__) |
My reasoning for not adding the change to the docs was that the pattern of use of test methods and the way they are described in the docs don't leave any reason for returning a value from a test method, so it seems like the only case when that would happen would be a mistake (possibly to be used by 3rd party code, but that seems unlikely). But I don't see any harm in adding it to docs, see below. I'm curious if there are scenarios where users might want to return values from test method and this change to the docs would dissuade them. I'm not sure if the docs should try to explain the reason for the change, it seems like a short explanation would be puzzling and a good explanation would be too long. Proposed text: |
I got it now, please ignore.. |
Lib/unittest/test/test_async_case.py
Outdated
| self.assertIn('It is deprecated to return a value!=None', w.warnings[0].message.args[0]) | ||
| self.assertIn('test', w.warnings[0].message.args[0]) | ||
| self.assertIn(w.warnings[0].filename, __file__) |
There was a problem hiding this comment.
It is better to move this outside of the with block.
| self.assertIn('It is deprecated to return a value!=None', w.warnings[0].message.args[0]) | |
| self.assertIn('test', w.warnings[0].message.args[0]) | |
| self.assertIn(w.warnings[0].filename, __file__) | |
| self.assertIn('It is deprecated to return a value!=None', w.warnings[0].message.args[0]) | |
| self.assertIn('test', w.warnings[0].message.args[0]) | |
| self.assertIn(w.warnings[0].filename, __file__) |
There was a problem hiding this comment.
Not assertIn, but assertEqual.
Lib/unittest/test/test_async_case.py
Outdated
| with self.assertWarns(DeprecationWarning) as w: | ||
| Test('test').run() | ||
| self.assertIn('It is deprecated to return a value!=None', w.warnings[0].message.args[0]) | ||
| self.assertIn('test', w.warnings[0].message.args[0]) |
There was a problem hiding this comment.
'test' is too general name. This substring is a part of the warning message ("... from a test case ..."), so the test is passed in any case. Use more specific names, like test1, test2, test_func, test_gen.
Also, would not be better to test str(w.warnings[0].message) instead of w.warnings[0].message.args[0]?
Lib/unittest/test/test_async_case.py
Outdated
| Test('test1').run() | ||
| self.assertIn('It is deprecated to return a value!=None', str(w.warnings[0].message)) | ||
| self.assertIn('test1', str(w.warnings[0].message)) | ||
| self.assertEquals(w.warnings[0].filename, __file__) |
There was a problem hiding this comment.
assertEquals is a deprecated alias. Use assertEqual.
There was a problem hiding this comment.
Ah, somehow missed the deprecation warnings in a PR for deprecation warnings :) thanks for catching that.
|
@serhiy-storchaka thanks for reviews! :) |
tests for #27748
https://bugs.python.org/issue41322