search

rogpeppe / go-internal

Selected Go-internal packages factored out from the standard library
BSD 3-Clause "New" or "Revised" License
825 stars 67 forks source link

testscript: add pty/ptyout commands #172

Closed FiloSottile closed 10 months ago

FiloSottile commented 1 year ago

This allows testing commands that open /dev/tty to interact with the user separately from stdin/stdout, like age.

See https://github.com/FiloSottile/age/blob/084c974f5393e5d2776fb1bb3a35eeed271a32fa/cmd/age/testdata/terminal.txt and https://github.com/FiloSottile/age/blob/084c974f5393e5d2776fb1bb3a35eeed271a32fa/cmd/age/testdata/scrypt.txt for example scripts.

mvdan commented 1 year ago

I'm slightly worried about adding a hard dependency on creack/pty, which is not a small library. I understand that testscript is for testing CLIs and this is an important feature to support, but I imagine it would be nicer to allow plugging in any PTY library. That would be more flexible long-term, and would keep the amount of dependencies small for the majority of users who don't need this feature.

FiloSottile commented 1 year ago

Pluggable PTY libraries sounds complex to me, and they have a relatively simple job: opening a pair of file descriptors that work as a virtual terminal. If that's the only blocker, I can probably reimplement the ~40% of creack/pty we need, especially if you're only targeting macOS and Linux.

mvdan commented 1 year ago

I have historically only used creack/pty on unix-y systems, but I'm not sure if trying to use it on Windows makes sense :)

To be clear, what I mean by pluggable is to keep your patch as-is, but abstract away the Open, Write, and Close calls to an interface that's satisfied by the library. That doesn't sound particularly complex to me, and gives the downstream the ability to choose which libraries to incorporate. As you can see we've been very careful about adding dependencies, and we're even trying to get rid of pkg/diff soon. The only dependency I think we'll want to add in the near term is x/tools, for the sake of forwarding now-redundant packages like txtar.

mvdan commented 1 year ago

Also: perhaps @rogpeppe or @myitcv disagree with me

FiloSottile commented 1 year ago

Rebased and removed the creack/pty dependency. Opening a PTY pair is really just a OpenFile and a couple IOCTLs.

FiloSottile commented 1 year ago

Test added! Testing -stdin would require a dependency on x/term or more IOCTLs and it didn't feel worth it.

FiloSottile commented 1 year ago

Sweet, thank you for the reviews! Resolved the conflict, should be ready to merge.

mvdan commented 1 year ago

@FiloSottile you got a 10m pty testscript timeout on macos on 1.20 there, any idea what might have happened? it succeeded on macos on 1.19, so perhaps it's a race or flakiness of some sort.

FiloSottile commented 11 months ago

@FiloSottile you got a 10m pty testscript timeout on macos on 1.20 there, any idea what might have happened? it succeeded on macos on 1.19, so perhaps it's a race or flakiness of some sort.

Turns out it's a os Go 1.20 regression. golang/go#61779

FiloSottile commented 10 months ago

Rebased and added a skip for https://github.com/golang/go/issues/61779.

mvdan commented 10 months ago

Thanks!