Repository navigation
fix(cli): COLUMNS/LINES, BOM and UTF-16, artifact names, report summary - #742
Merged
Merged
Conversation
`termlens diff - missing.snap` read standard input to its end before it opened the right operand, so at a terminal it sat until Ctrl-D and only then said the file was missing. It is the mirror of #651, which fixed `diff missing.snap -` by reading the left operand first. `load` is now `open` followed by `read`. With `-` on the left, `diff` opens and measures the right operand before it reads standard input, and then reads the two in argument order. Measuring matters as much as opening: a directory opens on Unix and fails only on its first read, and a file past the 1 GiB bound is refused by its length. So `open` checks the length, and for a directory it makes that first read itself. The read waits on nobody, and the message stays the operating system's own, `Is a directory (os error 21)`, as `render --text /` has always printed. Nothing changes when neither operand is `-`, or when `-` is on the right: the left operand is still opened, read and parsed before the right one is touched. One message does change. With `-` on the left, a right operand that cannot be opened is reported even when standard input is not a screen either, where the parse error of standard input used to come first. Before, the new test in tests/cli.rs, diff_reports_an_unreadable_right_operand_before_reading_stdin, failed: Timeout { waiting_for: "`termlens diff - …/missing.snap` to exit", timeout: 5s, … } and `sleep 5 | timeout 2 termlens diff - missing.snap` and `… diff - /` both exited 124. Now the test passes and both exit 2 at once: termlens: missing.snap: No such file or directory (os error 2) termlens: /: Is a directory (os error 21) Taking the directory's read or the length check out of `open` fails the test again, on that operand. Refs #715 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Windows PowerShell 5.1's `>` re-encodes what `termlens inspect` prints
as UTF-16LE behind a byte-order mark, with CRLF endings, and
`Out-File -Encoding utf8` puts a UTF-8 mark in front. `render` and `diff`
refused both, so the CLI could not read back its own output saved in the
default Windows shell, and marked JSON was reported as a text snapshot
with a bad header.
`read` now decodes the bytes before it parses them. With no mark they
are UTF-8, as before, and are not copied. Behind `FF FE` or `FE FF` they
are UTF-16 in that byte order; nothing is guessed to be UTF-16 without
the mark (`FF` is in no UTF-8 text, so the mark cannot be mistaken). A
leading U+FEFF is then dropped, whichever encoding carried it, before the
CRLF fold and the `{` test that tells JSON from text.
The 1 GiB bound stays a bound on what is parsed: UTF-16 grows by half
when its characters are three bytes of UTF-8, so the text is measured in
one pass, refused past the bound ("larger than 1 GiB as UTF-8"), and
only then given exactly the room it measured, reserved fallibly. The
raw bytes are dropped when decoding returns, so the peak is the raw
bytes plus the text, no more than the CRLF fold of a UTF-8 file already
took. UTF-16 cut off mid-unit or holding a lone surrogate is refused as
"stream did not contain valid UTF-16" rather than read as U+FFFD.
Before, with the 0.11.8 code (the same files, from tests/data/before.snap):
utf8-mark.snap: … line 1: expected `size: COLSxROWS …`, got "\u{feff}---"
utf8-mark.json: … line 1: expected `size: COLSxROWS …`, got "\u{feff}{"
utf16le.snap: stream did not contain valid UTF-8
utf16be.snap: stream did not contain valid UTF-8
all exit 2, and the new test in tests/cli.rs,
render_and_diff_read_a_screen_behind_a_byte_order_mark, failed on the
first. Now `diff before.snap <each>` prints `no difference`, and the test
passes: five marked files (UTF-8 text and JSON, UTF-16LE text and JSON,
UTF-16BE text) each render as the original, from a file and on a pipe,
and diff clean in both operand orders; a cut-off and a lone-surrogate
file exit 2 naming UTF-16. Three unit tests pin `decode`: both byte
orders and surrogate pairs, refusal of broken UTF-16, and the bound
measured in UTF-8 (22 bytes of UTF-16 that decode to 33 are refused at
32 and read at 33). Keeping the decoding and removing only the mark's
strip fails the test again, on the UTF-8 file.
Refs #716
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
curses and `tput` take COLUMNS and LINES over the size the terminal
reports. A spawned program inherited the test process's environment, so
from a shell that exported them, `termlens inspect --inherit-env` and
any test without `env_clear` laid the program out at that shell's size
rather than at `size()` or `--size`, and kept it there after a resize.
The fix is in the library, since that is where the CLI and the inspect
example get the environment from: `TerminalBuilder` inherits by default,
so a test hit this as much as `inspect` did. `spawn.rs` now drops both
names from the inherited environment on both of its paths (std's
`Command` on Unix, the PTY layer's `CommandBuilder` elsewhere) before
the builder's own variables are applied, so `.env("COLUMNS", …)` and
`inspect --env COLUMNS=…` still reach the child as given. Under
`env_clear` nothing changes, since nothing is inherited. Dropping them
rather than pinning them to the PTY's size keeps a later resize visible
to the program.
The rustdoc of `spawn` and `env`, DESIGN §1 and §5, and the inspect
usage text (CLI and example, still in step) say so. CHANGELOG lists it
under Changed: a test that leaned on an inherited COLUMNS now has to set
it.
Before, `COLUMNS=200 LINES=50 termlens inspect --inherit-env --size
40x10 sh -c 'tput cols; tput lines; stty size'` printed 200, 50, 10 40.
Now it prints 40, 10, 10 40, and with `--env COLUMNS=33` it prints 33.
Three tests, each seen failing first:
- basic.rs columns_and_lines_are_not_inherited_but_can_be_set runs its
own binary again with COLUMNS=200 LINES=50 (setting the test process's
environment is not safe beside other tests) and spawns `emit` there.
Before: `kept=1 columns=200 lines=50`; now `kept=1 columns=unset
lines=unset`, and `columns=33` with `.env("COLUMNS", "33")`. `kept`
shows the rest of the environment is still inherited.
- cli.rs inspect_inherit_env_leaves_out_the_callers_columns_and_lines:
`[200 50]` before, now `[unset unset]` and `10 40` from stty, and
`[40 unset]` with `--env COLUMNS=40`.
- inspect.rs inspect_clears_and_selectively_sets_the_child_environment
now also passes COLUMNS and LINES to the example under --inherit-env:
`inherited:yes:200:50` before, `inherited:yes:unset:unset` now.
Taking the removal out of the Unix path alone fails the basic.rs test
again. The PTY-layer path is the one Windows takes, and runs there with
the same test; on Unix it is taken only when the kernel reaps children,
whose one test binary cannot re-run itself (std's `output` cannot reap
under SIG_IGN), so no Unix test reaches it.
Refs #709
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The artifact hook names each screen `<test>-<n>.screen.{txt,json}`,
where `<n>` counts per process and `<test>` is the thread name libtest
gives, the test's path inside its binary. Two test binaries with a
failing test of the same name (`smoke`, `help`, `tests::it_works`), or
two attempts at one test under cargo-nextest's retries, each counted
from 1 into a shared directory, and `std::fs::write` truncated: the
second screen replaced the first, and the report action showed one.
`artifact::write` now creates the file with `create_new`, and on
`AlreadyExists` takes the next number from the same counter and tries
again, so later screens in this process skip the taken names too. It
gives up after 65,536 taken names in a row and says so on stderr, as it
reports any other write failure; the error the screen came from is
returned regardless, as before. When nothing collides the names are the
same as before, and the report action reads every file under the
directory, whatever its name. The rustdoc of `Error` says a file there is
never replaced.
Before, the new test in tests/artifact.rs,
same_named_failures_in_two_processes_leave_two_files, failed with and
without `serde`:
left: ["same_named_failures_in_two_processes_leave_two_files-1.screen.txt"]
right: […-1.screen.txt, …-2.screen.txt]
It runs its own binary twice, as two processes, into one directory; each
run's failing wait leaves a screen that names the run. Now the test
passes in both formats: two files, `-1` holding the first run's screen
and `-2` the second's. The test above it in that binary sets the
variable in-process, and the new one records nothing in that process,
only in the two it starts, each given the directory explicitly.
Refs #711
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The report action puts each file name into the step summary's HTML, escaping its markup, and since #648 a newline as ` `, because a blank line ends the HTML block the name sits in. A carriage return was written raw: awk's records split on LF only, so `html()` passed it through. CommonMark ends a line at a lone CR as at an LF, so a `.snap.new` whose path held `\r\r# Injected **bold**\r\r` ended the block and the rest of the name rendered as Markdown, a real heading. `html()` now turns each CR into ` ` beside the other escapes. The artifact's own file names already mapped CR to `_`. tools/report-action-selftest gains a case: a file name holding `x\r\r# Injected **bold**\r\ry.txt`. Before, it failed twice: the summary held the CRs raw rather than ` # Injected **bold**`, and a raw CR was left in it. Now it passes: the name is escaped, its screen sits verbatim in its one block, and no CR is left. The same summary through markdown-it-py (CommonMark) rendered, before, `<h1>Injected <strong>bold</strong></h1>` and `y.txt</code></summary>` in a paragraph; now one `<code>` line holding the name. The gsub gives the same output under gawk, mawk and busybox awk. Refs #708 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
`cargo test` runs each test binary with its package's directory as the
working directory, so `TERMLENS_ARTIFACT_DIR=rel-art` writes screens to
`crates/a/rel-art`, `crates/b/rel-art` and so on. The report action
looked for `rel-art` from the workspace root, found nothing, and wrote
"the failures left no screen behind", which was false. No doc said the
directory must be absolute.
The render step now checks the directory it will search, whichever of
`artifact-dir`, TERMLENS_ARTIFACT_DIR or the default it came from. A
value that does not start with `/`, a drive letter and a separator, or
`\\` (so `D:\a\_temp\termlens` and UNC paths on a Windows runner count
as absolute) gets a `::warning` annotation that names it and says to
use an absolute path under runner.temp. `%`, CR and LF in the value are
encoded, so the value cannot end the workflow command and start another.
With nothing found, the summary says the path is relative and where
cargo put the screens, in place of the sentence saying none were left.
It is only a warning: the value is still searched as before, so a
single-package repository that uses a relative value today, where cargo
runs the tests in the root, still gets its screens, and nothing a
consumer passes now stops working. The message avoids `${{ }}`, which
the runner would evaluate inside a `run:` block.
The README, the action's README and its `artifact-dir` description,
SKILL.md §9b and the rustdoc of `Error` now say the directory must be
absolute, and why.
tools/report-action-selftest: `expect` now takes the case's
`artifact-dir` and TERMLENS_ARTIFACT_DIR from the case, and fails when
the step prints more or fewer `::warning` commands than the case
expects, so every absolute case also proves no warning. New cases: a
screen in `crates/app/rel-art` with `artifact-dir: rel-art`, and the
same through TERMLENS_ARTIFACT_DIR. Before the fix both failed with
"0 warning(s), expected 1", and the summary said the failures left no
screen behind. Now both pass: one warning, and the summary says
`rel-art` is relative and in each package's own `rel-art`. Three
absolute Windows shapes (`D:\…`, `D:/…`, `\\server\…`) draw no warning.
A value holding `%`, CR and LF came out as `%25`, `%0D` and `%0A`.
Refs #712
Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The report action appended every screen to $GITHUB_STEP_SUMMARY whole and never counted. GitHub fails a step's summary upload once more than 1 MiB is written to it and shows an error annotation instead, so past the limit the whole report was lost, small entries included, exactly when a suite had many or large failures. Each entry is now built in the work directory first and appended only while the summary, measured with `wc -c`, stays within 921,600 bytes (900 KiB), which leaves room for the closing lines. From the first entry that would pass it, no more are appended, even smaller ones, so the summary stays in order; every screen is still rendered into the artifact as before. The summary then says "N more screens are not shown here, to keep this summary under GitHub's 1 MiB limit for a step; the artifact holds every screen", above the line naming the artifact. The log line now reads "rendered N screen(s), M of them into the step summary"; nothing in the repository reads it beyond the selftest's "rendered 1 screen(s)" prefix, which still matches. The action's README says so. tools/report-action-selftest gains a case: six screens of 202,000 bytes each, 1.2 MB in all. Before, it failed: the summary was 1,260,980 bytes with all six entries and no line about the rest. Now it passes: 840,821 bytes, the first four screens verbatim, "2 more screens are not shown here", and six SVGs in the artifact. With the real CLI built from this tree, eight `.snap.new` files of a 250x80 screen whose every cell changed colour, each beside its `.snap` (an entry of about 772 KB, the cell diff 528 KB of it), gave a 772,417- byte summary: one entry, then "7 more screens are not shown here", and 16 renderings in the artifact. Before, that summary was over 6 MB. Refs #710 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
# Conflicts: # CHANGELOG.md
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.
Seven CLI, spawn and report-action fixes, one commit each.
Changed (each CHANGELOG entry has a migration sentence):
termlens inspect --inherit-envpasses the caller's COLUMNS and LINES, so the program lays out at the wrong size #709. A spawned program no longer inheritsCOLUMNSorLINES, andinspect --inherit-envno longer passes them: curses andtputprefer them over the PTY's own size. Set with.env()/--env, either still reaches the program.Fixed:
termlens diff - missing.snapstill waits for standard input before reporting the missing file #715.termlens diff - missing.snaprefuses the missing operand before reading standard input.renderanddiffrefuse a saved screen with a byte-order mark, and call BOM-prefixed JSON a bad text header #716.renderanddiffread a saved screen behind a byte-order mark, and in UTF-16 (PowerShell 5.1's>).TERMLENS_ARTIFACT_DIRis created exclusively and never replaces one already there; a taken name moves<n>on. .TERMLENS_ARTIFACT_DIRlands in each crate's directory, and the report action then says no screen was left #712. The report action warns when its artifact directory is relative, and the docs say it must be absolute.main is merged in. The merged tree passes fmt, clippy and
cargo test --workspace --all-features.Closes #708
Closes #709
Closes #710
Closes #711
Closes #712
Closes #715
Closes #716