More unit testing of `NotificationRecipientService`, less integration testing of `NotificationService`
spec/services/notification_service_spec.rb
is consistently our slowest spec file. It's massive, at 1,500 lines.
Despite being a service spec, in actuality it's more like an integration spec. It's building up a bunch of user records, setting notification preferences on those users, then performing an action and verifying that the appropriate people are (or are not) emailed for that action based on their preferences.
As an example, let's look at one simple method that's heavily tested multiple times: #new_note
. It's a relatively simple method:
- Exit early if the note was somehow not on a noteable
- Exit early if the note is a system message
- Use
NotificationRecipientService
to build a list of recipients - Send a dynamic message to the
Notify
mailer based on thenoteable_type
At a unit level, that entire method can be tested with a few simple stubs, and it would take seconds to run.
This is what we're currently doing instead:
- We test it for notes on Issues
- We test it for notes on Issues when the author has enabled a setting
- We test it for notes on confidential Issues
- We test it for notes on Issues where
@all
is mentioned - We test it for notes on Project Snippets
- We test it for notes on Personal Snippets
- We test it for notes on Commits
These are all going through the exact same method in the service class. The method doesn't care at all if it's an issue, a confidential issue, a merge request, a snippet, or a commit. The thing we're actually testing in these tests is the NotificationRecipientService
class, which has zero unit tests.
@jneen recently did a great refactor of the recipient service class and I'm sure this was a major smell to her.
I think we can start unit testing all of these various conditions on the NotificationRecipientService
class, and greatly simplify these integration tests. It will likely be time-consuming, and it has to be done carefully because notifications are complicated and need to not break.