Test assertions for signals

[James Bennett]

At DUTH this year I opened a ticket to add test-case assertions checking whether (or not) a signal was sent:

https://code.djangoproject.com/ticket/27447

And corresponding PR:

https://github.com/django/django/pull/7508

The PR has gotten bogged down in API design discussion, so I'd like to hash that out here in hopes of landing this in time for 1.11 feature freeze.

The patch currently provides two assertions:

* assertSignalSent takes a signal and optionally a list of expected keyword arguments. It passes if the signal was sent and all expected arguments were present, and fails if the signal was not sent, or an expected argument was missing. It also functions as a context manager, allowing inspection of the values of the arguments (which are difficult to cleanly specify in the call signature of the assertion).

* assertSignalNotSent takes a signal and passes if the signal is not sent, and fails if it was.

The current discussion is mostly over the case of wanting assertSignalSent to also be able to distinguish how many *times* the signal was sent, perhaps assert a particular number of sends, and inspect each one individually.

Personally I'm against trying to do this in assertSignalSent, and would argue for a separate assertSignalSentMultiple if that use case is deemed important enough; coming up with a unified API that cleanly handles both the case of a signal send/inspection *and* also the case of multiple send/multiple inspection, seems like it would be more complicated than it needs to be (and having the assertion support two different behaviors for those cases seems doubly so).

[Aymeric Augustin]

Hello,

On 3 Jan 2017, at 09:16, James Bennett <ubern...@gmail.com> wrote:

> Personally I'm against trying to do this in assertSignalSent, and would argue for a separate assertSignalSentMultiple if that use case is deemed important enough; coming up with a unified API that cleanly handles both the case of a signal send/inspection *and* also the case of multiple send/multiple inspection, seems like it would be more complicated than it needs to be (and having the assertion support two different behaviors for those cases seems doubly so).

I agree that this sounds complicated and is better left out of scope for now.

To minimize cognitive load, we could mirror the unittest.mock APIs that perform the same function for mock calls. It provides:

- assert_called_with: succeeds if there’s at least one call and most recent call was made with the provided arguments
- assert_called_once_with: succeeds if there’s exactly one call and it was made with the provided arguments
- assert_any_call: succeeds if there’s at least one call with the provided arguments

assertSignalSent could work like assert_called_with. Later we can implement assertSignalSentOnce or other variants if needed.

If you prefer the semantics of assert_called_once_with or assert_any_call, that’s fine as well.

Regarding arguments, since signal receivers must accept **kwargs (that’s a document requirement), I think it’s acceptable for assertSignalSent to ignore unexpected arguments.

Best regards,

--
Aymeric.

[James Bennett]

On Tue, Jan 3, 2017 at 1:14 AM, Aymeric Augustin <aymeric....@polytechnique.org> wrote:

Regarding arguments, since signal receivers must accept **kwargs (that’s a document requirement), I think it’s acceptable for assertSignalSent to ignore unexpected arguments.

It's often the case that custom signals have particular arguments they want when being sent, and which receivers will depend on. Having a shortcut for saying "needs to have at least this set of arguments provided" seemed useful to me.

For background, this was written to test signal sending in django-registration, which does define a couple of custom signals. The code in the PR has been incorporated into a base TestCase class in django-registration already, and examples of how it's used can be seen in django-registration's tests. For example, where a repeated account activation should succeed and send a signal on the first try, but fail and send no signal on later attempts:

https://github.com/ubernostrum/django-registration/blob/master/registration/tests/test_hmac_workflow.py#L70 

[Tim Graham]

I agree with Carl's comment on the PR:

"I don't think it would complicate the API too much to have a count=1 kwarg, and if count > 1 expect received_kwargs to be a list of lists instead of just a list. This keeps the API identical to what it is currently for the common case, but makes it possible to make assertions about multiple signals. I think this is a better API than a totally separate assertion (we're still asserting that a signal was sent; what would the multiple version be named? assertSignalSentMultiple? Ugh.) This could be an enhancement in a separate PR, though.

Failing that, I think the assertion should pass if the signal was sent at least once with the named kwargs (if no kwargs named, then it should pass if sent at least once, period). The current behavior of just arbitrarily asserting against the kwargs of the most recent signal is not good, IMO."

To provide a real world use case, I tried using the assertions in Django's test suite. The context manager is a nice improvement except that most or all tests are testing that a single was sent once so using the assertion in its current form ("arbitrarily asserting against the kwargs of the most recent signal is not good") would be relaxing the assertions.