[v9,0/3] Add unit test framework and project plan

Message

Josh Steadmon Nov. 1, 2023, 11:31 p.m. UTC

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.

This series begins with a project document covering our goals for adding
unit tests and a discussion of alternative frameworks considered, as
well as the features used to evaluate them. A rendered preview of this
doc can be found at [2]. It also adds Phillip Wood's TAP implemenation
(with some slightly re-worked Makefile rules) and a sample strbuf unit
test. Finally, we modify the configs for GitHub and Cirrus CI to run the
unit tests. Sample runs showing successful CI runs can be found at [3],
[4], and [5].

[1] https://lore.kernel.org/git/CAJoAoZ=Cig_kLocxKGax31sU7Xe4==[e-mail]/
[2] https://github.com/steadmon/git/blob/unit-tests-asciidoc/Documentation/technical/unit-tests.adoc
[3] https://github.com/steadmon/git/actions/runs/5884659246/job/15959781385#step:4:1803
[4] https://github.com/steadmon/git/actions/runs/5884659246/job/15959938401#step:5:186
[5] https://cirrus-ci.com/task/6126304366428160 (unrelated tests failed,
    but note that t-strbuf ran successfully)

Changes in v9:
- Included some asciidoc cleanups suggested by Oswald Buddenhagen.
- Applied a style fixup that Coccinelle complained about.
- Applied some NULL-safety fixups.
- Used check_*() more widely in t-strbuf helper functions

Changes in v8:
- Flipped return values for TEST, TEST_TODO, and check_* macros &
  functions. This makes it easier to reason about control flow for
  patterns like:
    if (check(some_condition)) { ... }
- Moved unit test binaries to t/unit-tests/bin to simplify .gitignore
  patterns.
- Removed testing of some strbuf implementation details in t-strbuf.c

Changes in v7:
- Fix corrupt diff in patch #2, sorry for the noise.

Changes in v6:
- Officially recommend using Phillip Wood's TAP framework
- Add an example strbuf unit test using the TAP framework as well as
  Makefile integration
- Run unit tests in CI

Changes in v5:
- Add comparison point "License".
- Discuss feature priorities
- Drop frameworks:
  - Incompatible licenses: libtap, cmocka
  - Missing source: MyTAP
  - No TAP support: µnit, cmockery, cmockery2, Unity, minunit, CUnit
- Drop comparison point "Coverage reports": this can generally be
  handled by tools such as `gcov` regardless of the framework used.
- Drop comparison point "Inline tests": there didn't seem to be
  strong interest from reviewers for this feature.
- Drop comparison point "Scheduling / re-running": this was not
  supported by any of the main contenders, and is generally better
  handled by the harness rather than framework.
- Drop comparison point "Lazy test planning": this was supported by
  all frameworks that provide TAP output.

Changes in v4:
- Add link anchors for the framework comparison dimensions
- Explain "Partial" results for each dimension
- Use consistent dimension names in the section headers and comparison
  tables
- Add "Project KLOC", "Adoption", and "Inline tests" dimensions
- Fill in a few of the missing entries in the comparison table

Changes in v3:
- Expand the doc with discussion of desired features and a WIP
  comparison.
- Drop all implementation patches until a framework is selected.
- Link to v2: https://lore.kernel.org/r/[e-mail]


Josh Steadmon (2):
  unit tests: Add a project plan document
  ci: run unit tests in CI

Phillip Wood (1):
  unit tests: add TAP unit test framework

 .cirrus.yml                            |   2 +-
 Documentation/Makefile                 |   1 +
 Documentation/technical/unit-tests.txt | 240 ++++++++++++++++++
 Makefile                               |  28 ++-
 ci/run-build-and-tests.sh              |   2 +
 ci/run-test-slice.sh                   |   5 +
 t/Makefile                             |  15 +-
 t/t0080-unit-test-output.sh            |  58 +++++
 t/unit-tests/.gitignore                |   1 +
 t/unit-tests/t-basic.c                 |  95 +++++++
 t/unit-tests/t-strbuf.c                | 120 +++++++++
 t/unit-tests/test-lib.c                | 329 +++++++++++++++++++++++++
 t/unit-tests/test-lib.h                | 149 +++++++++++
 13 files changed, 1040 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/technical/unit-tests.txt
 create mode 100755 t/t0080-unit-test-output.sh
 create mode 100644 t/unit-tests/.gitignore
 create mode 100644 t/unit-tests/t-basic.c
 create mode 100644 t/unit-tests/t-strbuf.c
 create mode 100644 t/unit-tests/test-lib.c
 create mode 100644 t/unit-tests/test-lib.h

