feat: add Wait to wait for expected output #257

mastercactapus · 2023-03-12T15:00:55Z

This PR adds two new commands:

MatchLine that takes a regular expression and waits until the current line matches
MatchScreen that takes a regular expression and waits until the "screen" matches

The purpose is to allow waiting for things like network or serial devices that might have more variable delays while using VHS.

MatchLine example

Output "out.gif"

Type "sudo apt update" Enter
MatchLine "password"

Type "hunter2" Enter # fake password :D
MatchLine "^>"

Type "sudo apt dist-upgrade" Enter
MatchLine "^>"

Sleep 1

MatchScreen example

Output "out.gif"

Set Width 1280
Set Height 720

Type "telnet telehack.com" Enter
MatchScreen "command list"

Type "starwars" Enter
MatchScreen "A long time ago"

Ctrl+C
MatchLine "^\."

Type "exit" Enter
MatchLine "^>"

Sleep 1

Additional Info

A few string(s[0]) + strings.ToLower(s[1:]) places were changed to a toCamel helper with unit test (fixes issue with MUTLI_WORD commands formatting)
A .Buffer() method was added to *VHS to allow getting the current buffer as text (screen)
A .CurrentLine() method was added to *VHS to allow getting the current line as text
.SaveOutput() was updated to use the shared method (largely the same code as before)
Possibly closes Wait for command to finish before continuing #70

rbergmanaf · 2023-05-16T19:29:04Z

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?

mastercactapus · 2023-05-24T13:44:59Z

@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 @ like Type (e.g., MatchLine@1s), but it doesn't read well.

Alternatively, since it is a waiting command, we could just make it mandatory.

rbergmanaf · 2023-05-24T14:08:35Z

I don't feel strongly, but do like the idea of a short default. @ would probably be the least surprising given the precedent in Type, but as long as it's documented I imagine either way being acceptable personally. Maybe the charm team would have a preference?

maaslalani · 2023-05-24T15:27:43Z

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 Wait (possibly WaitFor / WaitUntil) so it reads a bit like english commands:

Type 'echo "Hello"'
Enter
Wait /hello/ 5s

maaslalani

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.

maaslalani · 2023-05-24T15:28:29Z

token.go

 	}
 	return string(t)
 }

