Skip to content

Commit 8bf6fbd

Browse files
committed
Merge branch 'js/doc-unit-tests'
Process to add some form of low-level unit tests has started. * js/doc-unit-tests: ci: run unit tests in CI unit tests: add TAP unit test framework unit tests: add a project plan document
2 parents 0946acf + d8f416b commit 8bf6fbd

File tree

13 files changed

+1041
-5
lines changed

13 files changed

+1041
-5
lines changed

.cirrus.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,4 +19,4 @@ freebsd_12_task:
1919
build_script:
2020
- su git -c gmake
2121
test_script:
22-
- su git -c 'gmake test'
22+
- su git -c 'gmake DEFAULT_UNIT_TEST_TARGET=unit-tests-prove test unit-tests'

Documentation/Makefile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ TECH_DOCS += technical/scalar
122122
TECH_DOCS += technical/send-pack-pipeline
123123
TECH_DOCS += technical/shallow
124124
TECH_DOCS += technical/trivial-merge
125+
TECH_DOCS += technical/unit-tests
125126
SP_ARTICLES += $(TECH_DOCS)
126127
SP_ARTICLES += technical/api-index
127128

Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
= Unit Testing
2+
3+
In our current testing environment, we spend a significant amount of effort
4+
crafting end-to-end tests for error conditions that could easily be captured by
5+
unit tests (or we simply forgo some hard-to-setup and rare error conditions).
6+
Unit tests additionally provide stability to the codebase and can simplify
7+
debugging through isolation. Writing unit tests in pure C, rather than with our
8+
current shell/test-tool helper setup, simplifies test setup, simplifies passing
9+
data around (no shell-isms required), and reduces testing runtime by not
10+
spawning a separate process for every test invocation.
11+
12+
We believe that a large body of unit tests, living alongside the existing test
13+
suite, will improve code quality for the Git project.
14+
15+
== Definitions
16+
17+
For the purposes of this document, we'll use *test framework* to refer to
18+
projects that support writing test cases and running tests within the context
19+
of a single executable. *Test harness* will refer to projects that manage
20+
running multiple executables (each of which may contain multiple test cases) and
21+
aggregating their results.
22+
23+
In reality, these terms are not strictly defined, and many of the projects
24+
discussed below contain features from both categories.
25+
26+
For now, we will evaluate projects solely on their framework features. Since we
27+
are relying on having TAP output (see below), we can assume that any framework
28+
can be made to work with a harness that we can choose later.
29+
30+
31+
== Summary
32+
33+
We believe the best way forward is to implement a custom TAP framework for the
34+
Git project. We use a version of the framework originally proposed in
35+
https://lore.kernel.org/git/[email protected]/[1].
36+
37+
See the <<framework-selection,Framework Selection>> section below for the
38+
rationale behind this decision.
39+
40+
41+
== Choosing a test harness
42+
43+
During upstream discussion, it was occasionally noted that `prove` provides many
44+
convenient features, such as scheduling slower tests first, or re-running
45+
previously failed tests.
46+
47+
While we already support the use of `prove` as a test harness for the shell
48+
tests, it is not strictly required. The t/Makefile allows running shell tests
49+
directly (though with interleaved output if parallelism is enabled). Git
50+
developers who wish to use `prove` as a more advanced harness can do so by
51+
setting DEFAULT_TEST_TARGET=prove in their config.mak.
52+
53+
We will follow a similar approach for unit tests: by default the test
54+
executables will be run directly from the t/Makefile, but `prove` can be
55+
configured with DEFAULT_UNIT_TEST_TARGET=prove.
56+
57+
58+
[[framework-selection]]
59+
== Framework selection
60+
61+
There are a variety of features we can use to rank the candidate frameworks, and
62+
those features have different priorities:
63+
64+
* Critical features: we probably won't consider a framework without these
65+
** Can we legally / easily use the project?
66+
*** <<license,License>>
67+
*** <<vendorable-or-ubiquitous,Vendorable or ubiquitous>>
68+
*** <<maintainable-extensible,Maintainable / extensible>>
69+
*** <<major-platform-support,Major platform support>>
70+
** Does the project support our bare-minimum needs?
71+
*** <<tap-support,TAP support>>
72+
*** <<diagnostic-output,Diagnostic output>>
73+
*** <<runtime-skippable-tests,Runtime-skippable tests>>
74+
* Nice-to-have features:
75+
** <<parallel-execution,Parallel execution>>
76+
** <<mock-support,Mock support>>
77+
** <<signal-error-handling,Signal & error-handling>>
78+
* Tie-breaker stats
79+
** <<project-kloc,Project KLOC>>
80+
** <<adoption,Adoption>>
81+
82+
[[license]]
83+
=== License
84+
85+
We must be able to legally use the framework in connection with Git. As Git is
86+
licensed only under GPLv2, we must eliminate any LGPLv3, GPLv3, or Apache 2.0
87+
projects.
88+
89+
[[vendorable-or-ubiquitous]]
90+
=== Vendorable or ubiquitous
91+
92+
We want to avoid forcing Git developers to install new tools just to run unit
93+
tests. Any prospective frameworks and harnesses must either be vendorable
94+
(meaning, we can copy their source directly into Git's repository), or so
95+
ubiquitous that it is reasonable to expect that most developers will have the
96+
tools installed already.
97+
98+
[[maintainable-extensible]]
99+
=== Maintainable / extensible
100+
101+
It is unlikely that any pre-existing project perfectly fits our needs, so any
102+
project we select will need to be actively maintained and open to accepting
103+
changes. Alternatively, assuming we are vendoring the source into our repo, it
104+
must be simple enough that Git developers can feel comfortable making changes as
105+
needed to our version.
106+
107+
In the comparison table below, "True" means that the framework seems to have
108+
active developers, that it is simple enough that Git developers can make changes
109+
to it, and that the project seems open to accepting external contributions (or
110+
that it is vendorable). "Partial" means that at least one of the above
111+
conditions holds.
112+
113+
[[major-platform-support]]
114+
=== Major platform support
115+
116+
At a bare minimum, unit-testing must work on Linux, MacOS, and Windows.
117+
118+
In the comparison table below, "True" means that it works on all three major
119+
platforms with no issues. "Partial" means that there may be annoyances on one or
120+
more platforms, but it is still usable in principle.
121+
122+
[[tap-support]]
123+
=== TAP support
124+
125+
The https://testanything.org/[Test Anything Protocol] is a text-based interface
126+
that allows tests to communicate with a test harness. It is already used by
127+
Git's integration test suite. Supporting TAP output is a mandatory feature for
128+
any prospective test framework.
129+
130+
In the comparison table below, "True" means this is natively supported.
131+
"Partial" means TAP output must be generated by post-processing the native
132+
output.
133+
134+
Frameworks that do not have at least Partial support will not be evaluated
135+
further.
136+
137+
[[diagnostic-output]]
138+
=== Diagnostic output
139+
140+
When a test case fails, the framework must generate enough diagnostic output to
141+
help developers find the appropriate test case in source code in order to debug
142+
the failure.
143+
144+
[[runtime-skippable-tests]]
145+
=== Runtime-skippable tests
146+
147+
Test authors may wish to skip certain test cases based on runtime circumstances,
148+
so the framework should support this.
149+
150+
[[parallel-execution]]
151+
=== Parallel execution
152+
153+
Ideally, we will build up a significant collection of unit test cases, most
154+
likely split across multiple executables. It will be necessary to run these
155+
tests in parallel to enable fast develop-test-debug cycles.
156+
157+
In the comparison table below, "True" means that individual test cases within a
158+
single test executable can be run in parallel. We assume that executable-level
159+
parallelism can be handled by the test harness.
160+
161+
[[mock-support]]
162+
=== Mock support
163+
164+
Unit test authors may wish to test code that interacts with objects that may be
165+
inconvenient to handle in a test (e.g. interacting with a network service).
166+
Mocking allows test authors to provide a fake implementation of these objects
167+
for more convenient tests.
168+
169+
[[signal-error-handling]]
170+
=== Signal & error handling
171+
172+
The test framework should fail gracefully when test cases are themselves buggy
173+
or when they are interrupted by signals during runtime.
174+
175+
[[project-kloc]]
176+
=== Project KLOC
177+
178+
The size of the project, in thousands of lines of code as measured by
179+
https://dwheeler.com/sloccount/[sloccount] (rounded up to the next multiple of
180+
1,000). As a tie-breaker, we probably prefer a project with fewer LOC.
181+
182+
[[adoption]]
183+
=== Adoption
184+
185+
As a tie-breaker, we prefer a more widely-used project. We use the number of
186+
GitHub / GitLab stars to estimate this.
187+
188+
189+
=== Comparison
190+
191+
:true: [lime-background]#True#
192+
:false: [red-background]#False#
193+
:partial: [yellow-background]#Partial#
194+
195+
:gpl: [lime-background]#GPL v2#
196+
:isc: [lime-background]#ISC#
197+
:mit: [lime-background]#MIT#
198+
:expat: [lime-background]#Expat#
199+
:lgpl: [lime-background]#LGPL v2.1#
200+
201+
:custom-impl: https://lore.kernel.org/git/[email protected]/[Custom Git impl.]
202+
:greatest: https://github.com/silentbicycle/greatest[Greatest]
203+
:criterion: https://github.com/Snaipe/Criterion[Criterion]
204+
:c-tap: https://github.com/rra/c-tap-harness/[C TAP]
205+
:check: https://libcheck.github.io/check/[Check]
206+
207+
[format="csv",options="header",width="33%",subs="specialcharacters,attributes,quotes,macros"]
208+
|=====
209+
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>>"
210+
{custom-impl},{gpl},{true},{true},{true},{true},{true},{true},{false},{false},{false},1,0
211+
{greatest},{isc},{true},{partial},{true},{partial},{true},{true},{false},{false},{false},3,1400
212+
{criterion},{mit},{false},{partial},{true},{true},{true},{true},{true},{false},{true},19,1800
213+
{c-tap},{expat},{true},{partial},{partial},{true},{false},{true},{false},{false},{false},4,33
214+
{check},{lgpl},{false},{partial},{true},{true},{true},{false},{false},{false},{true},17,973
215+
|=====
216+
217+
=== Additional framework candidates
218+
219+
Several suggested frameworks have been eliminated from consideration:
220+
221+
* Incompatible licenses:
222+
** https://github.com/zorgnax/libtap[libtap] (LGPL v3)
223+
** https://cmocka.org/[cmocka] (Apache 2.0)
224+
* Missing source: https://www.kindahl.net/mytap/doc/index.html[MyTap]
225+
* No TAP support:
226+
** https://nemequ.github.io/munit/[µnit]
227+
** https://github.com/google/cmockery[cmockery]
228+
** https://github.com/lpabon/cmockery2[cmockery2]
229+
** https://github.com/ThrowTheSwitch/Unity[Unity]
230+
** https://github.com/siu/minunit[minunit]
231+
** https://cunit.sourceforge.net/[CUnit]
232+
233+
234+
== Milestones
235+
236+
* Add useful tests of library-like code
237+
* Integrate with
238+
https://lore.kernel.org/git/[email protected]/[stdlib
239+
work]
240+
* Run alongside regular `make test` target

