vhs icon indicating copy to clipboard operation
vhs copied to clipboard

feat: add Set DisableRender option for testing.

Open PThorpe92 opened this issue 1 year ago • 7 comments

Flag for disabling rendering #363

This PR adds a "SET" option

set DisableRender <bool>

That does the obvious :smile: and keeps it from executing the ffmpeg command so just .txt output can be dealt more easily for our CI/CD testing plans over at EZA as can be seen at #147

This was my first time looking at the code base, so please let me know if I missed or overlooked something. Also please let me know if/where you would like tests added for this behavior. All I added was to the default token/parser tests, and I couldn't figure out exactly where a general test for this would go, so please do let me know and I'll add it. Everything I have tried with Set DisableRender true and with output being .txt has worked perfectly.

Just want to say thanks for a great program, and everything you guys do is awesome :rocket: :rocket:

EDIT: Just ran into this again and figured I'd fix the conflicts.

PThorpe92 avatar Aug 20 '23 13:08 PThorpe92

I have a similar use case. Can we have this please @maaslalani? 😃

mbechto avatar Nov 28 '23 15:11 mbechto

I have a similar use case. Can we have this please ? 😃

Assuming you have the same use case (testing the output of CLI program), You might like to know that we found diffing the txt output to be less than deterministic unfortunately :unamused:

We ended up going with trycmd and are working on a program to generate the test cases.

What kind of output are you looking to test? It would prob work for something fairly simple but eza output got complex to test this way pretty quick

PThorpe92 avatar Nov 28 '23 16:11 PThorpe92

Yes, I can confirm that depending on the machine running the tests, the results can vary. Also, if you want to make it as reproducible as possible, I found fixating the window size in columns and rows is a good idea.

In my case, I worked around both limitations using tmux to make screen captures:

Type "tmux"
Enter
Type "tmux resize-window -x 110 -y 24"
Enter
Type "tmux bind -n C-p 'capture-pane; save-buffer -a out.txt; delete-buffer'"
Enter

In each following test case, I use the Ctrl+p screen capture shortcut bound in tmux to make a combined (therefore -a append) out.txt with all screens. This is a little wacky, but it works well enough most of the time.

I had a brief look at the tools you mentioned, but I had the impression it could not be used to test an interactive TUI, that renders a lot of screens in unicode and simulating user input. I would be interested to know how do you handle this.

EDIT: Setting the window size in pixels won't work reliably. I had problems to reproduce the results on different machines (especially GitHub runners).

mbechto avatar Nov 28 '23 19:11 mbechto

Ahh yeah a TUI is a little more difficult eza is a CLI app meant to serve as a replacement for LS so it is quite a bit easier than a full blown interactive TUI..

Thank you for the tip though, one of my other projects is a TUI and I was trying to figure out how best to test the UI. Hopefully they merge this, although it's been so long now that I might have to go back and take a peek to make sure everything still works but I also don't think they have merged many commits since then either.

At one point I was going to work on building and maintaining a fork of VHS that only serves to test the output of TUI applications, when we found tryCMD for eza, I changed directions but now that I am in need of testing for a TUI I might revisit that idea.

PThorpe92 avatar Nov 28 '23 19:11 PThorpe92

If you plan to write a test tool suitable for TUIs feel free to hit me up. I like to code during my weekly train rides and would be happy to collaborate on anything Rust.

mbechto avatar Nov 28 '23 20:11 mbechto

Hey @PThorpe92, thanks for this PR, I think the concept makes sense to have but I would like to avoid increasing the surface area of the language by adding a new keyword for this feature. What do you think of simply modifying the Output to recognize an argument such as "none" or "/dev/null" or something along these lines:

Output none
Output "/dev/null" # perhaps?

maaslalani avatar Mar 15 '24 14:03 maaslalani

Yeah that makes sense to me, I'll make the adjustments this weekend :+1:

EDIT: now I remember why I didn't do that the first time. That leaves nowhere for the user to specify their output.txt file.

PThorpe92 avatar Mar 21 '24 11:03 PThorpe92

Hey @PThorpe92, thanks for the awesome PR, we really appreciate it.

I think disabling renders makes sense to me. However, the approach I'd like to take is that if the Output ascii.txt is specified then we don't render a GIF. That way the user would then have to also specify another Output out.gif to actually render the GIF.

I will close this PR for now (just doing some spring cleaning 😄) but feel free to open a new PR if this feature is still needed with the mutually exclusive Output version.

maaslalani avatar Jun 18 '24 16:06 maaslalani