Re: [RFC PATCH 0/2] add an external testing library for unit tests

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 2, 2023, 13:52 UTC
Message-ID: <230502.861qjyj0cb.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230427175007.902278-1-calvinwan@google.com>

On Thu, Apr 27 2023, Calvin Wan wrote:

Show 29 quoted lines

> In our current testing environment, we spend a significant amount of
> effort crafting end-to-end tests for error conditions that could easily
> be captured by unit tests (or we simply forgo some hard-to-setup and
> rare error conditions). Unit tests additionally provide stability to the
> codebase and can simplify debugging through isolation. Turning parts of
> Git into libraries[1] gives us the ability to run unit tests on the
> libraries and to write unit tests in C. Writing unit tests in pure C,
> rather than with our current shell/test-tool helper setup, simplifies
> test setup, simplifies passing data around (no shell-isms required), and
> reduces testing runtime by not spawning a separate process for every
> test invocation.
>
> Unit testing in C requires a separate testing harness that we ideally
> would like to be TAP-style and to come with a non-restrictive license.
> Fortunately, there already exists a C TAP harness library[2] with an MIT
> license (at least for the files included in this series). 
>
> This first patch introduces the C TAP harness and includes only the
> necessary files. The second patch showcases a basic example of it. As an
> RFC, I am wondering what the list thinks about using a second testing
> library for unit testing? Are there any problems with this particular
> external testing library and does it provide the necessary functionality
> we would want for unit testing Git libraries? How should the folders be
> structured and how should the new Makefile rules be written? Ultimately,
> this will help us determine the setup of our unit tests in future
> libification patches.
>
> [1] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==BGzC__Bg2_pr7krNq6MA@mail.gmail.com/
> [2] https://github.com/rra/c-tap-harness/

I have some out-of-tree patches I've been meaning to submit that massage some of our TAP output, and I'd really prefer if we don't end up with two TAP emitters in-tree if we can help it.

We can support such a thing, but nothing about your goals or your explanation here provides the "why".

Or rather, I'm not really buying the "passing data around" or "recuding [...] runtime [overhead]". I think you're describing how *some* of our *.sh to *.c interop goes, but we have test-lib.sh driving C code without those issues.

We already have pure-C libraries that we add a really shim to unit test,
the most extreme example of this is t0032-reftable-unittest.sh, whose
body is simply (excluding comments):
	
	#!/bin/sh
	test_description='reftable unittests'
	
	TEST_PASSES_SANITIZE_LEAK=true
	. ./test-lib.sh
	
	test_expect_success 'unittests' '
		TMPDIR=$(pwd) && export TMPDIR &&
		test-tool reftable
	'
	
	test_done

Now, that goes into reftable/test_framework.h which basically implements its own mini-test framework, so that's at least a *partial* argument for what you're suggesting here, but note that it doesn't emit TAP, it just returns an exit code, the EXPECT() etc. is purely internal. I.e. "what should we return?".

Probably a more git-y example is t0071-sort.sh, whose body is similar
(skipping most of the boilerplate):
	
	test_expect_success 'DEFINE_LIST_SORT_DEBUG' '
		test-tool mergesort test
	'

We then have similar library tests as e.g. t0063-string-list.sh, t0061-run-command.sh, t3070-wildmatch.sh etc.

None of those are perfect, but I think the current arrangement is rather ideal. We can write most or all of the test in C, but we just do so by calling a function that returns an exit code.

It does mean we need to spawn a shell for test-lib.sh, and call a test-tool at least once, but the overhead of that is trivial. It's not trivial in some cases where we call the helper in a loop, but that's much more easily addressed than hoisting all of TAP generation into the C space.