launchpad-reviewers team mailing list archive

[İlker Emre Koç <mp+479274@xxxxxxxxxxxxxxxxxx>]

Currently, in Launchpad, multipart package is in version 0.2.4. We wish to upgrade it to 1.2.1 for it to be compatible with further versions of Python. But this upgrade throws multiple test suite failures in buildbot.

This package is used only once in the codebase, for parsing an HTTP request in a single test. As a result, it was expected to be a simple upgrade at first, which proved incorrect. And weirdly, the failures had nothing to do with that particular test.

It turns out, Zope also uses multipart in its HTTP parsings, which made it sneakily depend on multipart from our perspective. This partially explains the doctest failures we got previously, which exclusively happened on doctest sections with http() method calls.

The failed tests on buildbot:
xx-bugtask-assignee-widget.rst
xx-filebug-attachments.rst
xx-productseries-review.rst
xx-productseries.rst

An example test (part of xx-productseries.rst):

	>>> print(
... 	http(
	...     	r"""
... POST /firefox/+spec/svg-support/+setproductseries HTTP/1.1
	... Authorization: Basic celso.providelo@xxxxxxxxxxxxx:test
	... Referer: https://launchpad.test/
	... Content-Type: multipart/form-data; boundary=---------------------------26999413214087432371486976730
	...
	... -----------------------------26999413214087432371486976730
	... Content-Disposition: form-data; name="field.productseries"
	...
	... 2
	... -----------------------------26999413214087432371486976730
	... Content-Disposition: form-data; name="field.productseries-empty-marker"
	...
	... 1
	... -----------------------------26999413214087432371486976730
	... Content-Disposition: form-data; name="field.whiteboard"
	...
	... would be great to have, but has high risk
	... -----------------------------26999413214087432371486976730
	... Content-Disposition: form-data; name="field.actions.continue"
	...
	... Continue
	... -----------------------------26999413214087432371486976730--
	... """
	... 	)
	... )  # noqa
	HTTP/1.1 303 See Other
	...
	Content-Length: 0
	...
	Location: http://.../firefox/+spec/svg-support
	...

(Note that the test failures are reproducible in local environments, just requiring the upgrade to multipart)

The hypothesis: As stated in multipart’s version 1.0.0 changelog, the criteria for an HTTP request’s formatting has been increased within the package, turning a lot more cutthroat. As a result, the HTTP requests written with r-string and doc-string formatting, which was what all the failed doctests did, is insufficient when it comes to the expected format of multipart.

To test this, I changed a part of one of the doctests (xx-productseries.rst) to be extremely explicit in its formatting of HTTP requests:

	>>> print(
	... 	http(
	... 	"POST /firefox/+spec/svg-support/+setproductseries HTTP/1.1\r\n"
	... 	+ "Authorization: Basic celso.providelo@xxxxxxxxxxxxx:test\r\n"
	... 	+ "Referer: https://launchpad.test/\r\n";
	... 	+ "Content-Type: multipart/form-data; boundary=---------------------------26999413214087432371486976730\r\n"
	... 	+ '\r\n'
	... 	+ "-----------------------------26999413214087432371486976730\r\n"
	... 	+ 'Content-Disposition: form-data; name="field.productseries"\r\n'
	... 	+ '\r\n'
	... 	+ '2\r\n'
	... 	+ '-----------------------------26999413214087432371486976730\r\n'
	... 	+ 'Content-Disposition: form-data; name="field.productseries-empty-marker"\r\n'
	... 	+ '\r\n'
	... 	+ '1\r\n'
	... 	+ '-----------------------------26999413214087432371486976730\r\n'
	... 	+ 'Content-Disposition: form-data; name="field.whiteboard"\r\n'
	... 	+ '\r\n'
	... 	+ 'would be great to have, but has high risk\r\n'
	... 	+ '-----------------------------26999413214087432371486976730\r\n'
	... 	+ 'Content-Disposition: form-data; name="field.actions.continue"\r\n'
	... 	+ '\r\n'
	... 	+ 'Continue\r\n'
	... 	+ '-----------------------------26999413214087432371486976730--\r\n'
	... ))
	
HTTP/1.1 303 See Other
...
Content-Length: 0
...
Location: http://.../firefox/+spec/svg-support
…

With the above format, the failures ceased to exist. It seems these doctest break the format of HTTP request standard, and the overall tests break because of it.

(Note that the way the tests break were pretty sneaky themselves, returning a “200 OK” status code and all. There weren’t any indications that the format could be broken.)

(Once again, note that most HTTP requests are constructed through function calls and other similar mechanisms, automatically, with no human input. As a result, this kind of error could only happen in unittests and doctests like this which write the HTTP requests within strings themselves (which was stated within the multipart 1.0.0 changelog as a possible breakpoint))

Thus, my proposed solution is simple, rewrite the parts of doctests which call the http() method in a way that fit the standards. Either through such an explicit matter as I showed, or through using other proper HTTP request classes or methods, which are probably already used in the codebase somewhere.


-- 
https://code.launchpad.net/~ilkeremrekoc/launchpad/+git/launchpad/+merge/479274
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilkeremrekoc/launchpad:upgrade-multipart-discussion into launchpad:master.