Conversation
|
All looks great to me. I am unsure about the best way to merge |
|
Added nbreview to this repository to allow reviews of notebboks. |
| @@ -4,54 +4,75 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
What's the reason to make params an argument for the function? I see you've added an example with a different set of parameters below. But tin the example we are building a test and the parameters need to be the good ones, the ones we want to check the call is being done so when the function is called that way. In other words, the defaults to the map_at function. Showing two set of params in this example diverts the understanding of what's being tested.
Reply via ReviewNB
There was a problem hiding this comment.
Thanks @dpshelio,
I think the example in the notebook is testing the use of the right parameters to do a right remote call: " Instead, we can use mocks to assert that our code does the right thing in terms of the messages it sends: the parameters of the function calls it makes to the remote resource". Just amended the cell to show this:
def test_build_default_params(_base, _params):
with patch.object(requests,'get') as mock_get:
default_map = map_at(51.0, 0.0)
mock_get.assert_called_with(_base, params=_params)
base="https://static-maps.yandex.ru/1.x/?"
good_params={
'z':12,
'size':'400,400',
'll':'0.0,51.0',
'lang':'en_US',
'l': 'map'
}
test_build_default_params(base, good_params)
and
base="https://static-maps.yandex.ru/1.x/?"
bad_params={
'z':15,
'size':'400,400',
'll':'0.0,51.0',
'lang':'en_US',
'l': 'map'
}
test_build_default_params(base, bad_params)
In any case, as I am new to this kind of testing. Examples require to be more intuitive and perhaps add more details for patching as there are few handy tests (e.g. https://kimsereylam.com/python/2021/03/19/how-to-use-patch-in-python-unittest.html). Happy to chat about this bits with @alessandrofelder and yourself next Tuesday :)
Thanks.
There was a problem hiding this comment.
I think a test called test_build_default_params should only test the default parameters. But I like the idea of showing the versatility of mocking. Could we have an additional bit of text after test_build_default_params that says something like "Mocking can also be used to test how we handle errors we receive from the remote resource" and then have a negative test that checks we handle a mocked dodgy internet connection as expected?
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
mxochicale
changed the title
alessandrofelder
left a comment
alessandrofelder
left a comment
There was a problem hiding this comment.
Some tiny additional comments :)
| "## 1. The Mock Object\n", | ||
| "**1.1 `unittest.mock` offers a base class for mocking objects called Mock.**" | ||
| "## The Mock Object\n", | ||
| "`unittest.mock` offers a base class for mocking objects called Mock. The following is an example how to define and object, call it and return id.\n" |
There was a problem hiding this comment.
| "`unittest.mock` offers a base class for mocking objects called Mock. The following is an example how to define and object, call it and return id.\n" | |
| "`unittest.mock` offers a base class for mocking objects called Mock. The following is an example how to define an object, call it and return it.\n" |
small typos
| "### 3.1 Using mocks to model test resources\n", | ||
| "### Using mocks to model test resources\n", | ||
| "Often we want to write tests for code which interacts with remote resources. (E.g. databases, the internet, or data files.)\n", | ||
| "We don't want to have our tests *actually* interact with the remote resource, as this would mean our tests failed\n", |
There was a problem hiding this comment.
| "We don't want to have our tests *actually* interact with the remote resource, as this would mean our tests failed\n", | |
| "We don't want to have our tests *actually* interact with the remote resource, as this would mean our tests could fail unpredictably from time to time, through no fault of our own, \n", |
mini-improvement, while we're here :)
| "source": [ | ||
| "That was quiet, so it passed. \n", | ||
| "When writing tests, one usually modifies one of the expectations, to something 'wrong', just to check it's not\n", | ||
| "When writing tests, we usually modifies one of the expectations, to something 'wrong', just to check it's not\n", |
There was a problem hiding this comment.
| "When writing tests, we usually modifies one of the expectations, to something 'wrong', just to check it's not\n", | |
| "When writing tests, we usually modify one of the expectations, to something 'wrong', just to check it's not\n", |
small typo
| @@ -4,54 +4,75 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
I think a test called test_build_default_params should only test the default parameters. But I like the idea of showing the versatility of mocking. Could we have an additional bit of text after test_build_default_params that says something like "Mocking can also be used to test how we handle errors we receive from the remote resource" and then have a negative test that checks we handle a mocked dodgy internet connection as expected?
3 participants
Addressing #220