Range-diff against v8:
1:  81c5148a12 ! 1:  f706ba9b68 unit tests: Add a project plan document
    @@ Commit message
         rare error conditions). Describe what we hope to accomplish by
         implementing unit tests, and explain some open questions and milestones.
         Discuss desired features for test frameworks/harnesses, and provide a
    -    preliminary comparison of several different frameworks.
    +    comparison of several different frameworks. Finally, document our
    +    rationale for implementing a custom framework.
     
         Co-authored-by: Calvin Wan <[e-mail]>
    @@ Documentation/technical/unit-tests.txt (new)
     +can be made to work with a harness that we can choose later.
     +
     +
    -+== Choosing a framework
    ++== Summary
     +
    -+We believe the best option is to implement a custom TAP framework for the Git
    -+project. We use a version of the framework originally proposed in
    ++We believe the best way forward is to implement a custom TAP framework for the
    ++Git project. We use a version of the framework originally proposed in
     +https://lore.kernel.org/git/[e-mail]/[1].
     +
    ++See the <<framework-selection,Framework Selection>> section below for the
    ++rationale behind this decision.
    ++
     +
     +== Choosing a test harness
     +
    @@ Documentation/technical/unit-tests.txt (new)
     +configured with DEFAULT_UNIT_TEST_TARGET=prove.
     +
     +
    ++[[framework-selection]]
     +== Framework selection
     +
     +There are a variety of features we can use to rank the candidate frameworks, and
    @@ Documentation/technical/unit-tests.txt (new)
     +
     +=== Comparison
     +
    -+[format="csv",options="header",width="33%"]
    ++:true: [lime-background]#True#
    ++:false: [red-background]#False#
    ++:partial: [yellow-background]#Partial#
    ++
    ++:gpl: [lime-background]#GPL v2#
    ++:isc: [lime-background]#ISC#
    ++:mit: [lime-background]#MIT#
    ++:expat: [lime-background]#Expat#
    ++:lgpl: [lime-background]#LGPL v2.1#
    ++
    ++:custom-impl: https://lore.kernel.org/git/[e-mail]/[Custom Git impl.]
    ++:greatest: https://github.com/silentbicycle/greatest[Greatest]
    ++:criterion: https://github.com/Snaipe/Criterion[Criterion]
    ++:c-tap: https://github.com/rra/c-tap-harness/[C TAP]
    ++:check: https://libcheck.github.io/check/[Check]
    ++
    ++[format="csv",options="header",width="33%",subs="specialcharacters,attributes,quotes,macros"]
     +|=====
     +Framework,"<<license,License>>","<<vendorable-or-ubiquitous,Vendorable or ubiquitous>>","<<maintainable-extensible,Maintainable / extensible>>","<<major-platform-support,Major platform support>>","<<tap-support,TAP support>>","<<diagnostic-output,Diagnostic output>>","<<runtime--skippable-tests,Runtime- skippable tests>>","<<parallel-execution,Parallel execution>>","<<mock-support,Mock support>>","<<signal-error-handling,Signal & error handling>>","<<project-kloc,Project KLOC>>","<<adoption,Adoption>>"
    -+https://lore.kernel.org/git/[e-mail]/[Custom Git impl.],[lime-background]#GPL v2#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,1,0
    -+https://github.com/silentbicycle/greatest[Greatest],[lime-background]#ISC#,[lime-background]#True#,[yellow-background]#Partial#,[lime-background]#True#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,3,1400
    -+https://github.com/Snaipe/Criterion[Criterion],[lime-background]#MIT#,[red-background]#False#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[lime-background]#True#,19,1800
    -+https://github.com/rra/c-tap-harness/[C TAP],[lime-background]#Expat#,[lime-background]#True#,[yellow-background]#Partial#,[yellow-background]#Partial#,[lime-background]#True#,[red-background]#False#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,4,33
    -+https://libcheck.github.io/check/[Check],[lime-background]#LGPL v2.1#,[red-background]#False#,[yellow-background]#Partial#,[lime-background]#True#,[lime-background]#True#,[lime-background]#True#,[red-background]#False#,[red-background]#False#,[red-background]#False#,[lime-background]#True#,17,973
    ++{custom-impl},{gpl},{true},{true},{true},{true},{true},{true},{false},{false},{false},1,0
    ++{greatest},{isc},{true},{partial},{true},{partial},{true},{true},{false},{false},{false},3,1400
    ++{criterion},{mit},{false},{partial},{true},{true},{true},{true},{true},{false},{true},19,1800
    ++{c-tap},{expat},{true},{partial},{partial},{true},{false},{true},{false},{false},{false},4,33
    ++{check},{lgpl},{false},{partial},{true},{true},{true},{false},{false},{false},{true},17,973
     +|=====
     +
     +=== Additional framework candidates
