Opened 12 years ago
Closed 9 years ago
#687 closed enhancement (fixed)
Write unit tests for redirect and redirect_obj
Reported by: | Elrond | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | 0.9.0 |
Component: | programming | Keywords: | bitesized, test |
Cc: | ben@… | Parent Tickets: |
Description
Well, the title says it all. Those methods could use some unit tests.
For redirect
two cases should be tested: with location=
given, and without.
redirect_obj
has big magic.
Attachments (3)
Change History (15)
comment:1 by , 11 years ago
Owner: | set to |
---|---|
Status: | new → in_progress |
comment:2 by , 11 years ago
Owner: | removed |
---|---|
Status: | in_progress → review |
comment:3 by , 11 years ago
Status: | review → accepted |
---|
Whoops! I came back and noticed that I'd adding the attachment and commend to the wrong ticket. Please ignore the above.
by , 11 years ago
Attachment: | trac#687-test_redirect.diff added |
---|
Add test for redirect with location.
by , 11 years ago
Attachment: | trac#686-user.url_for_self.diff added |
---|
Add url_for_self function to User.
comment:4 by , 11 years ago
Status: | accepted → review |
---|
I've blanked out the irrelevant attachment with an empty file (trac#686-user.url_for_self.diff
) and have attached one initial test for request in trac#687-test_redirect.diff
. This is the easy test out of the ones requested above. Feedback appreciated since I haven't written py.test tests before.
The other test requested are a bit more complicated, partly I think because of this urlgen
stuff that's generated on the fly. This is where the magic is, not in redirect_obj
. Hmm. Not quite sure where to go there.
comment:5 by , 11 years ago
Cc: | added |
---|
comment:6 by , 11 years ago
Status: | review → accepted |
---|
Okay, so for this test, I don't think that we need to mock anything. You can just set res = redirect(...)
and then assert res.location == ...
.
As for testing redirect without location
kwarg, we need to get urlgen attached to the request object and I'm not sure of the best way to do that at the moment. Maybe Elrond or cwebb knows? I'll get back to you.
The other test should be fairly simple once we get urlgen attached to the request object. Just get a user
object, pass it into redirect_obj
and check that res.location
is the users homepage.
Thanks for starting this.
comment:7 by , 11 years ago
Keywords: | test added |
---|
comment:8 by , 10 years ago
I'd love some pointers on setting up the second test here. We need to get urlgen attached to the request object.
comment:9 by , 10 years ago
For testing redirect_obj, testing this with User
is one good thing.
Maybe we should test this with all supported objects?
But I would encourage to also have a test with a fake object, that has an url_for_self
method. Just let it call urlgen with some simple args, or something made up. I don't know yet.
comment:10 by , 10 years ago
tl;dr: There's no nice way at present, might become easier once we mandate having a host configuration in the config, but in the meanwhile, you could fake the request.
We don't really have a nice way to create nice "context"/request objects without making a real request. It's something I've been thinking of putting together... I have a prototype of this inn another application. But in this case, it's kind of important to have a request object because we don't know what the domain is unless it's coming in off of a request. This may change soon... we might *require* setting a host/domain in the config (a pretty big change) due to changes necessary for federation stuff.
In the meanwhile, one hack would be to create a werkzeug request manually (see app.py for how this is done), only attach what is needed for the method to complete, and then pass it into the relevant method.
I could probably walk through the relevant parts of app.py if needed on IRC.
comment:11 by , 9 years ago
Status: | accepted → review |
---|
I've attached a patch to test redirect
and redirect_obj
.
by , 9 years ago
Attachment: | issue_687.patch added |
---|
comment:12 by , 9 years ago
Milestone: | → 0.9.0 |
---|---|
Resolution: | → fixed |
Status: | review → closed |
These tests look great, Sturm! Merged and pushed!
(irrelevant comment removed - was for wrong ticket)