Skip to content

Migrate QA PTY test harness from vt100 to rio-vt - #4931

Open
raphamorim wants to merge 3 commits into
Hmbown:mainfrom
raphamorim:main
Open

Migrate QA PTY test harness from vt100 to rio-vt#4931
raphamorim wants to merge 3 commits into
Hmbown:mainfrom
raphamorim:main

Conversation

@raphamorim

@raphamorim raphamorim commented Jul 27, 2026

Copy link
Copy Markdown

Swaps the QA PTY test harness (crates/tui/tests) from vt100 to rio-vt, Rio's terminal engine.

The Frame helper parsed the TUI's real PTY output with vt100 and read cell text and colors to assert on visible rendering. It now uses rio-vt through the same small surface. Cell colors are surfaced through a neutral qa_harness::Color enum (Default/Idx/Rgb) that mirrors exactly what the tests already matched on, so the assertions are unchanged; only the dev-dependency and the harness internals move.

The frame unit tests (blank interior columns, wide-glyph continuation cells) pass, and the PTY integration tests compile and run against the rio-vt harness.

Benchmark against vt100 and alacritty_terminal: https://github.com/raphamorim/rio-vt-benchmark

Background on the engine and why it was split out of Rio: https://rioterm.com/blog/2026/07/27/rio-vt-and-librio

No-Issue: migrate the QA PTY harness from vt100 to rio-vt

@raphamorim
raphamorim requested a review from Hmbown as a code owner July 27, 2026 19:05
@github-actions

Copy link
Copy Markdown
Contributor

Thanks @raphamorim for taking the time to contribute.

This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant recurring PR access by commenting /lgtm on a pull request.

@Hmbown

Hmbown commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Thanks for this @raphamorim — swapping the QA harness to rio-vt through a neutral Color enum is a clean shape, and keeping the Frame surface unchanged is the right call. I approved the workflow runs so this could actually get checked. Four things came back, and one of them is not yours to fix.

1. Compile error under -D warningsunused import: Color

Test (macos-latest) failed at compile, not at assertion:

error: unused import: `Color`
error: could not compile `codewhale-tui` (test "terminal_matrix_qa") due to 1 previous error
error: could not compile `codewhale-tui` (test "release_runtime_qa") due to 1 previous error

CI sets RUSTFLAGS: -Dwarnings, so an unused import is a hard failure. Both crates/tui/tests/terminal_matrix_qa.rs and crates/tui/tests/release_runtime_qa.rs import Color from the harness without using it — presumably a leftover from the vt100 surface. Locally cargo test -p codewhale-tui will not reproduce this without RUSTFLAGS=-Dwarnings, which is why it slipped.

2. cargo fmt --all -- --check is dirty

Lint failed in 28 seconds with zero compile requests, on the formatting step. The diff it printed is in the harness test code:

+            "done lost ANSI role"
+        );
         assert_ne!(

cargo fmt --all should clear it.

3. The link check wants one line in the PR body

link is a required check and it is failing only because the body has neither a closing keyword nor an opt-out. Since this is a test-harness migration with no issue behind it, add:

No-Issue: migrate the QA PTY harness from vt100 to rio-vt

I did not edit your PR body myself.

4. The one that needs a decision, not a fix — RUSTSEC-2020-0016

cargo-deny (advisories) fails on a transitive dependency the swap introduces:

error[unmaintained]: `net2` crate has been deprecated; use `socket2` instead
  ID: RUSTSEC-2020-0016
  Solution: No safe upgrade is available!

net2 v0.2.39
└── corcovado v0.5.0
    ├── rio-vt v0.5.0
    │   └── (dev) codewhale-tui v0.9.2
    └── teletypewriter v0.5.0
        └── rio-vt v0.5.0 (*)

This is a genuine, diff-caused finding rather than a flake, and it is the only one I would not just ask you to patch — it is upstream in rio-vtcorcovado.

It is worth weighing honestly: it is dev-only (never linked into a shipped binary), and it is unmaintained, not a vulnerability. So the risk is low. But cargo-deny is a required check and it does not currently distinguish dev from runtime for advisories, so as it stands this cannot merge.

Three ways forward, and I think the first is best:

  1. corcovado drops net2 for socket2 upstream. You are closest to that tree — is it viable? net2socket2 is usually mechanical.
  2. Scope the advisory exception to dev-dependencies in deny.toml, with a comment naming this advisory and why dev-only is acceptable. Narrow and auditable, but it does weaken a gate we deliberately keep strict.
  3. Blanket-ignore RUSTSEC-2020-0016. I would rather not — that hides it for runtime dependencies too.

If (1) has a timeline, I am happy to hold this PR for it. If not, tell me and I will take (2) as a separate maintainer commit so the exception lands with its own rationale rather than buried in your migration.

Items 1–3 are quick; item 4 is the actual gate. Ping me when it is pushed and I will re-run.

@raphamorim

Copy link
Copy Markdown
Author

Thanks for the review!

About net2: is only in corcovado's Windows (and fuchsia, never-built) TCP/UDP socket code, and nearly all of it is #[allow(unused)]. Rio uses corcovado for PTY/event polling, not network sockets, so this is dead mio-inherited API surface. The socket2 swap is feasible and I can compile-verify it for x86_64-pc-windows-msvc locally (teletypewriter already builds for that target and pulls corcovado). The only non-mechanical bit is keepalive (net2's Option vs socket2's TcpKeepalive/bool), and that method is dead.

@raphamorim

Copy link
Copy Markdown
Author

updated, corcovado drops net2 for socket2 upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants