extrawurst / gitui

Blazing 💥 fast terminal-ui for git written in rust 🦀
MIT License
18.64k stars 573 forks source link

Add test for AsyncLog respecting GIT_DIR #2387

Closed cruessler closed 1 month ago

cruessler commented 1 month ago

This is a follow-up to #2301. It adds a test for AsyncLog::fetch_helper_without_filter respecting GIT_DIR. The docs to std::env::set_var say that it is only safe to be used in single-threaded programs. Could this ever become an issue? On my machine, make check was green.

(Creating snapshot tests running the gitui binary has proven to be rather challenging. So far, I have not found a crate that would work with enable_raw_mode which is why I decided to open this PR.)

extrawurst commented 1 month ago

look how we use #[serial] already to mark tests that need to be surely run un-parallel: https://github.com/extrawurst/gitui/blob/90a226927b226f33736f6c5cad150c895008a92a/asyncgit/src/sync/cred.rs#L256

extrawurst commented 1 month ago

(Creating snapshot tests running the gitui binary has proven to be rather challenging. So far, I have not found a crate that would work with enable_raw_mode which is why I decided to open this PR.)

Yeah that's gonna be tough, i think the more realistic approach would be to put gitui in a lib and only start it in interactive mode when used as a bin. the lib version could allow us to provide a different rendering backend to ratatui and then we could run snapshottests against that and fully control the inputs we send to the binary

cruessler commented 1 month ago

Since there is support for snapshots in ratatui (https://ratatui.rs/recipes/testing/snapshots/), I think your approach would work well. It guess it would require a couple of changes to the app, but eventually testing would be quite straightforward. I’ll try to create a PoC once I’ve got a bit of spare time on my hand. :-)

extrawurst commented 1 month ago

Awesome, thank yoU!

extrawurst commented 1 month ago

@cruessler Hm did it make the CI flaky? see https://github.com/extrawurst/gitui/actions/runs/11362704155/job/31605115788

cruessler commented 1 month ago

@cruessler Hm did it make the CI flaky? see https://github.com/extrawurst/gitui/actions/runs/11362704155/job/31605115788

That’s definitely possible. I’ll have a look! Do you happen to know whether #[serial] has to be applied to all tests in a file?

extrawurst commented 1 month ago

@cruessler Hm did it make the CI flaky? see https://github.com/extrawurst/gitui/actions/runs/11362704155/job/31605115788

That’s definitely possible. I’ll have a look! Do you happen to know whether #[serial] has to be applied to all tests in a file?

@cruessler i don’t think it’s about the file it’s about making sure any test that this can interfere with needs to have that annotation

cruessler commented 1 month ago

Does it make sense to run this test as part of a different test suite in its own process? Something like a separate invocation of cargo test …?

extrawurst commented 1 month ago

the problem is that we do not even know what the result is. we should unwrap so that we can see exactly what failed. i will roll this PR back, can you open a new PR then that redoes it but unwraps the result at the end so we can see what is going wrong?

extrawurst commented 1 month ago

@cruessler its reverted

cruessler commented 4 weeks ago

@extrawurst I created a follow-up PR: #2409.