django-wiki / django-nyt

Notification system for Django with batteries included: Email digests, user settings, JSON API
Apache License 2.0
144 stars 47 forks source link

_notification_type_cache on NotificationType seems to cause errors with tests #34

Open mikhuang opened 7 years ago

mikhuang commented 7 years ago

In the tests for my application it seems that NotificationType is cleared after running each TestCase. This leads _notification_type_cache to no longer be valid. My current workaround is to manually create any needed NotificationType objects at the start of each test. Given the nature of how _notification_type_cache should work, I'm not sure if there's anything to do about this beyond mentioning it in documentation. Thoughts?

benjaoming commented 7 years ago

In the tests for my application it seems that NotificationType is cleared after running each TestCase.

Yeah, this sounds like transactions being rolled back after each test case?

Given the nature of how _notification_type_cache should work, I'm not sure if there's anything to do about this beyond mentioning it in documentation. Thoughts?

You could empty the cache in tearDown?

mikhuang commented 7 years ago

Yup, transactions seem to be rolled back after each test case.

Good call on directly emptying cache. It feels a little hacky but I'm now using the following:

from django_nyt import models as django_nyt_models

def clear_django_nyt_cache():
    django_nyt_models._notification_type_cache = {}

I don't know where (if anywhere) this should be noted for anyone who runs into this in the future. Perhaps this issue existing is sufficient.

benjaoming commented 7 years ago

@mikhuang I'm pretty sure this is fixed now, would you mind testing 1.0b5 without your work-around? :)

Thanks for reporting!

mikhuang commented 7 years ago

Sorry for the delay, thanks for checking in! Unfortunately, I still seem to need my workaround as I get the same error without it:

IntegrityError: insert or update on table "nyt_subscription" violates foreign key constraint "nyt_s_notification_type_id_ca8af379_fk_nyt_notificationtype_key"
DETAIL:  Key (notification_type_id)=(MY_UPDATE_KEY_HERE) is not present in table "nyt_notificationtype".

Specifically, I don't believe any NotificationTypes are added or deleted between my tests so I imagine clear_notification_type_cache is never called. At the very least, I could change my tests to manually call that function instead of the hacky one that only lives in my code :-P

benjaoming commented 7 years ago

How do you create notification types in your tests? Creating them should purge the cache because of the signals.. but there might be something I've overlooked

mikhuang commented 7 years ago

Sorry, not sure what you mean (also sorry for the delay). I don't directly call notify() in my tests. Instead, notifications are created mostly in post_save hooks associated with my models. Hope that helps, thanks for following up.

benjaoming commented 7 years ago

It's possible that transaction rollbacks can happen without the cache being cleared. So for instance:

  1. New NotificationType x created
  2. Cache accessed and populated with x
  3. Transaction with x rollback (uncaught)
  4. Cache remains the same, however x is no longer in db