bpo-42060: Remove assert from http/client.py

[fbidu]

This PR removes assert statement from the stdlib's HTTP client.py and switches by an if...raise approach that won't be removed if optimization is requested

https://bugs.python.org/issue42060

[Fidget-Spinner]

I feel that this behavior is different than the original code's

assert response.will_close != _UNKNOWN

Furthermore, I'm unsure if HTTPException, is the correct exception to use here; the documentation states that ConnectionError is usually raised by getresponse

I would suggest instead:

if response.will_close == _UNKNOWN:
    raise ConnectionError("Unknown if response will close.")

Thanks for the contribution!

[fbidu]

Oh, you're completely right. I took the three as equals and missed the mark. Thanks for the review!

[Fidget-Spinner]

Hi, the Travis CI tests are failing due to whitespace issues, You can run patchcheck on the code to clear that up, the devguide has a simple guide on how to run it at https://devguide.python.org/pullrequest/#patchcheck. Additionally, you may want to consider adding a Misc/News entry using blurb or blurb-it since behavior for python when running under runtime optimization is different due to this PR. You can read how to do that at https://devguide.python.org/committing/#what-s-new-and-news-entries. Thanks!

[fbidu]

Done, @Fidget-Spinner!

I'm not very familiar with cpython's testing suite myself. Is there a way to add a test that will be executed with the optimization turned on? If so, I could add a test for this specific PR itself

[Fidget-Spinner]

@fbidu, unfortunately, I'm not too clear on this myself. You'll have to get help from someone else, sorry. AFAIK, (from looking around the Travis and Ubuntu automated tests), they don't test with -O, and there are other parts of the stdlib that have asserts that break during -O too, eg. https://bugs.python.org/issue23819. From my cursory look around the python test suite, I don't know of any tests that specifically test with -O.

One possible way I can think of is to either call subprocess.Popen with sys.executable yourself and call with -O, or you can use the convenient functions from test.support for that, eg:

from test.support.script_helper import spawn_python, kill_python
p = spawn_python('-O', '-c', '...')
kill_python(p)

As for where to put such a test, the unittests are found in Lib/test/. For your current case, I would guess Lib/test/httplib.py. You can refer to https://devguide.python.org/runtests/ on how to run tests. The tests use the unittest library so you can refer to the documentation for that.

Once again, please take my advice with a large grain of salt, I'm unsure if tests for -O are usually written.

[Fidget-Spinner]

reST requires two backticks for code, not one like markdown :).

Suggested change

	Replace `asserts` on HTTPResponse and HTTPConnection with `if...raises` so that the conditional checks work while running with the optimization flag switched on
	Replace ``assert`` in :class:`http.client.HTTPResponse` and :class:`http.client.HTTPConnection` with ``raise``
	so that conditional checks work while running with Python's runtime optimization
	``-O`` flag enabled.

[terryjreedy]

@pitrou @serhiy-storchaka @vadmium The asserts who conversion is proposed were added by:
line 575 Jeremy Hilton
line 597 Antoine Pitrou, patch by Jon Kun, bpo-13464
line 1366 Serhiy Storchaka, patch by Martin Panter, bpo-21032
Perhaps any of them have an opinion about the proposal.

[serhiy-storchaka]

Please add tests if these exceptions can be triggered by malformed input.

If it is not possible, using assert is correct.

[bedevere-bot]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

[github-actions]

This PR is stale because it has been open for 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

[iritkatriel]

There had been no response to the request for changes, so I am closing this as abandoned.