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

Commit Message

Josh Steadmon May 17, 2023, 11:56 p.m. UTC

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

Change-Id: I182cdc1c15bdd1cbef6ffcf3d216b386f951e9fc
---
 Documentation/Makefile                 |  1 +
 Documentation/technical/unit-tests.txt | 47 ++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

Comments

Phillip Wood May 18, 2023, 1:13 p.m. UTC | #1

On 18/05/2023 00:56, [e-mail] wrote:
> Describe what we hope to accomplish by implementing unit tests, and
> explain some open questions and milestones.

Thanks for adding this.

> 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.

> 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/[e-mail]/[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.

> +
> +== 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

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

Glen Choo May 18, 2023, 8:15 p.m. UTC | #2

[e-mail] writes:

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

Thanks! I found this very helpful.

> 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.

The stated goals make sense to me, and I believe they are worth
restating. I believe this is mostly taken from Calvin's v1 cover letter

  https://lore.kernel.org/git/[e-mail]

so perhaps he should receive some writing credit in a commit trailer
(Helped-by?).

> +== 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.

A third option would be to pick another, more mature third party testing
library. As I mentioned in

  https://lore.kernel.org/git/[e-mail]

my primary concern is the maintainability and extensibility of a third
party library that (no offense to the original author) is not used very
widely, is relatively underdocumented, is missing features that we want,
and whose maintenance policy is relatively unknown to us. I'm not
opposed to taking in a third party testing framework, but we need to be
sure that we can rely on it instead of being something that requires
active upkeep.

I don't what sorts of testing libraries exist for C or how widely they
are used, but a quick web search gives some candidates that seem like
plausible alternatives to C TAP Harness:

- cmocka https://cmocka.org/ supports TAP, assert macros and mocking,
  and is used by other projects (their website indicates Samba, OpenVPN,
  etc). The documentation is a bit lacking IMO. It's apparently a fork
  of cmockery, which I'm not familiar with.

- Check https://libcheck.github.io/check/ supports TAP and has
  relatively good documentation, though the last release seems to have
  been 3 years ago.

- µnit https://nemequ.github.io/munit/ has a shiny website with nice
  docs (and a handy list of other unit test frameworks we can look at).
  The last release also seems to be ~3 years ago. Not sure if this
  supports TAP.

For flexibility, I also think it's reasonable for us to roll our own
testing library. I think it is perfectly fine for our unit test
framework to be suboptimal at the beginning, but owning the code makes
it relatively easy to fix bugs and extend it to our liking.

Josh Steadmon May 24, 2023, 5:40 p.m. UTC | #3

On 2023.05.18 13:15, Glen Choo wrote:
> [e-mail] writes:
> 
> > Describe what we hope to accomplish by implementing unit tests, and
> > explain some open questions and milestones.
> 
> Thanks! I found this very helpful.
> 
> > 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.
> 
> The stated goals make sense to me, and I believe they are worth
> restating. I believe this is mostly taken from Calvin's v1 cover letter
> 
>   https://lore.kernel.org/git/[e-mail]
> 
> so perhaps he should receive some writing credit in a commit trailer
> (Helped-by?).

Yeah, I missed some intended trailers while testing out b4. I'll make
sure it gets added back in V3.

Phillip Wood June 1, 2023, 9:19 a.m. UTC | #4

Hi Glen

On 18/05/2023 21:15, Glen Choo wrote:
> [e-mail] writes:
 >
> A third option would be to pick another, more mature third party testing
> library. As I mentioned in
> 
>    https://lore.kernel.org/git/[e-mail]
> 
> my primary concern is the maintainability and extensibility of a third
> party library that (no offense to the original author) is not used very
> widely, is relatively underdocumented, is missing features that we want,
> and whose maintenance policy is relatively unknown to us. I'm not
> opposed to taking in a third party testing framework, but we need to be
> sure that we can rely on it instead of being something that requires
> active upkeep.
> 
> I don't what sorts of testing libraries exist for C or how widely they
> are used, but a quick web search gives some candidates that seem like
> plausible alternatives to C TAP Harness:
> 
> - cmocka https://cmocka.org/ supports TAP, assert macros and mocking,
>    and is used by other projects (their website indicates Samba, OpenVPN,
>    etc). The documentation is a bit lacking IMO. It's apparently a fork
>    of cmockery, which I'm not familiar with.
> 
> - Check https://libcheck.github.io/check/ supports TAP and has
>    relatively good documentation, though the last release seems to have
>    been 3 years ago.
> 
> - µnit https://nemequ.github.io/munit/ has a shiny website with nice
>    docs (and a handy list of other unit test frameworks we can look at).
>    The last release also seems to be ~3 years ago. Not sure if this
>    supports TAP.

Thanks for finding those. I'd add

  - libtap https://github.com/zorgnax/libtap which seems to have nice
      assertions and TAP output. It is licensed under LGPL3 but used
      to be GPL2 and there don't seem to have been many changes since
      the license change so we could just use the older version.

I think it would be helpful to have a section in the plan which clearly 
states the features we want (TAP output, helpers that make it easy to 
write tests with good diagnostic output, ...) in our unit test framework 
and explains how the selected solution meets those criteria.

It would also be helpful to have a guide to writing effective tests that 
sets out some guidelines on where unit tests are appropriate and 
outlines our expectations in terms of style, coverage, leak cleanliness etc.

Best Wishes

Phillip

> For flexibility, I also think it's reasonable for us to roll our own
> testing library. I think it is perfectly fine for our unit test
> framework to be suboptimal at the beginning, but owning the code makes
> it relatively easy to fix bugs and extend it to our liking.

Patch

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. 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/[e-mail]/[our
+own TAP harness] just for Git.
+
+We believe that a large body of unit tests, living alongside the existing test
+suite, will improve code quality for the Git project.
+
+== 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.
+
+== Milestones
+
+* Settle on final TAP harness
+* Add useful tests of library-ish code
+* Integrate with CI
+* Integrate with
+  https://lore.kernel.org/git/[e-mail]/[stdlib
+  work]
+* Run along with regular `make test` target