Add support for combined test output (to land) #1212
Merged
Conversation
This file contains 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
Checkpointing so I can check Windows impl on an actual windows machine
Adds basic tests for TestOutput, particularly the line iterator
While adding tests for the libtestjson output, I noticed that my assumption that stdout/stderr ordering would be _mostly_ correct except in "extreme" scenarios was...completely wrong. With even the most simple mixed stdout/stderr output (no multithreading, no unbuffered output, no huge single writes) that the ordering of the output was wrong on my machine more often than not. After playing around with the idea of using a pseudo terminal, similar to how alacritty or other terminal emulators work, I checked the even easier case of...just passing the same file descriptor for both stdout and stderr...which works. Committing this in a separate branch so that I can push and add the Windows implementation before merging this back to the combined output branch.
sunshowers
force-pushed
the
combined-output
branch
from
93285cd
to
9e97ae2
Compare
sunshowers
force-pushed
the
combined-output
branch
from
9e97ae2
to
1b8b6ff
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1212 +/- ##
==========================================
- Coverage 78.26% 77.39% -0.88%
==========================================
Files 69 72 +3
Lines 17464 17860 +396
==========================================
+ Hits 13668 13822 +154
- Misses 3796 4038 +242 ☔ View full report in Codecov by Sentry. |
9 tasks
sunshowers
added a commit
that referenced
this pull request
Jan 9, 2024
For libtest JSON emulation, we need to have a way to model combined stdout and stderr output. The best way to do this happens to be to use the same file descriptor (HANDLE on Windows) for stdout and stderr. This is pretty useful generally, and in the future we may enable support for combined test output even outside the libtest-json context. The main alternative considered was having separate fds, but selecting from them into a single giant buffer with tracking indexes into it. However, that doesn't quite work because reading from pipes is done asynchronously with the test process writing to them. This typically causes lines from stdout and stderr to be clumped with each other. If the same fd is used as in this PR, the test process writes to stdout and stderr *synchronously*, which means that the combined output is recorded in the exact order the test process generates it in. Enabling combined output does mean that we lose the ability to track outputs separately, but that's not a huge deal. Co-authored-by: Rain <rain@sunshowers.io>
|
(the last commit did not have the correct attribution, so I force-pushed one that did) |
26 tasks
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.
This is a version of #1088 that's ready for landing.
The big changes compared to #1088 are: