BUG Fix tests in test_io #248
Conversation
The `test_civis_to_file_local` function had been writing a file to disk which remained behind after the test. Additionally, `called_once_with` is not a method of a `Mock` object; it becomes its own `Mock`, which takes any paramter given and always evaluates to `True`. Change all instances of those calls to `assert_called_once_with`. Add `autospec` to `patch` calls where possible to test that mocked functions are being called with valid arguments.
stephen-hoover
force-pushed
the
fix-io-test
branch
from
259f724 to
8659f04
Compare
|
@waltaskew , the failing test is unrelated, and will be fixed once #249 merges. I'll merge that into this branch before merging this PR. |
|
|
||
|
|
||
| @mock.patch('civis.io._files._civis_to_file') | ||
| @mock.patch('%s.open' % __name__, create=True) |
There was a problem hiding this comment.
Is this the test that was writing to disk? How was it able to do so when we were mocking the open function?
There was a problem hiding this comment.
I think we were mocking open in the wrong place. The mock actually wasn't being called at all. The error in the asserts in this test prevented us from noticing. I'm not certain what the right place to mock would be, but it looks like open was being mocked in the testing module, not in the _io._files module.
civis/tests/test_io.py
Outdated
| with TemporaryDirectory() as tdir: | ||
| fname = os.path.join(tdir, "foo") | ||
| assert not os.path.exists(fname) | ||
| civis.io.civis_to_file(int(test_obj), fname) |
There was a problem hiding this comment.
It looks like this is testing much the same behavior as the previous incarnation of the test, which at its root looks to be that if you call civis_to_file(123, 'path') the 'path' string has open called on it and that buffer from the open call is passed to _civis_to_file. What was the issue with using mock objects rather than real file objects?
I think we could test all of these things and more in one go if we mocked the requests call happening in _civis_to_file.
There was a problem hiding this comment.
Yes, my aim was to fix the tests as they were, rather than going too far in testing new things. The primary issue with the mocks was that open calls weren't actually being mocked. The tests were still calling the real open function, but they were written in such a way that we couldn't tell.
I rewrote them to drop the open mock because that seemed less prone to bugs of the sort which originally existed in this test and because I think it's a bit less tied to implementation details. I'm reluctant to go so far as to mock requests (and we'd have to also mock the Civis API client) because that feels brittle to me. Maybe there's a way to write that mocking robustly?
There was a problem hiding this comment.
I don't know that we're further from testing implementation details here.
In the previous test, we were trying to assert that the string passed to civis_to_file was opened and passed to _civis_to_file using mock objects. I think the issue was the use of the non-existent assert mock.called_once_with obscured the incorrect mocking of open.
In this new test, we're testing the same thing except by passing a real file path and replacing _civis_to_file with a function that writes to its passed buffer. It looks like we're still testing the same thing (did we pass the results of open(path) to _civis_to_file) except now we're doing so by checking file system state to see if our mocked _civis_to_file was called correctly rather than using mock object calls.
My 'mock requests' suggestion was something like
@mock.path.object(_files, 'requests')
@mock.path.object(_files, 'civis')
def test_civis_to_file(m_civis, m_requests):
m_requests.get().iter_content.return_value = [b'abc', b'def']
with tempfile.NamedTemporaryFile() as file:
_files.civis_to_file(123, file.name)
assert file.read() == b'abcdef'
m_civis.APIClient().files.get.assert_called_once_with(123)
m_requests.get.assert_called_once_with(m_civis.APIClient().files.get().file_url, stream=True)If we wanted to test something a bit beyond 'did we pass open(path) to _civis_to_file?'
civis/tests/test_io.py
Outdated
| "foo", "foo_name", mock_file) | ||
| def mock_read(buf, name, **kwargs): | ||
| return buf.read() | ||
| mock_file_to_civis_helper.side_effect = mock_read |
There was a problem hiding this comment.
What was the issue with the previous incarnation of this test mocking open?
There was a problem hiding this comment.
The mock wasn't being called. Rather than trying to figure out where to mock, it seemed simpler and more robust to implementation changes if I used real files.
|
@waltaskew , I agree that your suggested test looks better. I rewrote the two file transfer tests. See if you think this is better. I used a temporary directory instead of a |
|
LGTM |
|
@waltaskew , thanks for the review and the testing advice! Could you also press the "approved" button? |
2 participants
The
test_civis_to_file_localfunction had been writing a file to disk which remained behind after the test. Additionally,called_once_withis not a method of aMockobject; it becomes its ownMock, which takes any paramter given and always evaluates toTrue. Change all instances of those calls toassert_called_once_with. Addautospectopatchcalls where possible to test that mocked functions are being called with valid arguments.