TestApp.delete causes spurious warning #50

Closed
merwok opened this Issue Mar 2, 2013 · 8 comments

Comments

Projects
None yet
3 participants
@merwok

merwok commented Mar 2, 2013

webtest/app.py:550: WSGIWarning: You are not supposed to send a body in a DELETE request. Most web servers will ignore it

This is caused by a test that checks if params is not NoDefault, but the default value is u'' instead of NoDefault.

(Side note: using the stacklevel param of warnings.warn will cause the traceback to show the line that called the function, not the line of the function that calls warn).

@domenkozar

This comment has been minimized.

Show comment
Hide comment
@domenkozar

domenkozar Mar 3, 2013

Member

Basically adding correct default to all methods would fix this. With a test case that warning is raised and adding stacklevel param to the warning.

Member

domenkozar commented Mar 3, 2013

Basically adding correct default to all methods would fix this. With a test case that warning is raised and adding stacklevel param to the warning.

@merwok

This comment has been minimized.

Show comment
Hide comment
@merwok

merwok Mar 3, 2013

I could make a PR if you’re patient with my lack of git-fu.

merwok commented Mar 3, 2013

I could make a PR if you’re patient with my lack of git-fu.

@domenkozar

This comment has been minimized.

Show comment
Hide comment
@domenkozar

domenkozar Mar 3, 2013

Member

Sure, how can I help? :)

Member

domenkozar commented Mar 3, 2013

Sure, how can I help? :)

@merwok

This comment has been minimized.

Show comment
Hide comment
@merwok

merwok Mar 4, 2013

I know how to clone, commit and push, but merging is still infuriatingly confusing to me, so if I use the wrong branch or get an unmergeable PR, I’ll need help :)

merwok commented Mar 4, 2013

I know how to clone, commit and push, but merging is still infuriatingly confusing to me, so if I use the wrong branch or get an unmergeable PR, I’ll need help :)

@domenkozar

This comment has been minimized.

Show comment
Hide comment
@domenkozar

domenkozar Mar 4, 2013

Member

Workflow is the following:

  • you fork this repo
  • git clone your fork
  • commit changes
  • push back to your repo
  • on github, click "pull request", review changes and submit it

On Mon, Mar 4, 2013 at 6:40 AM, Éric Araujo notifications@github.comwrote:

I know how to clone, commit and push, but merging is still infuriatingly
confusing to me, so if I use the wrong branch or get an unmergeable PR,
I’ll need help :)


Reply to this email directly or view it on GitHubhttps://github.com/Pylons/webtest/issues/50#issuecomment-14364792
.

Member

domenkozar commented Mar 4, 2013

Workflow is the following:

  • you fork this repo
  • git clone your fork
  • commit changes
  • push back to your repo
  • on github, click "pull request", review changes and submit it

On Mon, Mar 4, 2013 at 6:40 AM, Éric Araujo notifications@github.comwrote:

I know how to clone, commit and push, but merging is still infuriatingly
confusing to me, so if I use the wrong branch or get an unmergeable PR,
I’ll need help :)


Reply to this email directly or view it on GitHubhttps://github.com/Pylons/webtest/issues/50#issuecomment-14364792
.

@merwok

This comment has been minimized.

Show comment
Hide comment
@merwok

merwok Mar 11, 2013

Yep, I know the basic flow as I said in my previous message, it’s really the branching model and the synchronization that don’t fit my head yet.

Basically adding correct default to all methods would fix this.

I did that and it does remove the spurious warning for DELETE.

With a test case that warning is raised and adding stacklevel param to the warning.

Problem is that warnings use their own reporting system, so you can’t just use assertRaises. In the stdlib we have special helpers to check that a warning was sent, but there’s nothing to do that in webtest at present.

stacklevel=3 does the right thing (3 = _gen_request → delete method → caller). I wanted to separate the two changes in two commits and then git hated me so I won’t push anything today, but I wanted to confirm the fixes.

merwok commented Mar 11, 2013

Yep, I know the basic flow as I said in my previous message, it’s really the branching model and the synchronization that don’t fit my head yet.

Basically adding correct default to all methods would fix this.

I did that and it does remove the spurious warning for DELETE.

With a test case that warning is raised and adding stacklevel param to the warning.

Problem is that warnings use their own reporting system, so you can’t just use assertRaises. In the stdlib we have special helpers to check that a warning was sent, but there’s nothing to do that in webtest at present.

stacklevel=3 does the right thing (3 = _gen_request → delete method → caller). I wanted to separate the two changes in two commits and then git hated me so I won’t push anything today, but I wanted to confirm the fixes.

@kmike

This comment has been minimized.

Show comment
Hide comment
@kmike

kmike Jul 16, 2013

Contributor

I think the correct way is to fix the warning issuing logic. Why is checking for if method == 'DELETE' and params: not enough? How is u'' non-empty?

Contributor

kmike commented Jul 16, 2013

I think the correct way is to fix the warning issuing logic. Why is checking for if method == 'DELETE' and params: not enough? How is u'' non-empty?

@merwok

This comment has been minimized.

Show comment
Hide comment
@merwok

merwok Aug 1, 2013

Thanks for the fix @noonat ! The stacklevel value is still the old one, I may open another ticket for that.

merwok commented Aug 1, 2013

Thanks for the fix @noonat ! The stacklevel value is still the old one, I may open another ticket for that.

@pyup-bot pyup-bot referenced this issue in scieloorg/scielo-manager Aug 26, 2016

Closed

Update django-webtest to 1.7.9 #1330

@pyup-bot pyup-bot referenced this issue in scieloorg/scielo-manager Sep 14, 2016

Closed

Update django-webtest to 1.8.0 #1338

@pyup-bot pyup-bot referenced this issue in Thermondo/viewflow-extensions Nov 17, 2017

Closed

Pin django-webtest to latest version 1.9.2 #173

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment