-
-
Notifications
You must be signed in to change notification settings - Fork 926
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
fix(testing.base.TestBase): parameterize api and srmock #577
Conversation
Looks good to me. |
"""Initializer, unittest-style""" | ||
|
||
super(TestBase, self).setUp() | ||
self._id = itertools.count(0) | ||
self.api = falcon.API() | ||
self.srmock = StartResponseMock() | ||
self.api = api or falcon.API() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: It's a good habit to explicitly test for None (e.g., falcon.API() if api is None else api
) since the object being passed in may (unexpectedly) evaluate to False, but in this case I don't think it matters since we are expecting instances of API and StartResponseMock which are known to always evaluate to True.
@swizzard Just a couple suggestions inline. Otherwise, could you please rebase onto master and squash down to a single commit? Thanks! |
I'd like to propose a new design here which I learned this trick first from Flask: class TestBase(unittest.TestCase):
api_class = falcon.API
srmock_class = StartResponseMock
def setUp(self):
# blah blah
self.api = self.api_class()
self.srmock = self.srmock_class()
# other blah blah I perceive that |
@philiptzou's We're actually already using this strategy in @swizzard What do you think? |
ed5146e
to
a8ee351
Compare
I implemented @philiptzou's fix. I usually just let my agonized, flailing git logs speak for themselves, so if I've messed something up on the rebase, please let me know. |
super(TestBase, self).setUp() | ||
self._id = itertools.count(0) | ||
self.api = falcon.API() | ||
self.srmock = StartResponseMock() | ||
self.api = self.api() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self.api = self.api_class()
attributes of `TestBase` to make subclassing easier. As currently implemented, `API` and `testing.srmock.StartResponseMock` objects are instantiated during `setUp`. This means subclasses either need to throw away those instances, or eschew the `super` call.
a8ee351
to
855551e
Compare
LGTM, thanks! |
fix(testing.base.TestBase): parameterize api and srmock
Make
api
andsrmock
arguments toTestBase.setUp
As currently implemented,
API
andtesting.srmock.StartResponseMock
objects are instantiated during
setUp
. This means subclasses eitherneed to throw away those instances, or eschew the
super
call.