Re: [PATCH] t-progress.c : unit tests for progress.c

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 11, 2023, 17:29 UTC
Message-ID: <xmqq34yhm5in.fsf@gitster.g>
In-Reply-To: <20231011082716.901048-1-siddhartth@google.com>

Siddharth Singh <siddhartth@google.com> writes:

Show 8 quoted lines

> These tests are directly inspired from the tests inside
> t/t0500-progress-display.sh
>
> The existing shell tests for the Git progress library only test the output of the library, not the correctness of the progress struct fields. Unit tests can fill this gap and improve confidence that the library works as expected. For example, unit tests can verify that the progress struct fields are updated correctly when the library is used.
>
> Change-Id: I190522f29fdab9291af71b7788eeee2c0f26282d
> Signed-off-by: Siddharth Singh <siddhartth@google.com>
> ---

The contents is of course important, but please also pay attention to the formatting to make what you write readable. Writing good things does not help if it is not read. Wrap lines to appropriate lengths.

Documentation/SubmittingPatches is your friend.

>
> Dear Git Community,

Show 13 quoted lines

> As you may be aware, my colleague Josh is proposing a unit testing
> framework[1] on the mailing list. I attempted to use the latest
> version of that series to convert t/helper/test-progress.c to unit
> tests. However, while writing the tests, I realized that the way
> progress.c is implemented makes it very difficult to test it in
> units.
>
> Firstly, most unit tests are typically written as a method that
> takes the expected output and the actual output of the unit being
> tested, and compares them for equality. However, because it's
> intended to print user-facing output on an interactive terminal,
> progress.c prints everything out to stderr, which makes it
> difficult to unit test.

It sounds like you found where the test framework is lacking. If making sure what we spew out to the standard error stream is worth covering in the unit tests, the test framework needs to support it, right?

Show 13 quoted lines

> There are a few ways to work around this issue in my opinion. One
> way is to modify the library that does not print to output stream
> and returns the data to the caller:
>
> static void display(struct progress *progress, uint64_t n, char *done){
> …
> }
>
> becomes
>
> struct strbuf display(struct progress *progress,uint64_t n,char *done){
> …
> }

The approach adds a feature for outside callers to access this bit of internal implementation detail of the progress code. If no real callers want to exercise that feature and it is only useful for testing, it smells like the tail wagging the dog, though.

It certainly is not worth butchering the real code for the sake of working around the lack of current unit test framework to contaminate the global namespace with way too generically named function "display()".

Assuming that you *must* work around the lack of stderr support, it probably would make much more sense to add a new member that points at an output stream to "struct progress". Make it a thin wrapper around "FILE *" that supports whatever display() and the helper functions it calls needs, like write(2) or fprintf(3). And you can mock that "output stream" in your unit tests to append to an in-core buffer if you wanted to. The production code does not have to care about your mock that way.