tests: get connect_nonblock_xfer.py test working again and simplify it
#16156
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #16156 +/- ##
=======================================
Coverage 98.57% 98.57%
=======================================
Files 164 164
Lines 21345 21345
=======================================
Hits 21041 21041
Misses 304 304 ☔ View full report in Codecov by Sentry. |
4745d57 to
aeae52f
Compare
|
Code size report: |
projectgus
approved these changes
Nov 12, 2024
This follows the behaviour of unix MicroPython (POSIX sockets) and the esp32 port. Signed-off-by: Damien George <damien@micropython.org>
CPython changed its non-blocking socket behaviour recently and this test would not run under CPython anymore. So the following steps were taken to get the test working again and then simplify it: - Run the test against CPython 3.10.10 and capture the output into the .exp file for the test. - Run this test on unix port of MicroPython and verify that the output matches the CPython 3.10.10 output in the new .exp file (it did). From now on take unix MicroPython as the source of truth for this test when modifying it. - Remove all code that was there for CPython compatibility. - Make it print out more useful information during the test run, including names of the OSError errno values. - Add polling of the socket before the send/write/recv/read to verify that the poll gives the correct result in non-blocking mode. Tested on unix MicroPython, ESP32_GENERIC, PYBD_SF2 and RPI_PICO_W boards. Signed-off-by: Damien George <damien@micropython.org>
aeae52f to
6902362
Compare
Converting this test to use |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
CPython changed its non-blocking socket behaviour recently and this test would not run under CPython anymore. So the following steps were taken to get the test working again and then simplify it:
As part of this, it was found that bare-metal lwIP boards (eg PYBD_SF2, RPI_PICO_W) failed the test due to non-conforming behaviour of send/write on a non-blocking socket that's not yet connected. That issue is also fixed in this PR.
Testing
Tested on unix MicroPython, ESP32_GENERIC, PYBD_SF2 and RPI_PICO_W boards. This test and all existing network tests pass. Multinet tests pass.
Trade-offs and Alternatives
Matching CPython behaviour here would be a lot of work and changes to socket code, which now uses
BlockingIOError. I don't think we need to do that, not a good use of time, is a big churn of functionality,and many things need to be retested and fixed (eg
asyncio).esp8266 can't pass this test because axtls doesn't fully support non-blocking mode, so the test is skipped on this platform. It does actually pass the normal socket (not TLS) bits of this test, but separating those out into a separate test is not worth it, IMO.