Re: [PATCH RFC v2 2/4] unit tests: Add a project plan document

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 18, 2023, 13:13 UTC
Message-ID: <669532af-fa0b-5dea-7da2-ac5caa0560c8@gmail.com>
In-Reply-To: <20230517-unit-tests-v2-v2-2-21b5b60f4b32@google.com>

On 18/05/2023 00:56, steadmon@google.com wrote:

> Describe what we hope to accomplish by implementing unit tests, and
> explain some open questions and milestones.

Thanks for adding this.

Show 37 quoted lines

> Change-Id: I182cdc1c15bdd1cbef6ffcf3d216b386f951e9fc
> ---
>   Documentation/Makefile                 |  1 +
>   Documentation/technical/unit-tests.txt | 47 ++++++++++++++++++++++++++++++++++
>   2 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/Makefile b/Documentation/Makefile
> index b629176d7d..3f2383a12c 100644
> --- a/Documentation/Makefile
> +++ b/Documentation/Makefile
> @@ -122,6 +122,7 @@ TECH_DOCS += technical/scalar
>   TECH_DOCS += technical/send-pack-pipeline
>   TECH_DOCS += technical/shallow
>   TECH_DOCS += technical/trivial-merge
> +TECH_DOCS += technical/unit-tests
>   SP_ARTICLES += $(TECH_DOCS)
>   SP_ARTICLES += technical/api-index
>   
> diff --git a/Documentation/technical/unit-tests.txt b/Documentation/technical/unit-tests.txt
> new file mode 100644
> index 0000000000..7c575e6ef7
> --- /dev/null
> +++ b/Documentation/technical/unit-tests.txt
> @@ -0,0 +1,47 @@
> += Unit Testing
> +
> +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. 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.

As we're already using prove as a TAP harness for our existing tests I'd prefer not to add another harness unless we really need to. prove allows us to run tests in parallel and has options for rerunning only the tests that failed last time and for running slow tests first to reduce the overall run time.

  I haven't looked at runtests in detail but at a quick glance I'm not 
sure which of those features it supports. I'm also worried about its 
windows compatibility. I see it sets some environment variables that 
some features of the test library require. I'm not sure if we plan to 
use those features, if we do I think we could probably set those paths 
when the tests are compiled.

Are you able to expand on why it needs a non-restrictive license? From a technical point of view surely anything that is GPLv2 compatible would be fine as that is the license we're already using for our code.

Show 6 quoted lines

> Fortunately,
> +there already exists a https://github.com/rra/c-tap-harness/[C TAP harness
> +library] with an MIT license (at least for the files needed for our purposes).
> +We might also consider implementing
> +https://lore.kernel.org/git/c902a166-98ce-afba-93f2-ea6027557176@gmail.com/[our
> +own TAP harness] just for Git.

If we do decide to go that route I'm very happy for you or one of your colleagues to take that patch forward.

> +We believe that a large body of unit tests, living alongside the existing test
> +suite, will improve code quality for the Git project.

This is slightly off-topic and can be addressed later. One thing that occurred to me was that if we end up with hundreds of unit files it would be good to link them into a single executable as we do with test-tool to avoid wasting time and disc space having to link hundreds of individual programs. We'd have to figure out how to run the individual tests though if we do that.

Show 16 quoted lines

> +
> +== Open questions
> +
> +=== TAP harness
> +
> +We'll need to decide on a TAP harness. The C TAP library is easy to integrate,
> +but has a few drawbacks:
> +* (copy objections from lore thread)
> +* We may need to carry local patches against C TAP. We'll need to decide how to
> +  manage these. We could vendor the code in and modify them directly, or use a
> +  submodule (but then we'll need to decide on where to host the submodule with
> +  our patches on top).
> +
> +Phillip Wood has also proposed a new implementation of a TAP harness (linked
> +above). While it hasn't been thoroughly reviewed yet, it looks to support a few
> +nice features that C TAP does not, e.g. lazy test plans and skippable tests.

strictly speaking both those are supported in terms of TAP output by c-tap-harness but they're not very friendly to use. For me the big difference is that my library provides a set of check* macros and functions that automatically print diagnostic messages when a check fails and the test framework maintains the pass/fail state based on those checks.

Best Wishes

Phillip

Show 10 quoted lines

> +== Milestones
> +
> +* Settle on final TAP harness
> +* Add useful tests of library-ish code
> +* Integrate with CI
> +* Integrate with
> +  https://lore.kernel.org/git/20230502211454.1673000-1-calvinwan@google.com/[stdlib
> +  work]
> +* Run along with regular `make test` target
>