test_form failed with treq 20.4.1

[yan12125]

I'm the maintainer of Arch Linux package python-klein, and I run a periodic script to check compatibility among Python packages. Recently I notice tests in python-klein no longer work with python-treq 20.4.1.

Steps to reproduce:

1. Clone commit c91085d of this repo

2. Change the following line to treq==20.4.1
   

   klein/tox.ini

   Line 46 in c91085d

   {test,coverage}: treq==20.3.0

3. Run tox -e coverage-py38-twcurrent

Actual results:

[FAIL]
Traceback (most recent call last):
  File "/home/yen/tmp/klein/.tox/coverage-py38-twcurrent/lib/python3.8/site-packages/klein/test/test_form.py", line 473, in test_customParameterValidation
    self.assertTrue(errorText.endswith("'not a number'"))
  File "/home/yen/tmp/klein/.tox/coverage-py38-twcurrent/lib/python3.8/site-packages/twisted/trial/_synctest.py", line 397, in assertTrue
    super(_Assertions, self).assertTrue(condition, msg)
  File "/usr/lib/python3.8/unittest/case.py", line 765, in assertTrue
    raise self.failureException(msg)
twisted.trial.unittest.FailTest: False is not true

klein.test.test_form.TestForms.test_customParameterValidation
===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/home/yen/tmp/klein/.tox/coverage-py38-twcurrent/lib/python3.8/site-packages/klein/test/test_form.py", line 409, in test_handlingGET
    self.assertEqual(calls, [(u"hello, big world", 4321)])
  File "/home/yen/tmp/klein/.tox/coverage-py38-twcurrent/lib/python3.8/site-packages/twisted/trial/_synctest.py", line 434, in assertEqual
    super(_Assertions, self).assertEqual(first, second, msg)
  File "/usr/lib/python3.8/unittest/case.py", line 912, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/usr/lib/python3.8/unittest/case.py", line 1118, in assertListEqual
    self.assertSequenceEqual(list1, list2, msg, seq_type=list)
  File "/usr/lib/python3.8/unittest/case.py", line 1100, in assertSequenceEqual
    self.fail(msg)
twisted.trial.unittest.FailTest: Lists differ: [('hello, big+world', 4321.0)] != [('hello, big world', 4321)]

First differing element 0:
('hello, big+world', 4321.0)
('hello, big world', 4321)

- [('hello, big+world', 4321.0)]
?              ^            --

+ [('hello, big world', 4321)]
?              ^


klein.test.test_form.TestForms.test_handlingGET

Expected results: tests pass

[twm]

Here's the full test on trunk:

klein/src/klein/test/test_form.py

Lines 392 to 409 in c91085d

	def test_handlingGET(self):
	# type: () -> None
	"""
	A GET handler for a Form with Fields receives query parameters matching
	those field names as input.
	"""
	router, calls = simpleFormRouter()
	stub = StubTreq(router.resource())
	response = self.successResultOf(
	stub.get(
	b"https://localhost/getme?name=hello,%20big+world&value=4321"
	)
	)
	self.assertEqual(response.code, 200)
	self.assertEqual(self.successResultOf(content(response)), b"got")
	self.assertEqual(calls, [(u"hello, big world", 4321)])

This looks like a behavioral change in treq. twisted/treq#265 made treq always parse URLs with hyperlink, rather than passing bytes URLs through.

Hyperlink seems to be percent-encoding the + when parsing the URL:

>>> from hyperlink import parse
>>> u = parse(b"https://localhost/getme?name=hello,%20big+world&value=4321".decode('ascii'))
>>> u
DecodedURL(url=URL.from_text('https://localhost/getme?name=hello,%20big%2Bworld&value=4321'))
>>> u.to_uri().to_text().encode('ascii')
b'https://localhost/getme?name=hello,%20big%2Bworld&value=4321'

This seems like a bug to me. + in the query part should be treated like %20. For example, this is what the web API does:

u = new URL("https://localhost/getme?name=hello,%20big+world&value=4321");
console.log(u.searchParams.get("name"));
// outputs: hello, big world

[dol-sen]

I can confirm this same error. But I am also getting a test fail at line 454 of test_customParameterValidation in test_form.py. The result is
"'not+a+number'" for
self.assertTrue( errorText.endswith("'not a number'") )

This sounds like the same hyperlink error

[wsanchez]

Hrm. I wonder why requires.io hasn't generated a PR with the treq update yet…

[yan12125]

There seems to be a bug in requires.io. treq 20.4.1 is released on Apr 17 [1] but the pull request created on Apr 19 [2] updated treq to 20.3.0.

[1] https://github.com/twisted/treq/releases/tag/release-20.4.1
[2] #337

[wsanchez]

OK I can open a PR and manually update some things, but I have a queue of PRs open that need review before I want to throw more on the pile.

[wsanchez]

OK #352 will update everything else, but we'll need a resolution on a fix to this before updating treq.

[wsanchez]

See python-hyper/hyperlink#129 for debate as to whether this test is correct.

[twm]

There was a Hyperlink release on Friday, so twisted/treq#304 is ready for review! I'll do a treq release as soon as that's in.

[yan12125]

Thanks for the update! I can confirm with hyperlink 21.0.0, all klein tests are green.

[wsanchez]

Thanks! Closing.