+func toCamel(s string) string {


Thanks for this extraction ❤️

maaslalani · 2023-05-24T15:29:07Z

testing.go

+
+// 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()")


Nice, this is great! 👌

maaslalani

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.

rbergmanaf · 2023-05-24T19:58:04Z

A simplified command like Wait that just waits for the next prompt to appear without needing an explicit expression for that one case does seem really convenient. It's the main way we are using it at this point, though we have some other cases that benefit from also having the ability to customize the regex.

mastercactapus · 2023-05-24T22:05:18Z

I like how Wait reads as well as implies what it does -- waits for a thing. Follow the what rather than the how.

I think WaitForLine is pretty clear, not sure if WaitForScreen reads as well, though. What about WaitOnScreen?

As for the common case of waiting for prompts, we could potentially do a Wait variation that uses WaitForLine but with a Set-able PROMPT variable or something like that with a sane default. Line & Screen could require a pattern, and Wait would not accept one to prevent the overlap.

maaslalani · 2023-05-24T22:12:59Z

I would propose something like the following:

Wait[+Context][@timeout] [regex]

Wait for prompt

Wait # simply wait for the prompt to appear (default timeout of 5 seconds)

Wait with timeout.

Wait@3m # wait for a maximum of 3 minutes for the prompt to appear

Wait with regex, we can decided whether the default is a screen or line.

Wait /hello/ # wait for hello to appear

Wait with context

Wait+Screen /hello/ # wait for hello to appear on the screen.
Wait+Line /hello/ # wait for hello to appear on the line.

Wait with all options:

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?

mastercactapus · 2023-05-24T22:21:32Z

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

maaslalani · 2023-05-24T22:23:27Z

As for defaulting, I think we should set the defaults for our "wait for prompt" use case.

Yup, sounds good!

Context is a nice idea, is that used elsewhere or would this be a new syntax?

This + syntax is used with the Ctrl and Alt commands, i.e.:

Ctrl+L
Alt+Enter

In terms of implementation, the Command would be Wait and the context would be the Options.

mastercactapus · 2023-05-24T22:30:43Z

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 readString and pass / as the endChar with a new token type REGEX to keep things clear.

Does all sound good?

maaslalani · 2023-05-25T01:06:45Z

The new REGEX token sounds great!

mastercactapus · 2023-05-25T01:09:05Z

Excellent, I'll switch this PR to a draft until I'm able to get those changes pushed up for review

joshmarinacci · 2024-03-14T21:40:12Z

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!

b-per · 2024-03-27T19:42:04Z

As this PR hasn't been closed yet and it is now conflicting with main, I spent a bit of time today to fork the repo and merge the changes from mastercactapus and justsh .

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 Wait feature, you can get the complied binary from the releases in my fork here or can also use eget with eget b-per/vhs to download the relevant binary for your machine.

ocervell · 2024-04-04T08:21:23Z

@b-per I've got an error on your fork:

panic: timeout waiting for "Line" to match >$; last value was: 

goroutine 1 [running]:
main.ExecuteWait({{0x1031214, 0x4}, {0x0, 0x0}, {0x10312b8, 0x4}}, 0xc0002b0000)
        /home/runner/work/vhs/vhs/command.go:159 +0x4b8
main.Execute({{0x1031214, 0x4}, {0x0, 0x0}, {0x10312b8, 0x4}}, 0xc0002b0000)
        /home/runner/work/vhs/vhs/command.go:27 +0xae
main.Evaluate({0x150af18, 0xc000463cc0}, {0xc0001ce000, 0x1e0}, {0x15050c0, 0xc000090028}, {0xc0006d1b78, 0x1, 0x0?})
        /home/runner/work/vhs/vhs/evaluator.go:143 +0xd53
main.glob..func2(0x1bff500, {0xc00047fb20, 0x1, 0x1031244?})
        /home/runner/work/vhs/vhs/main.go:99 +0x44e
github.com/spf13/cobra.(*Command).execute(0x1bff500, {0xc0000ac010, 0x1, 0x1})
        /home/runner/go/pkg/mod/github.com/spf13/[email protected]/command.go:983 +0xabc
github.com/spf13/cobra.(*Command).ExecuteC(0x1bff500)
        /home/runner/go/pkg/mod/github.com/spf13/[email protected]/command.go:1115 +0x3ff
github.com/spf13/cobra.(*Command).Execute(...)
        /home/runner/go/pkg/mod/github.com/spf13/[email protected]/command.go:1039
github.com/spf13/cobra.(*Command).ExecuteContext(...)
        /home/runner/go/pkg/mod/github.com/spf13/[email protected]/command.go:1032
main.main()
        /home/runner/work/vhs/vhs/main.go:250 +0xc9

My tape file:

# Where should we write the GIF?
Output demo.gif
Set Shell fish

# Set up a 1200x600 terminal with 46px font.
Set FontSize 25
Set Width 1500
Set Height 1000
Set BorderRadius 10

Type "secator x httpx testphp.vulnweb.com"
Sleep 500ms
Enter
Wait

Edit: nevermind, had to increase the WaitTimeout:

Set WaitPattern />$/
Set WaitTimeout 2m

Is there a way to just wait indefinitely ?

b-per · 2024-04-04T11:34:39Z

Yes, the panic behavior was discussed in this comment as well. (this might be why it wasn't merged at that time)

ocervell · 2024-04-05T08:03:32Z

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.

spkane · 2024-05-15T22:36:01Z

Are there any updates on the status of this work? What are the next steps that are needed?

@mastercactapus @maaslalani

mastercactapus · 2024-05-18T16:07:07Z

@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

spkane · 2024-05-20T17:44:31Z

@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.

maaslalani · 2024-05-31T19:14:30Z

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.

Assume the VHS tape didn't work as intended. Stop immediately.

Assume a timeout is a viable path, continue performing the actions in the tape.

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:
Timeout waiting for "Wait+Line hi" to match hi

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.

fenio · 2024-06-01T14:44:51Z

So what's next? Is this ready to be merged?
It really looks like MUST HAVE feature ;)

maaslalani · 2024-06-01T17:58:21Z

So what's next? Is this ready to be merged?

It really looks like MUST HAVE feature ;)

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!

fenio · 2024-06-02T04:11:16Z

Sure I'm happy to test it but it will be much easier with having some snapshot binary available to do so.
Currently I'm simply using brew install vhs and I don't really want to break my working setup.

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.

fenio · 2024-06-05T17:54:16Z

So no chance to have some freely available build which can be installed next to official vhs release? :)

maaslalani · 2024-06-05T18:23:41Z

Hey @fenio, sorry for the delayed response. To test out this branch you would do something like:

brew install go

export GO_HOME=$HOME/.config/go
export GO_BIN=$HOME/.config/go/bin
go install github.com/mastercactapus/vhs@match

and then run vhs from the GO_BIN or add it to the $PATH.

$HOME/.config/go/bin/vhs wait.tape

fenio · 2024-06-06T06:55:14Z

@maaslalani thanks for sharing info how to build it but it fails this way with:

❯ ~/demo go install github.com/mastercactapus/vhs@match
go: downloading github.com/mastercactapus/vhs v0.0.0-20240531212618-997662d08c11
go: github.com/mastercactapus/vhs@match: version constraints conflict:
	github.com/mastercactapus/[email protected]: parsing go.mod:
	module declares its path as: github.com/charmbracelet/vhs
	        but was required as: github.com/mastercactapus/vhs

fenio · 2024-06-06T07:35:39Z

@maaslalani
Ok nevermind. I've just cloned your repo and built vhs locally.
Works lovely!

I just had to set WaitTimeout to 30s cause with default it was not enough.
Other than that it works perfectly fine and my demo is now 41s long instead of 49s before and I no longer have to blindly guess sleep values.

I LOVE IT ;)

fenio · 2024-06-06T10:22:06Z

