Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add snapshot test using insta #2411

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

cruessler
Copy link
Contributor

@cruessler cruessler commented Oct 29, 2024

@extrawurst This PR is another try at adding snapshot tests to gitui. It doesn’t render to SVG as some of the other approaches do, but it works with the existing App and draw and only requires a minor type change in order to get it to work. Also, the text-based snapshot is still reasonably easy to read. So this feels very promising.

Next, I want to try to write a test that has interactivity, be it loading data from an actual git repo or keyboard input from a user.

@cruessler cruessler marked this pull request as ready for review November 1, 2024 10:24
@cruessler
Copy link
Contributor Author

@extrawurst I just marked this PR as ready for review! I found it way easier to create the test than I had anticipated, mostly because the application is already structured in a way that is very amenable to snapshot testing.

Known issues:

  • I had to add sleep after sending events to the app, and I found that the test on Windows failed before I found a value that didn’t fail. 500 most likely is way too high. I don’t exactly know what the best approach here is. In an ideal world, we would have something like wait_for which we could use to wait for specific events that would indicate that the app has finished loading or gotten a specific kind of async result.
  • We might want to add an additional layer between App and the test as the test currently requires a bit of setup that in turn requires certain knowledge about how App works. By abstracting that away, writing tests might get even easier. Another option is to move the existing setup or parts of it into App.

@cruessler
Copy link
Contributor Author

@extrawurst Did you get a chance to have a look? 😄

@extrawurst
Copy link
Owner

@cruessler ok i think this is a great prototype but i don't think its ready to be merged in this form.

  1. we should definitely make sure we can busyloop until a certain event happened (with a timeout) to make the test more stable.
  2. i am not a fan of it all living in main.rs what you pointed out would allow us to put this into a testing suite: putting what we have now in main on top of App into another layer, lets call it simply Gitui that we can then use in our tests with a proper public api.

@cruessler
Copy link
Contributor Author

@extrawurst I’ve extracted app initialization as well as the main loop into a new struct, Gitui. main.rs is now a lot lighter, with the code and tests living in gitui.rs.

I’m not sure about how to best implement a busy loop, though. We might be able to send the event that we get here, through a channel to the caller. Then, we could, in the caller, have a recv_timeout that we could use to wait for specific events. Does that sound like a possible solution? (I’m only vaguely familiar with crossbeam, so I’m happy about any context that you might be able to share.)

@extrawurst
Copy link
Owner

@cruessler awesome!

through a channel to the caller

yeah lets do that.
and please fix the clippy lints

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants