Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added tests for each branch in execute's run_cell method #982

Merged
merged 5 commits into from Apr 16, 2019

Conversation

@MSeal
Copy link
Collaborator

MSeal commented Apr 7, 2019

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.

@MSeal MSeal requested review from Carreau and mpacer Apr 7, 2019
@MSeal MSeal added this to the 5.5 milestone Apr 7, 2019
Copy link
Member

mpacer left a 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):

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 15, 2019

Member

You could make this a *args and then change the code below to be

outcome = {}
for d in args:
    outcome.update(d)
return outcome

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 15, 2019

Member

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

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 15, 2019

Member

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

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 15, 2019

Member

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

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 15, 2019

Member

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):

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 15, 2019

Member

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.

@meeseeksmachine

This comment has been minimized.

Copy link

meeseeksmachine commented Apr 15, 2019

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

@MSeal MSeal force-pushed the MSeal:runCellTests branch from 5fc0139 to d06295b Apr 16, 2019
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()

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 16, 2019

Member

This seems to be raising an error in 3.5 — any thoughts?

This comment has been minimized.

Copy link
@MSeal

MSeal Apr 16, 2019

Author Collaborator

That is weird... not sure why yet.

This comment has been minimized.

Copy link
@MSeal

MSeal Apr 16, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
@mpacer

mpacer Apr 16, 2019

Member

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.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Apr 16, 2019

This looks great! We should probably get the tests passing before merging though.

@mpacer
mpacer approved these changes Apr 16, 2019
@mpacer mpacer merged commit 6975629 into jupyter:master Apr 16, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MSeal

This comment has been minimized.

Copy link
Collaborator Author

MSeal commented Apr 16, 2019

🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.