673244 - test_delete_user failure on clean sreg build

Status: RESOLVED FIXED

(Cloud Services :: Server: Registration, defect)

Description

[Rob Miller [:rmiller]]

spire ~/tmp/server-sreg $ make test
bin/nosetests -s --with-xunit syncsreg/tests
..F.......
======================================================================
FAIL: test_delete_user (syncsreg.tests.test_user.TestUser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rob/tmp/server-sreg/syncsreg/tests/test_user.py", line 257, in test_delete_user
    self.assertEquals(res.json, WEAVE_MISSING_PASSWORD)
AssertionError: 6 != 7

----------------------------------------------------------------------
Ran 10 tests in 0.198s

FAILED (failures=1)
make: *** [test] Error 1

Comment 1

[Tarek Ziadé (:tarek)]

WebTest.delete API is not allowing bodies, and it looks related.

Can you try with this patch applied ? 

https://bitbucket.org/gawel/webtest/changeset/6721bda2c796

Comment 2

[Rob Miller [:rmiller]]

Nope, didn't seem to work.  After applying the patch to my venv's WebTest install, the original test is still failing, and in fact there's another failure:

spire ~/venv/server-full $ make test
bin/nosetests -s --with-xunit deps/server-core/services/tests deps/server-reg/syncreg/tests deps/server-storage/syncstorage/tests deps/server-sreg/syncsreg
....................................................................................F.........................................................F.......
======================================================================
FAIL: test_password_reset (syncreg.tests.functional.test_user.TestUser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rob/venv/server-full/deps/server-reg/syncreg/tests/functional/test_user.py", line 219, in test_password_reset
    self.assertTrue('Key does not match with username' in res)
AssertionError

======================================================================
FAIL: test_delete_user (syncsreg.tests.test_user.TestUser)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/rob/venv/server-full/deps/server-sreg/syncsreg/tests/test_user.py", line 257, in test_delete_user
    self.assertEquals(res.json, WEAVE_MISSING_PASSWORD)
AssertionError: 6 != 7

----------------------------------------------------------------------

Comment 3

[Tarek Ziadé (:tarek)]

I cannot reproduce those... :/ (neither jenkins btw)

Would you mind tracing them with pdb ?

Comment 4

[Rob Miller [:rmiller]]

I'll dig in since I can easily reproduce.

Comment 5

[Rob Miller [:rmiller]]

Okay, so the WebTest patch isn't sufficient b/c the source of the problem is in WebOb itself.  The following piece of code is the method behind WebOb's request.is_body_readable property getter:

https://bitbucket.org/ianb/webob/src/f98ba1b77341/webob/request.py#cl-686

...which checks the http_method_has_body dictionary...

https://bitbucket.org/ianb/webob/src/f98ba1b77341/webob/request.py#cl-33

...where it is specified that DELETE doesn't have a body, so the wsgi.input stream isn't even checked.

Possible solutions:

- try to get webob changed to allow reading the body value for DELETE requests

- hack the change into WebTest by overriding the is_body_readable property in TestRequest

- don't rely on sending info in the body for DELETE requests

#1 is relatively easy, codewise, but involves other players, convincing them that this should land in webob itself.

#2 seems very risky to me, b/c the behavior btn TestRequest and WebOb's real request would diverge.

#3 may be the easiest choice for us since we can do it in isolation w/o needing to change any external-to-services code.

Any other ideas I may have missed?

Comment 7

[Toby Elliott [:telliott]]

I suspect the answer is pinning to the previous webob version for now, and hoping this gets fixed.

If there's no interest in a fix, we'll probably want to go with a header instead of the body

Comment 8

[Tarek Ziadé (:tarek)]

Right now sreg is pinned at 1.0.7 if you use its makefile -- Pete got 1.0.8 because server-full does not pin it

Comment 9

[Ian Bicking (:ianbicking)]

It took a while to figure out when request bodies are allowed; HTTP 1.1 is vague, HTTP-BIS is a bit more specific:

http://tools.ietf.org/html/draft-ietf-httpbis-p2-semantics-15#section-7.7

"Bodies on DELETE requests have no defined semantics.  Note that sending a body on a DELETE request might cause some existing implementations to reject the request."

So, I guess the warning about existing implementations is accurate ;)  But I will change WebOb to accept it; in another place in HTTP-BIS it suggests any request with a Content-Length should just be accepted.

Change here:

https://bitbucket.org/ianb/webob/changeset/b3ef34c57936

Comment 10

[Rob Miller [:rmiller]]

Attachment: peg webob at the server-full layer

stop-gap measure until we get a DRY-compliant pinning system in place

Comment 11

[Tarek Ziadé (:tarek)]

was pushed by Rob: http://hg.mozilla.org/services/server-full/rev/d2430aca8e73