Skip to content

gh-131178: add E2E mockless tests for http.server command-line interface #134279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
May 24, 2025

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented May 19, 2025

This is a fresh PR because I don't know why GHA is stuck. Locally it works, and I don't know why it now fails remotely.

@picnixz
Copy link
Member Author

picnixz commented May 19, 2025

Ok, so the tests hang on macOS & Windows for some reason I'm not aware of. The tests are really more E2E tests but they can help catching possible issues so I'm keeping them.

@picnixz
Copy link
Member Author

picnixz commented May 19, 2025

!buildbot ASAN

@picnixz
Copy link
Member Author

picnixz commented May 19, 2025

Ok, since the build bots are struggling, I'll first remove the bits where we test the HTTP server. I'll try to see how we can reliably test this component later (I'm essentially applying PEP-11 recommendations when a Tier-1 bot is failing).

@picnixz picnixz marked this pull request as draft May 19, 2025 22:30
@picnixz
Copy link
Member Author

picnixz commented May 19, 2025

Ha!

0:11:22 load avg: 11.90 [1/2/1] test_httpservers failed (2 failures)
Re-running test_httpservers in verbose mode (matching: test_http_client, test_https_client)
test_http_client (test.test_httpservers.CommandLineRunTimeTestCase.test_http_client) ... 
python -m http.server:  Serving HTTP on :: port 54285 (http://[::]:54285/) ...
failed to start HTTP(s) server. Output was: 'Serving HTTP on :: port 54285 (http://[::]:54285/) ...\n'
FAIL
test_https_client (test.test_httpservers.CommandLineRunTimeTestCase.test_https_client) ... 
python -m http.server:  Traceback (most recent call last):
failed to start HTTP(s) server. Output was: 'Traceback (most recent call last):\n'

So there is something happening! I'll continue investigating this. At least, I think I know why it failed on the build bots. Presumably, the address being served was incorrectly parsed for some reason (namely it wasn't 127.0.0.1 or there was some exception being raised that I wasn't aware of)

@picnixz
Copy link
Member Author

picnixz commented May 20, 2025

!buildbot ASAN

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @picnixz for commit f735ee5 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F134279%2Fmerge

The command will test the builders whose names match following regular expression: ASAN

The builders matched are:

  • AMD64 Arch Linux Asan PR
  • x86-64 MacOS Intel ASAN NoGIL PR
  • AMD64 Arch Linux Asan Debug PR

@picnixz picnixz marked this pull request as ready for review May 20, 2025 01:05
@ggqlq
Copy link
Contributor

ggqlq commented May 23, 2025

I tried to reproduce these problems locally based on the commit that cause the buildbot failed(commit hash 605022a) and It appears that both of the AttributeError: 'NoneType' object has no attribute 'create_default_context' and AssertionError: False is not true issues mentioned in PR #134224 stem from running ssl in an environment that is missing the ssl module build. The server process in test_https_client will exit prematurely due to the missing ssl module and print the error traceback. The proc.stdout.readline() loop in the wait_for_server function will never find the server startup message because the output contains only the error traceback and the function returns False after timeout. The assertion self.assertTrue(wait_for_server(...)) fails with AssertionError: False is not true, while the subsequent fetch_file call will never reach SSL context operation that throw AttributeError, it explains why test_http_client and test_https_client failed with different reasons. I think both of the runtime tests failed are related to my mistake that I didn't check the ssl module build in PR #132540 and not due to specific environment, I think this PR have already fixed them by adding the skipIf(ssl is None) decorator.

Here's my reproduction of these bugs

  • CPU: AMD64
  • OS: Debian 12
$ git reset --hard 605022aeb69ae19cae1c020a6993ab5c433ce907

$ ./configure --with-openssl=donotexist
# ...

$ make -j12
# ...
Could not build the ssl module!
Python requires a OpenSSL 1.1.1 or newer

$ ./python -m test test_httpservers -v
# ...
======================================================================
ERROR: test_http_client (test.test_httpservers.CommandLineRunTimeTestCase.test_http_client)
----------------------------------------------------------------------
# ...
AttributeError: 'NoneType' object has no attribute 'create_default_context'

======================================================================
FAIL: test_https_client (test.test_httpservers.CommandLineRunTimeTestCase.test_https_client)
----------------------------------------------------------------------
# ...
AssertionError: False is not true

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

@picnixz
Copy link
Member Author

picnixz commented May 23, 2025

The proc.stdout.readline() loop in the wait_for_server function will never find the server startup message because the output contains only the error traceback and the function returns False after timeout

Ah thank you! this is something I couldn't understand as I couldn't show the traceback in full. Thank you very much for that.

@picnixz
Copy link
Member Author

picnixz commented May 23, 2025

Actually, there is something else. The errors were:

FAIL: test_https_client (test.test_httpservers.CommandLineRunTimeTestCase.test_https_client)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/ec2-user/buildbot/buildarea/3.x.itamaro-macos-intel-aws.macos-with-brew.asan.nogil/build/Lib/test/test_httpservers.py", line 1533, in test_https_client
    self.assertTrue(self.wait_for_server(proc, 'https', port, bind))
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: False is not true

However, I've already disabled the test if ssl is None, so False is being returned for another reason, not because of missing ssl.

@picnixz
Copy link
Member Author

picnixz commented May 23, 2025

!buildbot x86-64 MacOS Intel

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @picnixz for commit 7686f3a 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F134279%2Fmerge

The command will test the builders whose names match following regular expression: x86-64 MacOS Intel

The builders matched are:

  • x86-64 MacOS Intel NoGIL PR
  • x86-64 MacOS Intel ASAN NoGIL PR

@picnixz
Copy link
Member Author

picnixz commented May 24, 2025

Ok, I think I know what happened. When requesting 127.0.0.1, the response may be "Serving 127.0.0.1 on port (http://:)" where HOST is different from 127.0.0.1. So instead, we can just assume that having this message means that I can use 127.0.0.1 as a host component (sometimes I would see http://[::]:PORT but I wouldn't be able to use urllib.request on [::], which is why I would need 127.0.0.1 instead).

Therefore, I think I can merge this one without worries. If build bot fails (I can't seem to run them), I'll revert the PR (again). But the failing build bots now succeed, so it should be fine.

@picnixz picnixz merged commit 5d9c8fe into python:main May 24, 2025
38 checks passed
@picnixz picnixz deleted the patch/httpserver-test-131178 branch May 24, 2025 11:48
@picnixz picnixz changed the title gh-131178: fix SSL tests for http.server command-line interface gh-131178: add E2E mockless tests for http.server command-line interface May 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants