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.

Sign up for GitHub

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

Jump to bottom

Fixed #26778 -- Fixed ModelSignal.connect() weak argument. #6802

Merged
merged 1 commit into from Jun 19, 2016
Merged

Fixed #26778 -- Fixed ModelSignal.connect() weak argument. #6802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion django/db/models/signals.py
View file
Open in desktop
Expand Up @@ -27,7 +27,10 @@ def _lazy_method(self, method, apps, receiver, sender, **kwargs):
return partial_method(sender)

def connect(self, receiver, sender=None, weak=True, dispatch_uid=None, apps=None):
self._lazy_method(super(ModelSignal, self).connect, apps, receiver, sender, dispatch_uid=dispatch_uid)
self._lazy_method(
super(ModelSignal, self).connect, apps, receiver, sender,
weak=weak, dispatch_uid=dispatch_uid,
)

def disconnect(self, receiver=None, sender=None, weak=None, dispatch_uid=None, apps=None):
if weak is not None:
Expand Down
15 changes: 14 additions & 1 deletion tests/signals/tests.py
View file
Open in desktop
Expand Up @@ -4,7 +4,7 @@
from django.db import models
from django.db.models import signals
from django.dispatch import receiver
from django.test import TestCase
from django.test import TestCase, mock
from django.test.utils import isolate_apps
from django.utils import six

Expand Down Expand Up @@ -258,6 +258,19 @@ def __call__(self, signal, sender, **kwargs):
self.assertTrue(b._run)
self.assertEqual(signals.post_save.receivers, [])

@mock.patch('weakref.ref')
def test_lazy_model_signal(self, ref):
def callback(sender, args, **kwargs):
pass
signals.pre_init.connect(callback)
signals.pre_init.disconnect(callback)
self.assertTrue(ref.called)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use ref.assert_called_once_with() here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure how to get the appropriate arguments:

ref.assert_called_once_with(callback)

AssertionError: Expected call: ref(<function callback at 0x7f1de47ec500>)
Actual call: ref(<function callback at 0x7f1de47ec500>, 
<bound method ModelSignal._remove_receiver of
<django.db.models.signals.ModelSignal object at 0x7f1de706d4d0>>)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From looking at the code the call could differ from Python 2 to 3 and is really an implementation detail which is not worth testing after all.

My initial reflexion was more about the fact assert_not_called() was used below instead of self.assertFalse(ref.called) but now I realize there's no assert_called() method.

LGTM

ref.reset_mock()

signals.pre_init.connect(callback, weak=False)
signals.pre_init.disconnect(callback)
ref.assert_not_called()


class LazyModelRefTest(BaseSignalTest):
def setUp(self):
Expand Down