MAINT: pytest cleanups #10294
MAINT: pytest cleanups #10294
Conversation
| @@ -94,7 +94,7 @@ def test_overlapping_assignments(): | |||
| srcidx = tuple([a[0] for a in ind]) | |||
| dstidx = tuple([a[1] for a in ind]) | |||
|
|
|||
| yield _check_assignment, srcidx, dstidx | |||
There was a problem hiding this comment.
Can we use the parameterize decorator here instead?
There was a problem hiding this comment.
Tried that; doesn't work with nose. I can drop the second commit if you want, but a similar PR was merged previously.
There was a problem hiding this comment.
Good point, I guess this is fine. Deprecating this style of test feels like a step backwards to me, but that's beyond our control.
There was a problem hiding this comment.
I added limited support for parameters on nose in nose_tools just for some of these cases. It doesn't have the full capabilities of the pytest version, however.
There was a problem hiding this comment.
Correct. parametrize doesn't even replace yield because it must be static on collection. I looked at other examples and the same thing was done as here. Complain to the pytest folks if you want.
|
Just for the sake of linking, second commit is fallout from pytest-dev/pytest#1714 |
numpy/testing/pytest_tools/utils.py
Outdated
| raise ValueError('Function did not raise an exception') | ||
|
|
||
| return assert_raises_context() | ||
| return pytest.raises(exception_class) |
There was a problem hiding this comment.
Look at what it's replacing: no kwargs are present. The documentation specifies that there should be no kwargs when fn is none.
There was a problem hiding this comment.
Grep https://docs.pytest.org/en/latest/builtin.html for "context manager"
There was a problem hiding this comment.
Right, the documentation specifies that they must not be passed, but the implementation just silently drops them if they are. Forwarding them would probably cause pytest to throw an appropriate error, although an elif kwargs: raise TypeError would do that job too.
There was a problem hiding this comment.
Maybe I could simply remove the branch, though I seem to recall that previously failed.
numpy/lib/tests/test_format.py
Outdated
|
|
||
|
|
||
| def test_read_version_1_0_bad_magic(): | ||
| for magic in bad_version_magic + malformed_magic: | ||
| f = BytesIO(magic) | ||
| yield raises(ValueError)(format.read_array), f | ||
| raises(ValueError)(format.read_array)(f) |
There was a problem hiding this comment.
No value in using the raises decorator here now that it's not a yield test - just use assert_raises
numpy/lib/tests/test_format.py
Outdated
| @@ -728,13 +728,15 @@ def test_read_magic(): | |||
| def test_read_magic_bad_magic(): | |||
| for magic in malformed_magic: | |||
| f = BytesIO(magic) | |||
| yield raises(ValueError)(format.read_magic), f | |||
| with assert_raises(ValueError): | |||
| format.read_array(f) | |||
There was a problem hiding this comment.
Convention seems to be to use the shorter assert_raises(ValueError, format.read_array, f) in almost all our test cases
|
Thanks @xoviat! |
No description provided.