2:  00d3c95a81 ! 2:  8b831f4937 unit tests: add TAP unit test framework
    @@ t/unit-tests/t-strbuf.c (new)
     +static int assert_sane_strbuf(struct strbuf *buf)
     +{
     +	/* Initialized strbufs should always have a non-NULL buffer */
    -+	if (buf->buf == NULL)
    ++	if (!check(!!buf->buf))
     +		return 0;
     +	/* Buffers should always be NUL-terminated */
    -+	if (buf->buf[buf->len] != '\0')
    ++	if (!check_char(buf->buf[buf->len], ==, '\0'))
     +		return 0;
     +	/*
     +	 * Freshly-initialized strbufs may not have a dynamically allocated
    @@ t/unit-tests/t-strbuf.c (new)
     +	if (buf->len == 0 && buf->alloc == 0)
     +		return 1;
     +	/* alloc must be at least one byte larger than len */
    -+	return buf->len + 1 <= buf->alloc;
    ++	return check_uint(buf->len, <, buf->alloc);
     +}
     +
     +static void t_static_init(void)
    @@ t/unit-tests/test-lib.h (new)
     +
     +/*
     + * Test checks are built around test_assert(). checks return 1 on
    -+ * success, 0 on failure. If any check fails then the test will
    -+ * fail. To create a custom check define a function that wraps
    -+ * test_assert() and a macro to wrap that function. For example:
    ++ * success, 0 on failure. If any check fails then the test will fail. To
    ++ * create a custom check define a function that wraps test_assert() and
    ++ * a macro to wrap that function to provide a source location and
    ++ * stringified arguments. Custom checks that take pointer arguments
    ++ * should be careful to check that they are non-NULL before
    ++ * dereferencing them. For example:
     + *
     + *  static int check_oid_loc(const char *loc, const char *check,
     + *			     struct object_id *a, struct object_id *b)
     + *  {
    -+ *	    int res = test_assert(loc, check, oideq(a, b));
    ++ *	    int res = test_assert(loc, check, a && b && oideq(a, b));
     + *
    -+ *	    if (res) {
    -+ *		    test_msg("   left: %s", oid_to_hex(a);
    -+ *		    test_msg("  right: %s", oid_to_hex(a);
    ++ *	    if (!res) {
    ++ *		    test_msg("   left: %s", a ? oid_to_hex(a) : "NULL";
    ++ *		    test_msg("  right: %s", b ? oid_to_hex(a) : "NULL";
     + *
     + *	    }
     + *	    return res;
    @@ t/unit-tests/test-lib.h (new)
     +#define check_int(a, op, b)						\
     +	(test__tmp[0].i = (a), test__tmp[1].i = (b),			\
     +	 check_int_loc(TEST_LOCATION(), #a" "#op" "#b,			\
    -+		       test__tmp[0].i op test__tmp[1].i, a, b))
    ++		       test__tmp[0].i op test__tmp[1].i,		\
    ++		       test__tmp[0].i, test__tmp[1].i))
     +int check_int_loc(const char *loc, const char *check, int ok,
     +		  intmax_t a, intmax_t b);
     +
    @@ t/unit-tests/test-lib.h (new)
     +#define check_uint(a, op, b)						\
     +	(test__tmp[0].u = (a), test__tmp[1].u = (b),			\
     +	 check_uint_loc(TEST_LOCATION(), #a" "#op" "#b,			\
    -+			test__tmp[0].u op test__tmp[1].u, a, b))
    ++			test__tmp[0].u op test__tmp[1].u,		\
    ++			test__tmp[0].u, test__tmp[1].u))
     +int check_uint_loc(const char *loc, const char *check, int ok,
     +		   uintmax_t a, uintmax_t b);
     +
    @@ t/unit-tests/test-lib.h (new)
     +#define check_char(a, op, b)						\
     +	(test__tmp[0].c = (a), test__tmp[1].c = (b),			\
     +	 check_char_loc(TEST_LOCATION(), #a" "#op" "#b,			\
    -+			test__tmp[0].c op test__tmp[1].c, a, b))
    ++			test__tmp[0].c op test__tmp[1].c,		\
    ++			test__tmp[0].c, test__tmp[1].c))
     +int check_char_loc(const char *loc, const char *check, int ok,
     +		   char a, char b);
     +
3:  aa1dfa4892 = 3:  08d27bb5f9 ci: run unit tests in CI

base-commit: a9e066fa63149291a55f383cfa113d8bdbdaa6b3