testscript: add pty/ptyout commands #172
Conversation
|
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. |
|
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. |
|
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 |
FiloSottile
force-pushed
the
filippo/pty
branch
from
9f274cd
to
4471c00
Compare
|
Rebased and removed the creack/pty dependency. Opening a PTY pair is really just a OpenFile and a couple IOCTLs. |
FiloSottile
force-pushed
the
filippo/pty
branch
from
4471c00
to
d43ebe7
Compare
There was a problem hiding this comment.
This looks reasonable to me - it just needs a test, which would perhaps double as an example of how it can be used.
cc @rogpeppe for thoughts. My main objection was the added dependency, but 200 lines of code isn't particularly worrisome given that this feature is likely to be useful to some testscript users.
FiloSottile
force-pushed
the
filippo/pty
branch
from
4fe53bb
to
bb03dcf
Compare
|
Test added! Testing -stdin would require a dependency on x/term or more IOCTLs and it didn't feel worth it. |
There was a problem hiding this comment.
I have quite mixed feelings about this PR (I started reviewing it ages ago and didn't complete the review because of that). On the one hand, it seems like it's definitely useful in some circumstances. On the other, it introduces a bunch of very system-dependent code that feels a bit out of place here. It also feels like a bit of a slippery slope: once we've got the ability to talk to a terminal, why not testing of arbitrary interactions? It does seem like the primitives provided here aren't quite enough to make this that useful without some more customisation (for example, tty output will commonly have control characters such as cr-lf sequences, but txtar files aren't well set up for making that clear so that cmp will work OK, for example).
On balance, I just about fall across the line of "OK", but it's a close-run thing!
I wonder about moving the pty-specific stuff into an internal/pty package so that the system-specific stuff is more cleanly delineated.
Also, this definitely needs a test.
testscript/testscript.go
Outdated
| <-doneR | ||
| <-doneW | ||
| ts.ptyin = "" | ||
| ts.ptyout = ptyBuf.String() |
There was a problem hiding this comment.
I don't understand how this isn't racy - it doesn't synchronize with anything after setting this field, so I don't see how a happens-before relationship can be formed between this write and the read that happens to check its value.
There was a problem hiding this comment.
Maybe I'm missing some feature, but I think there's simply no concurrency between the exec command and the ttyout/cmp/... commands?
FiloSottile
force-pushed
the
filippo/pty
branch
from
bb03dcf
to
4a655cc
Compare
There was a problem hiding this comment.
Thank you for the review and sorry for the lag, I had somehow missed it and only saw it as I was checking in yesterday.
It does seem like the primitives provided here aren't quite enough to make this that useful without some more customisation (for example, tty output will commonly have control characters such as cr-lf sequences, but txtar files aren't well set up for making that clear so that cmp will work OK, for example).
FWIW I am using this as-is in age. I mostly need to check that the logic for using the terminal is correct, so ttyout patterns work for that.
testscript/testscript.go
Outdated
| <-doneR | ||
| <-doneW | ||
| ts.ptyin = "" | ||
| ts.ptyout = ptyBuf.String() |
There was a problem hiding this comment.
Maybe I'm missing some feature, but I think there's simply no concurrency between the exec command and the ttyout/cmp/... commands?
FiloSottile
force-pushed
the
filippo/pty
branch
3 times, most recently
from
8b3284c
to
d6f5598
Compare
There was a problem hiding this comment.
LGTM; thanks for bearing with me all this time. I still don't like all the crap this adds, and the general poor usability (why can we do cp stdout foo but not cp ttyout foo for example?), but I don't see much alternative, and we can hopefully improve things in the future without breaking compatibility.
There was a problem hiding this comment.
LGTM; thanks for bearing with me all this time. I still don't like all the crap this adds, and the general poor usability (why can we do cp stdout foo but not cp ttyout foo for example?), but I don't see much alternative, and we can hopefully improve things in the future without breaking compatibility.
|
Sweet, thank you for the reviews! Resolved the conflict, should be ready to merge. |
|
@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 |
FiloSottile
force-pushed
the
filippo/pty
branch
from
4ae4aa5
to
94b0f0d
Compare
|
Rebased and added a skip for golang/go#61779. |
|
Thanks! |
This allows testing commands that open
/dev/ttyto 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.