nextest-rs/nextest

Add option to use a pty for tests (Inappropriate ioctl for device error during TUI tests)

Opened this issue · 8 comments

orhun commented

Hello 👋🏼

We are experiencing some nextest related test failures here: ratatui-org/ratatui#981

In a nutshell, initializing a terminal with ratatui with termion backend during tests throws the following error for cargo nextest run:

Error: Os { code: 25, kind: Uncategorized, message: "Inappropriate ioctl for device" }

Whereas cargo test works fine.

Here is a minimal reproducibility example:

cargo new repro && cd repro/
cargo add ratatui --no-default-features --features termion
cargo add termion

Then add the following test:

use ratatui::{backend::TermionBackend, Terminal};
use std::io::{self};

#[test]
fn termion_backend() -> Result<(), Box<dyn std::error::Error>> {
    let mut stdout = io::stdout();
    let backend = TermionBackend::new(&mut stdout);
    let mut terminal = Terminal::new(backend)?;
    Ok(())
}

cargo nextest run:

Details 
    Starting 1 test across 1 binary
        FAIL [   0.004s] repro::bin/repro termion_backend

--- STDOUT:              repro::bin/repro termion_backend ---

running 1 test
test termion_backend ... FAILED

failures:

failures:
    termion_backend

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s


--- STDERR:              repro::bin/repro termion_backend ---
Error: Os { code: 25, kind: Uncategorized, message: "Inappropriate ioctl for device" }

   Canceling due to test failure
------------
     Summary [   0.005s] 1 test run: 0 passed, 1 failed, 0 skipped
        FAIL [   0.004s] repro::bin/repro termion_backend
error: test run failed

cargo test:

Details 
running 1 test
test termion_backend ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
sunshowers commented

Thanks for the issue, and also for maintaining ratatui which I love!

This occurs because nextest doesn't allocate a pty for each test. It's something I've been wanting to do but never have had the time to.

We aren't going to change the default, but this is something that can be managed in configuration -- for some or all tests, as desired.

I don't have the time to work on it within the next few weeks, but I think you're kind of an expert in the area being a ratatui maintainer! If you'd like to implement this I'd love it -- we just need to add a use-pty config in both the profile config and in per-test overrides. It would be lovely to do this on both Unix and on Windows, using the portable-pty crate.

  • 👍1
  • ❤️1
orhun commented

Absolutely, I would love to take a stab at this issue. Not sure when but I'm thinking I can get to it pretty soon. Thanks for the explanation and kind words! 🧀

  • ❤️1
orhun commented

Hello again! Sorry it took me forever to get to this. I have a couple of points that I might need your guidance:

First, I thought of using CommandBuilder instead of std::process::Command along with a master/slave pair for executing things in a pty. That attempt failed due to the missing functions of CommandBuilder such as stdin and current_dir. This approach will also require a lot of refactoring across nextest-runner, so I'm not sure if I'm doing the right thing here.

  • we just need to add a use-pty config in both the profile config and in per-test overrides

I'm not fully familiar with the codebase but I'm guessing we would like to add a config option. But maybe I misunderstood that, can you clarify what you mean by per-test overrides?

sunshowers commented

Thanks! I think the way to address that would be to write an abstraction over the different ways to run various commands, behind an enum or trait object.

However, I noticed while looking at portable-pty that it doesn't seem to have async support. So we may need to wrap portable-pty to make it work in the async world. To do this, we have to think about a few things:

@wez -- hey! Wondering if you have thoughts on making portable-pty work with async. Is that something you'd be willing to accept upstream?

wez commented

hey @sunshowers! portable-pty doesn't currently implement async directly because Windows conpty is documented as only supporting synchronous I/O:

https://learn.microsoft.com/en-us/windows/console/createpseudoconsole

An open handle to a stream of data that represents user input to the device. This is currently restricted to synchronous I/O.

(and the same for its output).

Whether that happens to work with IO completion ports or other async techniques on Windows, I don't know. From a system's perspective, in portable-pty, those streams are always pipes which are fundamentally compatible with async IO, but I'm not sure what sorts of other magic maybe involved that this restriction is documented. Maybe @DHowett-MSFT can weigh in on this?

We do have an example of async use at a higher level via the smol crate and its Unblock helper:

https://github.com/wez/wezterm/blob/main/pty/examples/whoami_async.rs

I'm certainly open to PRs that make this more integrated, but I do want to note that it is desirable (although not a fully hard requirement if a good case is made for it) that the implementation doesn't make tokio a requirement for the crate. The reason being that wezterm's async runtime is smol based, as that was the sanest thing to integrate fairly directly into the GUI loop. I think having one or more crate features to control this is a fine outcome.

I think the signals side of this might be a bit easier to wrangle, but I do want to note that synchronizing output/error and signal delivery is a potentially tricky problem, because the output/error pipes may still have buffered data at the time a signal is detected.

sunshowers commented

Thanks @wez! The whoami_async approach makes sense to me.

I think the signals side of this might be a bit easier to wrangle

Hmm the Tokio code to handle things like SIGCHLD is pretty hairy, requiring global tracking of all processes since SIGCHLD can be coalesced. See https://github.com/tokio-rs/tokio/blob/6fcd9c02176bf3cd570bc7de88edaa3b95ea480a/tokio/src/process/unix/reap.rs

wez commented

Oh yeah, SIGCHLD is objectively horrible. When it comes to ptys I think wrapping it up in something like spawn_blocking + the underlying synchronous wait is probably fine, assuming that 1000's of ptys and waits don't need to happen concurrently.

sunshowers commented

It'll be typically 10s of ptys, not thousands thankfully.