fix: read the redirect log as bytes, not text (CII-240) - #426
Merged
hfudev merged 1 commit intoAug 21, 2026
Conversation
`_PopenRedirectProcess._forward_io` opened the redirect log in text mode while it was still being appended to. Every read reaches a temporary EOF, which finalizes the incremental decoder, so a UTF-8 sequence straddling that boundary raises UnicodeDecodeError. The surrounding `except Exception` swallows it: the process exits 0 with an empty stderr and DUT output stops for the rest of the session, surfacing much later as a pexpect timeout that names no cause. Decoding there was never needed - `MessageQueue.put` re-encodes through `to_bytes`, the pexpect buffer is bytes, and the serial transport already forwards `read_all()` undecoded - so reading in binary removes a failure mode and makes the two transports consistent. One behaviour change: text mode applied universal-newline translation, so a DUT sending CRLF was forwarded as LF. Binary mode preserves CR, which is what the serial transport already delivers and what the console writer and the Unity parser already handle.
Member
|
LGTM. Thank you for the fix. I'll do the release next Monday |
This was referenced Aug 31, 2026
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
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.
Fixes #425.
_PopenRedirectProcess._forward_ioread the redirect log in text mode while it was stillbeing appended to. Every read reaches a temporary EOF, which finalizes the incremental
decoder, so a UTF-8 sequence straddling that boundary raises
UnicodeDecodeError. Thesurrounding
except Exceptionswallows it — the process exits 0 with an empty stderr andDUT output stops for the rest of the session, surfacing later as a pexpect timeout that
names no cause.
Why binary rather than a decoder
The decode was never needed here:
MessageQueue.putacceptsstr | bytesand re-encodes throughto_bytes(), which isthe identity for bytes;
PexpectProcess's buffer is bytes;read_all()undecoded(
pytest-embedded-serial/pytest_embedded_serial/serial.py:244).So today's path is a bytes → str → bytes round trip. Reading in binary removes the failure
mode and makes the two transports consistent.
One behaviour change
Text mode applied universal-newline translation, so a DUT sending CRLF was forwarded as LF.
Binary mode preserves CR — which is what the serial transport already delivers, and both
_listenand the Unity parser handle CRLF already.test_forward_io_preserves_carriage_returnspins it so it stays deliberate.
Tests
Two unit tests next to the existing
test_listen_no_data_loss_*, in the same shape.The reproduction condition is worth noting: the multi-byte sequence has to be split
between two appends, which a byte-at-a-time UART/QEMU stream does naturally. Writing a
whole line in a single
write()does not reproduce it.On unmodified
mainboth fail:With this change both pass.
Test runs on this machine
pytest-embedded/tests/— 36 passed, 1 skipped, 2 failed.All of those failures reproduce on unmodified
main: I ran the same selection on both refsand the sorted
FAILEDlists are identical (emptydiff), with the same 47/24/9 totals.They need QEMU binaries, serial ports, an Arduino CLI or NuttX images, none of which are
present here.
test_temp_disable_packagesfails for a separate reason — it expectsImportErrorfor the sibling packages, which the editable install from CONTRIBUTING makesimportable.
Not included
Narrowing
except Exception.MessageQueue.putalready swallows a closed queue itself, andthe only escape I observed at teardown was
BrokenPipeError, but I have not shown that listto be exhaustive — so this PR stays one change. Happy to add it here if you would rather have
it together.