Makefile

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -682,6 +682,9 @@ TEST_BUILTINS_OBJS =
682682
TEST_OBJS =
683683
TEST_PROGRAMS_NEED_X =
684684
THIRD_PARTY_SOURCES =
685+
UNIT_TEST_PROGRAMS =
686+
UNIT_TEST_DIR = t/unit-tests
687+
UNIT_TEST_BIN = $(UNIT_TEST_DIR)/bin
685688

686689
# Having this variable in your environment would break pipelines because
687690
# you cause "cd" to echo its destination to stdout. It can also take
@@ -1335,6 +1338,12 @@ THIRD_PARTY_SOURCES += compat/regex/%
13351338
THIRD_PARTY_SOURCES += sha1collisiondetection/%
13361339
THIRD_PARTY_SOURCES += sha1dc/%
13371340

1341+
UNIT_TEST_PROGRAMS += t-basic
1342+
UNIT_TEST_PROGRAMS += t-strbuf
1343+
UNIT_TEST_PROGS = $(patsubst %,$(UNIT_TEST_BIN)/%$X,$(UNIT_TEST_PROGRAMS))
1344+
UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
1345+
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
1346+
13381347
# xdiff and reftable libs may in turn depend on what is in libgit.a
13391348
GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
13401349
EXTLIBS =
@@ -2676,6 +2685,7 @@ OBJECTS += $(TEST_OBJS)
26762685
OBJECTS += $(XDIFF_OBJS)
26772686
OBJECTS += $(FUZZ_OBJS)
26782687
OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
2688+
OBJECTS += $(UNIT_TEST_OBJS)
26792689

