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.

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

_notification_type_cache on NotificationType seems to cause errors with tests #34

Open
mikhuang opened this issue May 29, 2017 · 7 comments · Fixed by #36
Open

_notification_type_cache on NotificationType seems to cause errors with tests #34

mikhuang opened this issue May 29, 2017 · 7 comments · Fixed by #36
Labels

Comments

@mikhuang
Copy link

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
Copy link
Member

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
Copy link
Author

mikhuang commented Jun 2, 2017

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
Copy link
Member

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

Thanks for reporting!

@mikhuang
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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 benjaoming reopened this Jul 25, 2017
@benjaoming
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants