Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7k
Reveal previously hidden AttributeErrors and TypeErrors#3668
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lovelydinosaur commentedNov 24, 2015
Prob worth issuing as two separate PRs. |
akx commentedNov 25, 2015
Sure, I'll snip that one out. |
tests/test_model_serializer.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Addressing any windows test failures should probably be treated as a separate PR.
I assume that's what this change is about, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yeah -- though it's a bit odd thatrepr would act differently across platforms...
akx commentedNov 26, 2015
I'll split the test stuff into yet another PR. :) |
93e344f to447ad2aComparetests/test_permissions.py Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just a regular AttributeError would be okay here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or perhapsreturn self.this_attribute_does_not_exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I subclassedAttributeError for the test for the purpose of being able to check for that exact exception being propagated up, which was the point of the patch in the first place :)
lovelydinosaur commentedNov 27, 2015
Okay, on review at this point I'd actually rather have this, butwithout the test cases. |
akx commentedNov 27, 2015
Well, if you're sure! I'll take the test cases out. |
Quietly catching `AttributeError` and `TypeError` when calling`get_queryset()` is rather insidious, as those exceptions get caught nomatter where they might happen in the call stack.
akx commentedNov 27, 2015
Tests are now gone,@tomchristie. |
lovelydinosaur commentedNov 27, 2015
May seem counter-intuitive, but they come with an associated cost, and can be okay not to strictly test against obviously better/cleaner behavior. |
Reveal previously hidden AttributeErrors and TypeErrors
This ensures AttributeErrors and TypeErrors do not get hidden when DRF is just looking for a
get_queryset().