26802690
ifndef NO_CURL
26812691
OBJECTS += http.o http-walker.o remote-curl.o
@@ -3178,7 +3188,7 @@ endif
31783188

31793189
test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
31803190

3181-
all:: $(TEST_PROGRAMS) $(test_bindir_programs)
3191+
all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS)
31823192

31833193
bin-wrappers/%: wrap-for-bin.sh
31843194
$(call mkdir_p_parent_template)
@@ -3609,7 +3619,7 @@ endif
36093619

36103620
artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
36113621
GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
3612-
$(MOFILES)
3622+
$(UNIT_TEST_PROGS) $(MOFILES)
36133623
$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
36143624
SHELL_PATH='$(SHELL_PATH_SQ)' PERL_PATH='$(PERL_PATH_SQ)'
36153625
test -n "$(ARTIFACTS_DIRECTORY)"
@@ -3671,7 +3681,7 @@ clean: profile-clean coverage-clean cocciclean
36713681
$(RM) headless-git.o
36723682
$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
36733683
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
3674-
$(RM) $(TEST_PROGRAMS)
3684+
$(RM) $(TEST_PROGRAMS) $(UNIT_TEST_PROGS)
36753685
$(RM) $(FUZZ_PROGRAMS)
36763686
$(RM) $(SP_OBJ)
36773687
$(RM) $(HCC)
@@ -3850,3 +3860,15 @@ $(FUZZ_PROGRAMS): all
38503860
$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@
38513861

