Added tests for each branch in execute's run_cell method #982
Conversation
There was a problem hiding this comment.
There are a few places I've made specific suggestions or asked questions that you can address locally to those suggestions/questions.
But the biggest change happening here is the introduction and heavy use of the python mock MagicMock functionality. This is not something we've been doing up to this point, meaning that we need to do extra work to make sure people understand what's happening in these tests. Otherwise, you might be the only one who is able to maintain these tests.
I think the minimal thing would more docstrings for the methods you are introducing that explain (in addition to the usual things parameters, etc.):
- how they are to be used
- what kinds of objects they're manipulating under the hood
- what is expected behaviour
Then I think it's worth describing what the architecture is trying to accomplish.
Note on style that may be more meaningful than I would have thought: in general we prefer to use simple asserts rather than self.assert*()s. This makes tests more easily readable (as they do not look functionally different than regular code just using there APIs.
By having tests that look substantially identical to the code that they are running they act as examples of how to use the APIs. As this is currently written, I don't think anyone would be able to work from this to understand how to use these APIs.
Could you try to use any of the idioms from pytest (e.g., fixtures) to make this code better fit the style of the rest of the tests codebase?
| @@ -40,7 +42,64 @@ def _normalize_base64(b64_text): | |||
| except (ValueError, TypeError): | |||
| return b64_text | |||
|
|
|||
| class TestExecute(PreprocessorTestsBase): | |||
|
|
|||
| def merge_dicts(first, second): | |||
There was a problem hiding this comment.
You could make this a *args and then change the code below to be
outcome = {}
for d in args:
outcome.update(d)
return outcome
There was a problem hiding this comment.
Also since this is a general pattern you might want to put it in nbconvert/preprocessors/tests/base.py and import it here.
| preprocessor.enabled = True | ||
| for opt in opts: | ||
| setattr(preprocessor, opt, opts[opt]) | ||
| # Perform some state setup that should probably be in the init |
There was a problem hiding this comment.
Is this a suggestion for a change that should be made in the ExecutePreprocessor __init__ or a test specific init?
|
|
||
| from .base import PreprocessorTestsBase | ||
| from ..execute import ExecutePreprocessor, CellExecutionError, executenb | ||
|
|
||
| import IPython | ||
| from mock import MagicMock |
There was a problem hiding this comment.
in Python 3 isn't it from unittest.mock import MagicMock?
| parent_id = 'fake_id' | ||
| cell_mock = MagicMock(source='"foo" = "bar"', outputs=[]) | ||
| # Hack to help catch `.` and `[]` style access to outputs against the same mock object | ||
| cell_mock.__getitem__.side_effect = lambda n: cell_mock.outputs if n == 'outputs' else None |
There was a problem hiding this comment.
is this to get IPython IPython.utils.ipstruct.Struct like functionality? Is this needed because the MagicMock isn't able to inherit from that? Could we create a MagicMock subclass that just builds this functionality in for any of its attributes if we're going to be treating it like a struct?
| return preprocessor | ||
|
|
||
| @staticmethod | ||
| def prepare_cell_mocks(*messages): |
There was a problem hiding this comment.
I think this needs a good deal of explanation at a high level before this will be easy to follow. I don't think we've been using mock much up to this point and this makes all of these heavily dependent on understanding how that works. I'm going to stop here, not because I don't have reviews to give for code further down but because this is going to take a few times going over with greater understanding on each pass given the new approach this is introducing.
|
This pull request has been mentioned on Jupyter Community Forum. There might be relevant details there: https://discourse.jupyter.org/t/request-for-help-for-a-new-nbconvert-release/811/1 |
| def test_idle_message(self, preprocessor, cell_mock, message_mock): | ||
| preprocessor.run_cell(cell_mock) | ||
| # Just the exit message should be fetched | ||
| message_mock.assert_called_once() |
There was a problem hiding this comment.
This seems to be raising an error in 3.5 — any thoughts?
There was a problem hiding this comment.
That is weird... not sure why yet.
There was a problem hiding this comment.
Gah, since I changed to the built-in mock library for 3, this function isn't implemented in 3.5: https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_once
There was a problem hiding this comment.
I think it's good to switching to the built-in when possible — it sets us up for dropping the dependency when we drop support for python 2.
|
This looks great! We should probably get the tests passing before merging though. |
|
🎊 |
Refactoring run_cell wasn't really feasible to do safely without these tests. This should now allow #905 to be tested (and updated) with all code paths that are currently supported.