Fix reporting terminal dimensions #381
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
MisterDA
force-pushed
the
terminal-dimensions
branch
from
dace603
to
d25192a
Compare
MisterDA
force-pushed
the
terminal-dimensions
branch
2 times, most recently
from
f3c9b07
to
1eb8a29
Compare
|
@OlivierNicole has done a nice review for this PR, thanks. |
- simplify macro dance; - enable size detection on macOS; - use CAML_NAME_SPACE before OCaml 5.0.
This allows the library to report the correct size even when test are run buffered with `dune runtest`, as it changes the standard output of the executable. The correct size was reported when the test was run unbuffered (e.g., with `dune exec -- test.exe`).
In some platforms, incorrect reporting would lead to a division by zero.
Even though we had the C stubs, it was disabled in OCaml code.
MisterDA
force-pushed
the
terminal-dimensions
branch
from
1eb8a29
to
846cf88
Compare
art-w
approved these changes
Sep 20, 2023
|
|
||
| CAMLprim value ocaml_alcotest_get_terminal_dimensions(value unit) | ||
| { | ||
| CAMLparam1(unit); | ||
| CAMLlocal2(result, pair); | ||
| struct winsize ws; | ||
| int z = ioctl(STDOUT_FILENO, TIOCGWINSZ, &ws); | ||
| if (z == 0) | ||
| int fd = open("/dev/tty", O_RDONLY | O_NONBLOCK | O_CLOEXEC); |
There was a problem hiding this comment.
In case someone else is looking at this and doesn't know why STDOUT_FILENO was replaced by "/dev/tty" (like I was), the difference is when the output of alcotest is piped (say through a grep): stdout wouldn't have a size so the formatting would be different (and probably be confusing to users?), while the tty always return the same size (... I don't have strong opinion regarding this change, in general it's weird for piped programs to respect terminal size but it seems fine for alcotest)
dinosaure
approved these changes
Sep 20, 2023
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.
Fix division by zero when size of the terminal is incorrectly reported as zero. (fix Random crashes when running tests in a non-standard terminal #356, Fix reporting terminal dimensions #381, @MisterDA)
Enable terminal size reporting on macOS and Windows. Also report the terminal size even when the test is run buffered by Dune.
(Fix reporting terminal dimensions #381, @MisterDA)
Allow overriding the number of columns with
ALCOTEST_COLUMNSenv var. (Manually set output width #322, Fix reporting terminal dimensions #381, @MisterDA)My intent was to use a bigger default than 80 columns when the CI is detected. Sadly, this would seem to require too big of a refactoring to communicate the default size to the
Ppmodule, which is also why I haven't made the number of columns configurable programmatically, and exposed it only via an environment variable to fix reproducibility issues for the Unix platform.Oddly enough, communicating the bounds to the
Formatmodule withFormat.set_marginhides the first[OK],[FAIL], etc tag. I've haven't dug why.I've tested that this on macOS, Windows FreeBSD, and Linux.