ziglang / zig

General-purpose programming language and toolchain for maintaining robust, optimal, and reusable software.
https://ziglang.org
MIT License
32.1k stars 2.35k forks source link

Make std.Progress node available within unit test functions #20137

Open 190n opened 1 month ago

190n commented 1 month ago

I find it somewhat common to write a unit test which exhaustively checks many cases. Currently, the test runner does use std.Progress to report how many of the test functions have run, but there's no granularity within one function, which would be useful for test functions that take a long time to iterate over many test cases. It would be easier to monitor the progress of these tests if the progress node created for each test were made available to the test code somehow, which could optionally use it to report granular progress.

fearedbliss commented 2 weeks ago

Hey all, I've just ran into this issue, maybe there is another technique I could use in terms of architecture, or maybe I missed something, but I upgraded my OpenZFS snapshot cleaner to use 0.13.0 (mainly because I'm attracted by the progress bar haha). I got the bar working and it's nice, however, all of my existing "integration style unit tests" are broken. This is primarily being caused because we currently only allow one progress bar to be in the global section, but from within the test perspective, I'm not able to access that main node (which would allow me to pass that main node down the stack and allow the code to re-use that node). I was expecting there to be some sort of std.testing.progress as an easy way to get that node.

You can take a look at the usage of the main code, and the test at the following links:

Declared on Line 95, passed down into the Snapshot Manager on line 103.

The progress bar is stored inside the Snapshot Manager struct to be used later on in execution when the user confirms they want their snapshots deleted. As the snapshots are being deleted, the progress bar will update.

From the test perspective we can see that I've used dependency injection to mock out all of the dependencies (to the filesystem, etc. I've used the fat pointer interface approach w/ anyopaque + vtables to get that effect).

351 would need to be replaced with w/e solution we can come up with to allow testing code throughout the code base that takes a progress node.

Thanks for taking a look and the help, and keep up the great work. I'm happy to be sponsoring the project.

Jonathan

EDIT: One thing I thought about was that worse case scenario, I could implement an interface that essentially wraps the Progress / Node API itself...

fearedbliss commented 1 week ago

I was able to do a workaround to still be able to get all my tests to run and still have the progress bar display properly (including messages that come after the bar). It's a little nasty but it would have been more difficult if I tried to implement an interface to abstract away the whole progress bar api..

The "simpler" workaround was basically to make the root progress node passed into my snapshot manager an optional, that way I can pass null in the tests, and thus skip all of the sections of the code that uses the progress bar. I basically made a helper function called use_progress_bar that returns true or false if the self.progress_bar == null. Then every call that will interact with the progress bar object needs to call this function, and also every defer also needs an internal progress bar check so that it only tries to end() if the object was being used. This prevents panics that happen in the tests (if not checked).

The other thing which may be a separate issue or may be expected behavior, is that I was getting some weirdness between my stdout results being displayed after the code that uses the progress bar runs. The only way I was able to get it to display properly was to add a lock and unlock of the stderr stream right after the function using the progress bar finished. That allowed the bar to be displayed, and also for the stdout messages following the function, to be displayed appropriately. I'm happy to open another issue if this is an actual problem.

You can see all of this in the following links: