Add snapshot test using insta

[cruessler]

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

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

@extrawurst Did you get a chance to have a look? :smile:

[extrawurst]

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

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

@cruessler awesome!

through a channel to the caller

yeah lets do that. and please fix the clippy lints

[cruessler]

@extrawurst Just a little status update: I think I’ll get to this PR this week.

[cruessler]

@extrawurst It turned out that removing sleep was easier than I thought! The test now has all instances of sleep removed. I quite like the result, even though writing a snapshot test requires a certain amount of knowledge with respect to how the app works internally, so we might want to add another abstraction on top of what’s already there.

What do you think?

[extrawurst]

@cruessler i love it! is there a way to control the size of the test buffer? I would prefer to have tests only use the minimum required buffer size for the purpose of the test to still make sense

[cruessler]

Do you mean the size of the test terminal? Yes, that is completely configurable.

https://github.com/cruessler/gitui/blob/b1ce826faf455239df610a1a68e7ab090cd4fec4/src/gitui.rs#L241

If you want to find a good size yourself, feel free to add a commit to this PR. Instructions for running cargo insta: https://insta.rs/docs/quickstart/#reviewing-snapshots.

One thing I forgot to mention: the busy loop in wait_for_async_git_notification doesn’t have a timeout currently, so a test can potentially get stuck in the loop and not finish. The way it is written right now enables skipping AsyncGitNotifications until the expected one is received, but it only has a timeout on the individual recv_timeout calls, not on the loop.

[extrawurst]

@cruessler

potentially get stuck in the loop and not finish

in what scenario could that happen?

size of the test terminal?

exactly. i wonder what our rule for this should be, maybe "smallest size needed to reflect whats tested" maybe?

[cruessler]

@extrawurst

in what scenario could that happen?

If the application keeps sending AsyncGitNotifications, but not the ones we’re waiting for, the loop could go on for quite some time. I don’t know how likely this is to happen in practice, though, so maybe it’s not that big a deal since recv_timeout will panic when there’s no message as far as I remember.

exactly. i wonder what our rule for this should be, maybe "smallest size needed to reflect whats tested" maybe?

That sounds like a good idea! I wasn’t sure what the smallest possible size would be and I wanted all screenshots to be vaguely real-world like and the same size, so I cautiously chose dimensions that probably are a bit too large for most cases.

[extrawurst]

so I cautiously chose dimensions that probably are a bit too large for most cases

lets find a general minimum size where the ui still makes somewhat sense as a baseline to start from and then we use a bigger size for tests that require it

[cruessler]

I’ve now made the screenshots smaller. They are still quite wide as that is necessary for the path shortening to see the unshortened path in the header. The height is just a bit more than is required to show two panes above each other without either of them looking too small. Is that better?

[cruessler]

Looks like path shortening stopped working on Windows. Maybe that’s the reason the screenshots were 120 characters wide before my latest changes. :-)