bpo-31285: fix an assertion failure and a SystemError in warnings.warn_explicit#3219
Conversation
|
|
||
| /* Split the source into lines. */ | ||
| source_list = PyObject_CallMethodObjArgs(source, | ||
| source_list = PyObject_CallMethodObjArgs((PyObject *)&PyUnicode_Type, |
There was a problem hiding this comment.
Use just PyUnicode_Splitlines().
| return splitlines_ret_val | ||
| return BadSource() | ||
| return BadLoader() | ||
| self.assertRaises(IndexError, self.module.warn_explicit, |
There was a problem hiding this comment.
Since this test already takes multiple lines, I think it would look clearer if use a context manager form:
with self.assertRaises(IndexError):
self.module.warn_explicit(...)
But why IndexError is expected? Wouldn't be better to test the normal case that doesn't raise an error? You can use test.support.captured_stderr() for capturing the warning output.
| self.assertNotIn(b'Warning!', stderr) | ||
| self.assertNotIn(b'Error', stderr) | ||
|
|
||
| def test_warn_explicit(self): |
There was a problem hiding this comment.
This name is too general, but the test isn't a general test of warn_explicit(). Use something more specific like test_issue31285.
| 'foo', UserWarning, 'bar', 1, | ||
| module_globals={'__loader__': _get_bad_loader(42), | ||
| '__name__': 'foobar'}) | ||
| self.assertIn('bar:1: UserWarning: foo', stderr.getvalue()) |
There was a problem hiding this comment.
I am not sure whether this and the other call to assertIn() are necessary, as we want to make sure a SystemError and an assertion failure don't happen...
| # warn_explicit() should neither raise a SystemError nor cause an | ||
| # assertion failure, in case the return value of get_source() has a | ||
| # bad splitlines() method. | ||
| def _get_bad_loader(splitlines_ret_val): |
There was a problem hiding this comment.
Starting underscore is not needed.
| del self.module._showwarnmsg | ||
| with support.captured_stderr() as stderr: | ||
| self.module.warn_explicit( | ||
| 'foo', ArithmeticError, 'bar', 1, |
There was a problem hiding this comment.
Is it valid to use ArithmeticError which is not a warning category?
There was a problem hiding this comment.
It is valid (IIUC, only warning.warns() requires the category to be a subclass of Warning). I used ArithmeticError because it caused get_filter() to return 'default'. But now it seems quite obscure to me, so I would change it.
Also, my test fails with -Werror, so I would fix that too.
|
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
|
Sorry, @orenmn and @serhiy-storchaka, I could not cleanly backport this to |
|
Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6. |
|
Sorry @orenmn and @serhiy-storchaka, I had trouble checking out the |
|
Sorry, @orenmn and @serhiy-storchaka, I could not cleanly backport this to |
…gs.warn_explicit. (pythonGH-3219) (cherry picked from commit 91fb0af)
|
GH-3775 is a backport of this pull request to the 3.6 branch. |
|
GH-3805 is a backport of this pull request to the 2.7 branch. |
…pythonGH-3803) (forgot to remove it in pythonGH-3219) (cherry picked from commit 8b4ff53)
…) in case __loader__.get_source() has a bad splitlines() method. (pythonGH-3219)
_warnings.c: add checks whether the return value of the received loader's get_source() is invalid.test_warnings/__init__.py: add tests to verify the assertion failure and the SystemError are no more.https://bugs.python.org/issue31285