Tests are failing with the latest werkzeug 2.0.1
frenzymadness opened this issue · 6 comments
/usr/bin/python3 test_httpbin.py
....................................FFFFF......................
======================================================================
FAIL: test_redirect_n_equals_to_1 (__main__.HttpbinTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/builddir/build/BUILD/httpbin-0.7.0/test_httpbin.py", line 549, in test_redirect_n_equals_to_1
self.assertEqual(
AssertionError: 'http://localhost/get' != '/get'
- http://localhost/get
+ /get
======================================================================
FAIL: test_redirect_n_higher_than_1 (__main__.HttpbinTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/builddir/build/BUILD/httpbin-0.7.0/test_httpbin.py", line 527, in test_redirect_n_higher_than_1
self.assertEqual(
AssertionError: 'http://localhost/relative-redirect/4' != '/relative-redirect/4'
- http://localhost/relative-redirect/4
+ /relative-redirect/4
======================================================================
FAIL: test_redirect_to_post (__main__.HttpbinTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/builddir/build/BUILD/httpbin-0.7.0/test_httpbin.py", line 536, in test_redirect_to_post
self.assertEqual(
AssertionError: 'http://localhost/post' != '/post'
- http://localhost/post
+ /post
======================================================================
FAIL: test_relative_redirect_n_equals_to_1 (__main__.HttpbinTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/builddir/build/BUILD/httpbin-0.7.0/test_httpbin.py", line 555, in test_relative_redirect_n_equals_to_1
self.assertEqual(
AssertionError: 'http://localhost/get' != '/get'
- http://localhost/get
+ /get
======================================================================
FAIL: test_relative_redirect_n_higher_than_1 (__main__.HttpbinTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/builddir/build/BUILD/httpbin-0.7.0/test_httpbin.py", line 562, in test_relative_redirect_n_higher_than_1
self.assertEqual(
AssertionError: 'http://localhost/relative-redirect/6' != '/relative-redirect/6'
- http://localhost/relative-redirect/6
+ /relative-redirect/6
----------------------------------------------------------------------
Ran 63 tests in 6.087s
FAILED (failures=5)
sorry, previous comment was wrong. This is caused by a change in werkzeug. I bisected it, and the problematic commit I think is pallets/werkzeug@2cd4fa9 - this was a little hard to debug because there's a commit in the history that seems to break werkzeug entirely with Python 3.10 at least, I had to use an 'alternative history' branch where I fixed the bug in that commit to do a successful bisect.
As best I can tell, something somewhere in there is messing with responses before they're returned. for e.g. the failure in test_redirect_to_post - that test is backed by redirect_to in httpbin/core.py. If I log what the "Location" header in the response is right before redirect_to returns, it's correct - it's just /post. So something else between that function returning and the response actually getting back to test_redirect_to_post is messing with the response and making the "Location" absolute.
@davidism ahoy! Hope you don't mind the ping. Any idea what's going on here, as you wrote the commit that seems to have caused it? It's a big commit and I'm struggling to pinpoint the issue. The stuff about autocorrect_location_header obviously looks like a suspect, but I can't see that that code actually changed in the commit, so much as just moved...
As far as I understand it, autocorrect_location_header has always produced full URLs, not just paths. If you don't want that behavior, you can set Response.autocorrect_location_header = False, but I'm not sure what exactly that implies. I'm not sure why the tests were seeing path-only URLs before.
To debug this, I'd set a breakpoint in the if self.autocorrect_location_header block and see what it's doing differently in 1.x and 2.x.
yeah, I'm not saying that part actually is the problem, it's just the most obvious thing I saw on a brief look through the commit.
Most of the tests are of httpbin code that very explicitly intends to get relative URLs. redirect_to doesn't, in particular, but relative_redirect_n_times for instance has a bit of an obvious clue in the name, and does this:
response.headers["Location"] = url_for("relative_redirect_n_times", n=n - 1, _external=False)
where using _external=False is specifically ensuring it gets a relative path, not an absolute one. So httpbin is definitely specifically trying to get/return relative Locations in several of the tests, and previously was getting them, but after that commit it is ultimately producing absolute ones.
Your suggested next step was probably what I was gonna do next too, but I figured instead of flailing around blindly at "likely suspects" it'd be an idea to see if the person who wrote it could spot the issue :)
OK, so I added some debug prints and indeed the issue is autocorrect_location_header, somehow (I still don't have the how figured out). Before 2cd4fa9 , somehow, we do not enter the if self.autocorrect_location_header: block during the affected tests. After 2cd4fa9 , we do enter that block. Trying to figure out why now.
Ah. Heh. Turns out it's really pretty simple, and I dunno why I didn't notice it before...httpbin's core.py has this:
# Prevent WSGI from correcting the casing of the Location header
BaseResponse.autocorrect_location_header = False
which obviously stopped 'working' when the logic moved to Response. That should be fairly easy to fix.