using get_or_create when testing for group membership #273
Conversation
|
@gandalfar: Does this make sense? |
| group = Group.objects.get(name="ambassadors") | ||
| group, created = Group.objects.get_or_create(name="ambassadors") | ||
| if created: | ||
| group.save() |
There was a problem hiding this comment.
I'm not sure if this should ever be done in a test...
if I correctly understand what this does, at least ;)
There was a problem hiding this comment.
What do you feel should not be done, the get_or_create invocation, or something else?
There was a problem hiding this comment.
the group.save()...
you know, the line I commented on :)
There was a problem hiding this comment.
I guess I have to ask then, why do you think object should not be saved in a test? I am asking, so you can share your knowledge about this stuff... You know us being a learning group and all...
There was a problem hiding this comment.
because in this case the ambassadors group should be created during migrate (0002_adding_groups_and_setting_permissions.py), but that doesn't happen for tests at the moment (as far as I understand, this is the issue @gandalfar is having)
a workaround in tests that "fix" stuff that should have happened elsewhere is bad practice
There was a problem hiding this comment.
Well if this is neessasy for now until we figure out the fix for the migration than let's leave it for the refactathlon.
You said it so perfectly :)
There was a problem hiding this comment.
in that case I recommend a note (in a comment) stating it should be removed later on
|
It's actually my bug. South migrations aren't being run. I'm checking what's going on now. |
|
Goran, would it make it too much of a problem if you unmered my pull request? Github allows you to revert it - https://github.com/blog/1857-introducing-the-revert-button Sorry for this mess. I'll have a better pull request next week. Alternatively you can merge Alja's workaround and I'll assign myself a bug to recode it properly. |
|
@gandalfar which one? you can revert the commits from the pull request yourself and create a new pull request, which "undoes" the first one ;) |
| def test_get_ambassadors_for_country(self): | ||
| self.up1.country = "SI" | ||
| self.up1.save() | ||
|
|
||
| group = Group.objects.get(name="ambassadors") | ||
| group, created = Group.objects.get_or_create(name="ambassadors") | ||
| if created: |
There was a problem hiding this comment.
This is not necessary, since get_or_create saves the created object already.
https://docs.djangoproject.com/en/dev/ref/models/querysets/#get-or-create
Not sure what you felt that this has to be here?
There was a problem hiding this comment.
just an additional test to ensure that the underlying layer actually saves the object
There was a problem hiding this comment.
Second south migration adds 'ambassadors' group permission. There is a bug in py.test and django_pytest that doesn't correctly run south migrations. I'm still looking up for a fix but it's getting a bit tricky.
There was a problem hiding this comment.
@goranche I don't know. We could then (re)write the tests for all of the Django, if we adopt this mindset. Not to mention, this is a "bolognese" at it's best :).
I am not exactly sure what is the problem with South? Is this needed because the migrations arent runinig? I am intrigued about pytest having this bug, what exactly is it?
There was a problem hiding this comment.
@ercchy everything is bolognese to you ;)
There was a problem hiding this comment.
Only the latest version actually runs South migration - pytest-dev/pytest-django#129 . Last week this patch blew up django-taggit and I haven't got time yet to make a reproducible test case for them. I plan to do it on Wednesday.
There was a problem hiding this comment.
ugh... @ercchy somehow I thought your comment was about another test...
never mind me 👼
(except for the "everything is bolognese to you" 😬 )
Also made sure the country updates when the marker is being manually marked and fixed a few other smaller issues related to that.
Getting read of some unused variables, making variable names more readable.
|
Good Job Cat! |
using get_or_create when testing for group membership
No description provided.