It's just 8s shorter but (not sure why) size dropped from around 1,05MB to 350kB which is significant amount.
For my other demo time dropped from 50s to 45s and size from 387kB to 360kB.
Lovely results.
Final demos can be seen here: https://github.com/fenio/pv-mounter

Please merge it as I already updated my tape files ;)

maaslalani · 2024-06-06T15:30:10Z

Yeah the 5s wait timeout also caught me off guard. I think it should probably be either 15 or 30 seconds by default.

fenio · 2024-06-06T15:48:13Z

Definitely. If someone tries to find something that will allow to work with commands that finish in unpredictable time then it's probably way more than 5 seconds ;)
I'd say 30s as default should fit most of the use cases.
And maybe a bit more clear statement that it timed out. I didn't write down how exactly it fails in case it exceeds timeout but it definitely wasn't clearly communicated and probably should have been.
Anyway even with default set to 0.1s it's still something I want to be included in main branch ;)
This is just killer feature.

mastercactapus · 2024-06-06T18:32:52Z

I updated the default to 15s for now, though I'm happy to adjust it if it should be higher. I expect use cases to vary a lot, so I'm not sure of the best value.

fenio · 2024-06-19T16:00:54Z

So how about merge? What are the blockers for it?

3v1n0 · 2024-06-19T22:50:32Z

Maybe documentation should be updated too?

mastercactapus added 6 commits November 26, 2022 15:15

add Match and MatchAny commands

bb16d08

allow changing Match value

476a839

switch to MatchLine and MatchScreen

4a41562

Merge branch 'main' into match

b741ff8

remove leftover code

78c0e37

Merge remote-tracking branch 'origin/main' into match

4d1b73d

Merge remote-tracking branch 'origin/main' into match

53ed73a

maaslalani reviewed May 24, 2023

View reviewed changes

mastercactapus marked this pull request as draft May 25, 2023 01:09

mastercactapus added 8 commits May 24, 2023 20:17

add wait and regex to lexer

7ae1fbb

add parseWait

4213ef3

Add wait option defaults

ffb4024

add execute for wait

d6216c2

allow setting WAitTimeout and WaitPattern

73a4c20

remove MATCH_LINE and MATCH_SCREEN

8d0302f

add description to Buffer method

5b60921

add wait to parser test and fix setting parsing

944d605

maaslalani changed the title ~~feat: add MatchLine and MatchScreen to wait for expected output~~ feat: add Wait to wait for expected output Mar 14, 2024

This was referenced Mar 27, 2024

Merge main with the Wait PR #461

Closed

Merge main wait b-per/vhs#1

Merged

Merge branch 'main' into match

c9836db

mastercactapus mentioned this pull request May 18, 2024

Command err handling #480

Merged

mastercactapus added 2 commits May 31, 2024 16:23

Merge remote-tracking branch 'upstream/main' into match

28a3c13

update signatures with errors

997662d

set default wait timeout to 15s

adaeead

feat: add Wait to wait for expected output #257

Are you sure you want to change the base?

feat: add Wait to wait for expected output #257

Conversation

mastercactapus commented Mar 12, 2023

rbergmanaf commented May 16, 2023

mastercactapus commented May 24, 2023 • edited Loading

rbergmanaf commented May 24, 2023

maaslalani commented May 24, 2023

maaslalani left a comment

Choose a reason for hiding this comment

maaslalani May 24, 2023

Choose a reason for hiding this comment

maaslalani May 24, 2023

Choose a reason for hiding this comment

maaslalani left a comment

Choose a reason for hiding this comment

rbergmanaf commented May 24, 2023

mastercactapus commented May 24, 2023

maaslalani commented May 24, 2023 • edited Loading

mastercactapus commented May 24, 2023 • edited by maaslalani Loading

maaslalani commented May 24, 2023 • edited Loading

mastercactapus commented May 24, 2023

maaslalani commented May 25, 2023

mastercactapus commented May 25, 2023

joshmarinacci commented Mar 14, 2024

b-per commented Mar 27, 2024

ocervell commented Apr 4, 2024 • edited Loading

b-per commented Apr 4, 2024

ocervell commented Apr 5, 2024

spkane commented May 15, 2024

mastercactapus commented May 18, 2024

spkane commented May 20, 2024

maaslalani commented May 31, 2024

fenio commented Jun 1, 2024

maaslalani commented Jun 1, 2024

fenio commented Jun 2, 2024

fenio commented Jun 5, 2024

maaslalani commented Jun 5, 2024

fenio commented Jun 6, 2024

fenio commented Jun 6, 2024

fenio commented Jun 6, 2024

maaslalani commented Jun 6, 2024

fenio commented Jun 6, 2024

mastercactapus commented Jun 6, 2024

fenio commented Jun 19, 2024

3v1n0 commented Jun 19, 2024

mastercactapus commented May 24, 2023 •

edited

Loading

maaslalani commented May 24, 2023 •

edited

Loading

mastercactapus commented May 24, 2023 •

edited by maaslalani

Loading

maaslalani commented May 24, 2023 •

edited

Loading

ocervell commented Apr 4, 2024 •

edited

Loading