-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add mocked_relations decorator for all related models. #28
Conversation
I added Django 1.9 and 1.10 to the tox environments, because mocked_relations is affected by changes in the Django plumbing. One of the serializer tests is broken by a change in Django 1.10, so I marked it as skipped for now. I'm not sure how to fix it.
Current coverage is 100% (diff: 100%)@@ master #28 diff @@
===================================
Files 6 6
Lines 369 479 +110
Methods 0 0
Messages 0 0
Branches 0 0
===================================
+ Hits 369 479 +110
Misses 0 0
Partials 0 0
|
Thanks and happy new year! Indeed this is a big one! I will experiment with it over the next few days and get back to you. About the broken test by Django 1.10, I looked at the code carefully but I can't seem to find why I thought it was useful, since django-rest-framework includes a field even if null. My guess is that we can delete it since it doesn't specify an expected behavior. If the coverage drops, I will (probably) understand its purpose if any and define it better so it's clearer. |
Have you had a chance to look at this? I have a couple of smaller patches ready, but I thought it would be easier to wait until this one is resolved before I submit those. |
Hey there! Sorry for the delay i've been very busy this month and didn't wanna fire off a hasty response. The PR looks good, but since it kind of overlaps with a feature (#31) I have been working on I wanted to get your opinion on how to combine the two before merging. I'm referring to "Implement decorators for unified model patching" from the TODO section in the README. Basically I also wanted to provide the ability to patch a chain of methods of a Django model (or any class for that matter), and be able to verify that those were called with the expected parameters. For example to be able to replace this: class TestCar(TestCase):
qs = MockSet()
@patch.object(Car, 'objects', qs)
@patch.object(Car, 'validate_price')
@patch.object(Car, 'save')
def test_car_validates_price_before_save(self, save_mock, validate_price_mock):
validate_price_mock.return_value = True
obj = Car(price=80000, speed=300)
obj.save()
validate_price_mock.assert_called_with() with this: class CarModelMocker(ModelMocker):
""" If a model method passed in the test is provided it is bootstrapped
with the implementation below otherwise it's replaced with a MagicMock """
def validate_price(self):
""" Skip expensive call to remote api """
return True # Or a more complex response
class TestCar(TestCase):
@CarModelMocker(Car, 'validate_price', 'method_to_skip1', 'method_to_skip2...')
def test_car_validates_price_before_save(self, mocker):
obj = Car(price=80000, speed=300)
obj.save()
mocker.method('validate_price').assert_called_with() |
Hey @donkirkby, before we merge the PR, I wanted to ask you about the changes to the tox environments. With that serializer test skipped, I run tox with the original python/django version combinations and everything works correctly. I used
Can you have a look and in case all possible combinations work as expected to undo changes to .travis.yml, tox.ini and requirements-testing.txt? Thanks! |
I will certainly undo those changes if you want, @stphivos, but I added in the different versions of Django to catch differences in the relationship managers. As an example, this code handles the fact that the
If I didn't test with more than one Django version, I wouldn't have caught that. I can see three possibilities:
Obviously, it's a trade off between being quick and being thorough. Let me know which option you want, and I'll adjust the PR. |
I see what you mean, yeah I will merge it as it is then and we stick with the combinations that work. Thanks! |
That's great, thanks for the merge! When you're finished with PR #29, I'll create another one with several small fixes. |
Sounds good! 👍 |
Here's a slightly late Christmas gift. This is the feature we've been stabilizing in our project, and I think it's ready now. It's a big change, so just ask if you want me to make some changes to make it more compatible with your project.
I added Django 1.9 and 1.10 to the tox environments, because mocked_relations is affected by changes in the Django plumbing.
One of the serializer tests is broken by a change in Django 1.10, so I marked it as skipped for now. I'm not sure how to fix it, because I don't really understand what it's testing. If you can explain the test to me, I might be able to help adapt it to Django 1.10.
Here's the Django change that broke the serializer test: