feat: add Wait to wait for expected output #257
Conversation
|
I've been experimenting with this and it's a useful feature, thanks for creating this! I've noticed though that it would benefit from a timeout option in the event the expected pattern isn't encountered? |
|
@rbergmanaf Yeah, I could see using that; should it be optional or required? If optional, should it have a default? It seems odd to have something hang forever by default. Something short, like 3 seconds, and then you can manually raise it for long commands. If the default is shorter, then it's more likely a user will put their value in for longer-running commands -- the last thing we want is a script that takes a long time failing halfway through because the user didn't realize there was a 30-second timeout or something. MatchLine <regex> [timeout]I considered using Alternatively, since it is a waiting command, we could just make it mandatory. |
|
I don't feel strongly, but do like the idea of a short default. |
|
I agree on the time out being a great option, I agree on the default of 3 seconds as well having the user manually bump it if they know it will be longer. Additionally, I would probably have this renamed to |
There was a problem hiding this comment.
This is great! I'm really excited about this feature, just a bit concerned about the naming. I don't know if MatchLine and MatchScreen are the best names.
I also think waiting for the prompt i.e. MatchLine "^>" will be a very popular incantation of the command. I wonder if it's worth having a Wait command that simply Sleeps until the prompt shows up.
token.go
Outdated
| } | ||
| return string(t) | ||
| } | ||
|
|
||
| func toCamel(s string) string { |
There was a problem hiding this comment.
Thanks for this extraction ❤️
|
|
||
| // CurrentLine returns the current line from the buffer. | ||
| func (v *VHS) CurrentLine() (string, error) { | ||
| buf, err := v.Page.Eval("() => term.buffer.active.getLine(term.buffer.active.cursorY+term.buffer.active.viewportY).translateToString().trimEnd()") |
There was a problem hiding this comment.
This is great! I'm really excited about this feature, just a bit concerned about the naming. I don't know if MatchLine and MatchScreen are the best names.
I also think waiting for the prompt i.e. MatchLine "^>" will be a very popular incantation of the command. I wonder if it's worth having a Wait command that simply Sleeps until the prompt shows up.
|
A simplified command like |
|
I like how I think As for the common case of waiting for prompts, we could potentially do a |
|
I would propose something like the following: Wait[+Context][@timeout] [regex]
Wait # simply wait for the prompt to appear (default timeout of 5 seconds)
Wait@3m # wait for a maximum of 3 minutes for the prompt to appear
Wait /hello/ # wait for hello to appear
Wait+Screen /hello/ # wait for hello to appear on the screen.
Wait+Line /hello/ # wait for hello to appear on the line.
Wait+Screen@1m /hello/What are your thoughts? I think this syntax is nicer since it limits the commands but I'm very open to having different API that is potentially nicer? |
|
Context is a nice idea, is that used elsewhere or would this be a new syntax? As for defaulting, I think we should set the defaults for our "wait for prompt" use case. Set WaitPattern />$/ # default wait pattern
Set WaitTimeout 5s # default wait timeout
Wait # can be used in most cases to wait for the next prompt
Wait+Line # identical to the above
Wait@1m # for longer-running commands
Wait /custom-pattern/
Wait+Screen /dialog title/ # match anywhere on the screen |
Yup, sounds good!
This Ctrl+L
Alt+EnterIn terms of implementation, the |
|
Okay, that all sounds reasonable, and thanks for the insight on context. The last bit to clarify is regex parsing -- we can probably piggyback on Does all sound good? |
|
The new REGEX token sounds great! |
|
Excellent, I'll switch this PR to a draft until I'm able to get those changes pushed up for review |
|
@maaslalani ready for another look |
|
@mastercactapus Thanks for your patience on this PR. I will take a look and get this merged in soon! |
|
Hey @mastercactapus, I'm running into a timeout panic when a match is not detected: 
I think we shouldn't panic here but rather do one of two things.
I'm leaning towards option one, current the panic does do that but we should output a nicer message, similar to what it's currently showing without the stack trace: |
|
For option 2, timeout should not be considered a viable path IMO; it would be dangerous to keep executing commands in an unknown state (e.g., To do that/option 1: If there's already a built-in way to stop an error, I'm happy to make the update. |
|
^ put another way, the panic was intentional to be a "stop with error" for functions that don't return an error |
|
That makes absolute sense! We should error out if a timeout occurs. Yes, we'll likely need a way to bubble up and stop execution of the VHS program in this case. Perhaps we can add a method to mark the VHS instance as failed which would exit or cancel the context that we use when the user press ctrl+c |
|
@denisonbarbosa we should definitely use this feature in our tapes once it will land! |
|
I've been testing this branch for some CI/CD pipelines and it's worked very well for most of the jobs I've tested it on! As a simulation of a command in a CI pipeline that generates a lot of line-based output, consider the following tape which enumerates words in the local This tape completes successfully, and the video correctly shows the last page of the dictionary followed by a new prompt line. However, if I try to match the last line of output to determine success or failure, This fails with the following panic: I noticed that the last screen of output mentioned in the panic stops short of the last page of the dictionary by about 1000 words, which feels similar to a buffer not being flushed somewhere. $ nl < /usr/share/dict/words | grep -E 'zinkenite|ZZZ' | tee /dev/tty | cut -f1 | sort -nr | paste -sd- | xargs -t -L1 | bc
478827 zinkenite
479826 ZZZ
echo 479826-478827
999Noting that the Specifically, Should diff --git a/testing.go b/testing.go
index 8524684..e468e6b 100644
--- a/testing.go
+++ b/testing.go
@@ -55,7 +55,7 @@ func (v *VHS) SaveOutput() {
// Buffer returns the current buffer.
func (v *VHS) Buffer() ([]string, error) {
// Get the current buffer.
- buf, err := v.Page.Eval("() => Array(term.rows).fill(0).map((e, i) => term.buffer.active.getLine(i).translateToString().trimEnd())")
+ buf, err := v.Page.Eval("() => Array(term.rows).fill(0).map((e, i) => term.buffer.active.getLine(term.buffer.active.viewportY+i).translateToString().trimEnd())")
if err != nil {
return nil, fmt.Errorf("read buffer: %w", err)
}After making the above change locally and re-running the tape with |
maaslalani
changed the title
|
I've just started using VHS and this feature is the one thing I need before adopting it. I have some commands that take varying amounts of time to run depending on the network. I'd really like to say: Wait until you see text "XYZ" instead of Sleep. Thank you! |
|
As this PR hasn't been closed yet and it is now conflicting with I also set a GH pipeline to build the relevant binaries. So, until this PR is part of this repo, If anyone is interested in the |
|
@b-per I've got an error on your fork: My tape file: Edit: nevermind, had to increase the WaitTimeout: Is there a way to just wait indefinitely ? |
|
Yes, the panic behavior was discussed in this comment as well. (this might be why it wasn't merged at that time) |
|
Other than that which happens only when the timeout is reached so kinda makes sense. Other than that, it's worked really well for me once I set a proper (longer) timeout. |
|
Are there any updates on the status of this work? What are the next steps that are needed? |
|
@spkane I updated and resolved the conflicts, the main remaining issue is that there's no good way to have a command return an error (i.e., other than panic), which is needed because WAIT introduces the need for something to fail/timeout possibly. I'll look at opening a separate PR to allow command funcs to return an error, as I have a bit of time this AM |
Thanks, @mastercactapus! I definitely want to test this logic out, as I often create demos where I am waiting for events to occur. I frequently don't know how long the command will take to complete, so currently, I have to do a bunch of testing and guesstimate how long to sleep and then hope that it works out for the given run. |
We just merged #480 which allows us now to show errors during execution of tapes. This will make showing the timeout errors much nicer than panics! Thanks for the incredible work @mastercactapus. |
|
So what's next? Is this ready to be merged? |
Yes, just need to do some testing now and then will merge! Please feel free to try this branch out as well, feedback from others is always welcome too! |
|
Sure I'm happy to test it but it will be much easier with having some snapshot binary available to do so. But since I'm recording sessions with kubernetes which means output time is sometimes comletely unpredictable I'm really interested with feature "proceed when prompt is back". BTW GIFs produced by vhs are crashing Github mobile app but that's their problemand already submitted. |
This PR adds two new commands:
MatchLinethat takes a regular expression and waits until the current line matchesMatchScreenthat takes a regular expression and waits until the "screen" matchesThe purpose is to allow waiting for things like network or serial devices that might have more variable delays while using VHS.
MatchLineexampleMatchScreenexampleAdditional Info
string(s[0]) + strings.ToLower(s[1:])places were changed to atoCamelhelper with unit test (fixes issue withMUTLI_WORDcommands formatting).Buffer()method was added to*VHSto allow getting the current buffer as text (screen).CurrentLine()method was added to*VHSto allow getting the current line as text.SaveOutput()was updated to use the shared method (largely the same code as before)