38523862
fuzz-all: $(FUZZ_PROGRAMS)
3863+
3864+
$(UNIT_TEST_BIN):
3865+
@mkdir -p $(UNIT_TEST_BIN)
3866+
3867+
$(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/test-lib.o $(GITLIBS) GIT-LDFLAGS $(UNIT_TEST_BIN)
3868+
$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
3869+
$(filter %.o,$^) $(filter %.a,$^) $(LIBS)
3870+
3871+
.PHONY: build-unit-tests unit-tests
3872+
build-unit-tests: $(UNIT_TEST_PROGS)
3873+
unit-tests: $(UNIT_TEST_PROGS)
3874+
$(MAKE) -C t/ unit-tests

ci/run-build-and-tests.sh

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ if test -n "$run_tests"
5050
then
5151
group "Run tests" make test ||
5252
handle_failed_tests
53+
group "Run unit tests" \
54+
make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
5355
fi
5456
check_unignored_build_artifacts
5557

ci/run-test-slice.sh

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,4 +15,9 @@ group "Run tests" make --quiet -C t T="$(cd t &&
1515
tr '\n' ' ')" ||
1616
handle_failed_tests
1717

18+
# We only have one unit test at the moment, so run it in the first slice
19+
if [ "$1" == "0" ] ; then
20+
group "Run unit tests" make --quiet -C t unit-tests-prove
21+
fi
22+
1823
check_unignored_build_artifacts

t/Makefile

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ TAR ?= $(TAR)
1717
RM ?= rm -f
1818
PROVE ?= prove
1919
DEFAULT_TEST_TARGET ?= test
20+
DEFAULT_UNIT_TEST_TARGET ?= unit-tests-raw
2021
TEST_LINT ?= test-lint
2122

2223
ifdef TEST_OUTPUT_DIRECTORY
@@ -41,6 +42,7 @@ TPERF = $(sort $(wildcard perf/p[0-9][0-9][0-9][0-9]-*.sh))
4142
TINTEROP = $(sort $(wildcard interop/i[0-9][0-9][0-9][0-9]-*.sh))
4243
CHAINLINTTESTS = $(sort $(patsubst chainlint/%.test,%,$(wildcard chainlint/*.test)))
4344
CHAINLINT = '$(PERL_PATH_SQ)' chainlint.pl
45+
UNIT_TESTS = $(sort $(filter-out unit-tests/bin/t-basic%,$(wildcard unit-tests/bin/t-*)))
4446

4547
# `test-chainlint` (which is a dependency of `test-lint`, `test` and `prove`)
4648
# checks all tests in all scripts via a single invocation, so tell individual
@@ -65,6 +67,17 @@ prove: pre-clean check-chainlint $(TEST_LINT)
6567
$(T):
6668
@echo "*** $@ ***"; '$(TEST_SHELL_PATH_SQ)' $@ $(GIT_TEST_OPTS)
6769

70+
$(UNIT_TESTS):
71+
@echo "*** $@ ***"; $@
72+
73+
.PHONY: unit-tests unit-tests-raw unit-tests-prove
74+
unit-tests: $(DEFAULT_UNIT_TEST_TARGET)
75+
76+
unit-tests-raw: $(UNIT_TESTS)
77+
78+
unit-tests-prove:
79+
@echo "*** prove - unit tests ***"; $(PROVE) $(GIT_PROVE_OPTS) $(UNIT_TESTS)
80+
6881
pre-clean:
6982
$(RM) -r '$(TEST_RESULTS_DIRECTORY_SQ)'
7083

@@ -149,4 +162,4 @@ perf:
149162
$(MAKE) -C perf/ all
150163

151164
.PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
152-
check-chainlint clean-chainlint test-chainlint
165+
check-chainlint clean-chainlint test-chainlint $(UNIT_TESTS)

0 commit comments

Comments
 (0)