Move tests to separate directories, and document

Today, with the tests inside a `tests` intermingled with the
corresponding library's source code, we have a few problems:

- We have to be careful that wildcards don't end up with tests being
  built as part of Nix proper, or test headers being installed as part
  of Nix proper.

- Understanding `#include` resolution is very hard, because files
  instead `tests` directories can shadow those outside by mistake.

This solves the first problem and partially solves the second problem
--- test headers still need to live inside a `tests` directory, but
there is no punning of project source/header file "root" dirs and `-I`
dirs.

Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>

.gitignore

@@ -41,14 +41,14 @@ perl/Makefile.config
 /src/libexpr/parser-tab.hh
 /src/libexpr/parser-tab.output
 /src/libexpr/nix.tbl
-/src/libexpr/tests/libnixexpr-tests
+/tests/unit/libexpr/libnixexpr-tests
 
 # /src/libstore/
 *.gen.*
-/src/libstore/tests/libnixstore-tests
+/tests/unit/libstore/libnixstore-tests
 
 # /src/libutil/
-/src/libutil/tests/libnixutil-tests
+/tests/unit/libutil/libnixutil-tests
 
 /src/nix/nix
 

Makefile

@@ -25,11 +25,11 @@ makefiles = \
 endif
 
 ifeq ($(ENABLE_BUILD)_$(ENABLE_TESTS), yes_yes)
-UNIT_TEST_ENV = _NIX_TEST_UNIT_DATA=unit-test-data
+UNIT_TEST_ENV = _NIX_TEST_UNIT_DATA=tests/unit/data
 makefiles += \
-  src/libutil/tests/local.mk \
-  src/libstore/tests/local.mk \
-  src/libexpr/tests/local.mk
+  tests/unit/libutil/local.mk \
+  tests/unit/libstore/local.mk \
+  tests/unit/libexpr/local.mk
 endif
 
 ifeq ($(ENABLE_TESTS), yes)

doc/internal-api/doxygen.cfg.in

@@ -39,17 +39,21 @@ INPUT                  = \
   src/libcmd \
   src/libexpr \
   src/libexpr/flake \
-  src/libexpr/tests \
-  src/libexpr/tests/value \
+  tests/unit/libexpr \
+  tests/unit/libexpr/value \
+  tests/unit/libexpr/test \
+  tests/unit/libexpr/test/value \
   src/libexpr/value \
   src/libfetchers \
   src/libmain \
   src/libstore \
   src/libstore/build \
   src/libstore/builtins \
-  src/libstore/tests \
+  tests/unit/libstore \
+  tests/unit/libstore/test \
   src/libutil \
-  src/libutil/tests \
+  tests/unit/libutil \
+  tests/unit/libutil/test \
   src/nix \
   src/nix-env \
   src/nix-store

doc/manual/src/contributing/testing.md

@@ -30,32 +30,69 @@ The unit tests are defined using the [googletest] and [rapidcheck] frameworks.
 > ├── libexpr
 > │   ├── value/context.hh
 > │   ├── value/context.cc
+> │   …
+> │
+> ├── tests
 > │   │
 > │   …
->     └── tests
-> │       ├── value/context.hh
-> │       ├── value/context.cc
+> │   └── unit
+> │       ├── libutil
+> │       │   │
+> │       │   …
 > │       │
+> │       ├── libexpr
+> │       │   └── test
+> │       │       ├── value/context.hh
+> │       │       ├── value/context.cc
+> │       │       …
 > │       …
-> │
-> ├── unit-test-data
-> │   ├── libstore
-> │   │   ├── worker-protocol/content-address.bin
-> │   │   …
-> │   …
+> │       └── data
+> │           ├── libutil
+> │           │   ├── git/tree.txt
+> │           │   …
+> │           ├── libexpr
+> │           │   │
+> │           │   …
+> │           …
 > …
 > ```
 
-The unit tests for each Nix library (`libnixexpr`, `libnixstore`, etc..) live inside a directory `src/${library_shortname}/tests` within the directory for the library (`src/${library_shortname}`).
+The tests for each Nix library (`libnixexpr`, `libnixstore`, etc..) live inside a directory `src/${library_name_without-nix}-tests` next to the directory for the library (`src/${library_name_without-nix}`).
+Given a interface (header) and implementation pair in the original library, say, `libexpr/value/context.{hh,cc}`, we write tests for it in `libexpr-tests/value/context.cc`, and (possibly) declare additional interfaces for testing purposes in `libexpr-tests/test/value/context.hh`.
+
+> **Note**
+>
+> Only the extra header is nested inside the `test` directory. This is explained further below.
 
-The data is in `unit-test-data`, with one subdir per library, with the same name as where the code goes.
-For example, `libnixstore` code is in `src/libstore`, and its test data is in `unit-test-data/libstore`.
-The path to the `unit-test-data` directory is passed to the unit test executable with the environment variable `_NIX_TEST_UNIT_DATA`.
+The data is in `tests/unit/data`, with one subdir per library, with the same name as where the code goes.
+For example, `libnixstore` code is in `src/libstore`, and its test data is in `tests/unit/data/libstore`.
+The path to the `tests/unit/data` directory is passed to the unit test executable with the environment variable `_NIX_TEST_UNIT_DATA`.
 
 > **Note**
 > Due to the way googletest works, downstream unit test executables will actually include and re-run upstream library tests.
 > Therefore it is important that the same value for `_NIX_TEST_UNIT_DATA` be used with the tests for each library.
-> That is why we have the test data nested within a single `unit-test-data` directory.
+> That is why we have the test data nested within a single `tests/unit/data` directory.
+
+### Rationale
+
+The use of a separate directory for the unit tests might seem inconvenient, as the tests are not "right next to" the part of the code they are testing.
+But organizing the tests this way has one big benefit:
+there is no risk of any build-system wildcards for the library accidentally picking up test code that shoud not built and installed as part of the library.
+
+Likewise, the use of the `test/` subdir might seem superfluous:
+Isn't the point of the `*-test` subdir to indicate that these files are tests?
+Why do we need another `test` subdirectory?
+The answer is that we need to be able to tell apart the two headers, and make sure we include the right one.
+For `.cc` files, this is matter of doing `#include "foo.hh"` vs `#include "tests/foo.hh`
+For `.hh` files, this is a bit more subtle, because a `#include "foo.hh` instead `test/foo.hh` will end up including itself because `#include "..."`
+[always prioritizes files in the same directory](https://pubs.opengroup.org/onlinepubs/9699919799/utilities/c99.html#tag_20_11_04), rather than the original header from the library.
+Instead, use `#include <foo.hh>` to get the original library header, and `#include "tests/foo.hh"` to get the test header.
+
+Why do we have test headers at all?
+These are usually for [rapidcheck]'s `Arbitrary` machinery, which is used to describe how to generate values of a given type for the sake of running property tests.
+Because types contain other types, `Arbitrary` "instances" for some type are not just useful for testing that type, but also any other type that contains it.
+Indeed, if we don't reuse the upstream type's `Arbitrary` instance, the downstream type's `Arbitrary` instance would become much more complex and hard to understand.
+In order to reuse these instances, we therefore declare them in these testing headers.
 
 ### Running tests
 

flake.nix

@@ -91,7 +91,7 @@
           ./misc
           ./precompiled-headers.h
           ./src
-          ./unit-test-data
+          ./tests/unit
           ./COPYING
           ./scripts/local.mk
           functionalTestFiles

mk/common-test.sh

@@ -1,7 +1,7 @@
 # Remove overall test dir (at most one of the two should match) and
 # remove file extension.
 test_name=$(echo -n "$test" | sed \
-    -e "s|^unit-test-data/||" \
+    -e "s|^tests/unit/data/||" \
     -e "s|^tests/functional/||" \
     -e "s|\.sh$||" \
     )

unit-test-data/libutil/git/check-data.sh → tests/unit/data/libutil/git/check-data.sh

@@ -2,7 +2,7 @@
 
 set -eu -o pipefail
 
-export TEST_ROOT=$(realpath ${TMPDIR:-/tmp}/nix-test)/git-hashing/unit-test-data
+export TEST_ROOT=$(realpath ${TMPDIR:-/tmp}/nix-test)/git-hashing/check-data
 mkdir -p $TEST_ROOT
 
 repo="$TEST_ROOT/scratch"

tests/unit/libexpr/local.mk

@@ -0,0 +1,28 @@
+check: libexpr-tests_RUN
+
+programs += libexpr-tests
+
+libexpr-tests_NAME := libnixexpr-tests
+
+libexpr-tests_DIR := $(d)
+
+libexpr-tests_INSTALL_DIR :=
+
+libexpr-tests_SOURCES := \
+    $(wildcard $(d)/tests/*.cc) \
+    $(wildcard $(d)/tests/value/*.cc)
+
+libexpr-tests_CXXFLAGS += \
+    -I tests/unit/libexpr \
+    -I tests/unit/libstore \
+    -I tests/unit/libutil \
+    -I src/libexpr \
+    -I src/libfetchers \
+    -I src/libstore \
+    -I src/libutil
+
+libexpr-tests_LIBS = \
+    libstore-tests libutils-tests \
+    libexpr libfetchers libstore libutil
+
+libexpr-tests_LDFLAGS := $(GTEST_LIBS) -lgmock

src/libexpr/tests/value/context.hh → tests/unit/libexpr/tests/value/context.hh

@@ -3,7 +3,7 @@
 
 #include <rapidcheck/gen/Arbitrary.h>
 
-#include <value/context.hh>
+#include "value/context.hh"
 
 namespace rc {
 using namespace nix;

src/libstore/tests/local.mk → tests/unit/libstore/local.mk

@@ -20,10 +20,16 @@ libstore-tests_DIR := $(d)
 
 libstore-tests_INSTALL_DIR :=
 
-libstore-tests_SOURCES := $(wildcard $(d)/*.cc)
+libstore-tests_SOURCES := $(wildcard $(d)/tests/*.cc)
 
-libstore-tests_CXXFLAGS += -I src/libstore -I src/libutil
+libstore-tests_CXXFLAGS += \
+    -I tests/unit/libstore \
+    -I tests/unit/libutil \
+    -I src/libstore \
+    -I src/libutil
 
-libstore-tests_LIBS = libutil-tests libstore libutil
+libstore-tests_LIBS = \
+    libutil-tests \
+    libstore libutil
 
 libstore-tests_LDFLAGS := -lrapidcheck $(GTEST_LIBS)

src/libstore/tests/machines.cc → tests/unit/libstore/tests/machines.cc

@@ -139,7 +139,7 @@ TEST(machines, getMachinesWithIncorrectFormat) {
 }
 
 TEST(machines, getMachinesWithCorrectFileReference) {
-    auto path = absPath("src/libstore/tests/test-data/machines.valid");
+    auto path = absPath("tests/unit/libstore/test-data/machines.valid");
     ASSERT_TRUE(pathExists(path));
 
     settings.builders = std::string("@") + path;
@@ -166,6 +166,6 @@ TEST(machines, getMachinesWithIncorrectFileReference) {
 }
 
 TEST(machines, getMachinesWithCorrectFileReferenceToIncorrectFile) {
-    settings.builders = std::string("@") + absPath("src/libstore/tests/test-data/machines.bad_format");
+    settings.builders = std::string("@") + absPath("tests/unit/libstore/test-data/machines.bad_format");
     EXPECT_THROW(getMachines(), FormatError);
 }

src/libstore/tests/outputs-spec.cc → tests/unit/libstore/tests/outputs-spec.cc

@@ -1,4 +1,4 @@
-#include "outputs-spec.hh"
+#include "tests/outputs-spec.hh"
 
 #include <nlohmann/json.hpp>
 #include <gtest/gtest.h>

src/libstore/tests/outputs-spec.hh → tests/unit/libstore/tests/outputs-spec.hh

@@ -5,7 +5,7 @@
 
 #include <outputs-spec.hh>
 
-#include <tests/path.hh>
+#include "tests/path.hh"
 
 namespace rc {
 using namespace nix;

src/libutil/tests/local.mk → tests/unit/libutil/local.mk

@@ -20,14 +20,16 @@ libutil-tests_DIR := $(d)
 
 libutil-tests_INSTALL_DIR :=
 
-libutil-tests_SOURCES := $(wildcard $(d)/*.cc)
+libutil-tests_SOURCES := $(wildcard $(d)/tests/*.cc)
 
-libutil-tests_CXXFLAGS += -I src/libutil
+libutil-tests_CXXFLAGS += \
+    -I tests/unit/libutil \
+    -I src/libutil
 
 libutil-tests_LIBS = libutil
 
 libutil-tests_LDFLAGS := -lrapidcheck $(GTEST_LIBS)
 
-check: unit-test-data/libutil/git/check-data.sh.test
+check: tests/unit/data/libutil/git/check-data.sh.test
 
-$(eval $(call run-test,unit-test-data/libutil/git/check-data.sh))
+$(eval $(call run-test,tests/unit/data/libutil/git/check-data.sh))

src/libutil/tests/args.cc → tests/unit/libutil/tests/args.cc

@@ -1,5 +1,5 @@
-#include "../args.hh"
-#include "libutil/fs-sink.hh"
+#include "args.hh"
+#include "fs-sink.hh"
 #include <list>
 
 #include <gtest/gtest.h>
@@ -165,4 +165,4 @@ RC_GTEST_PROP(
 
 #endif
 
-}
+}

src/libutil/tests/characterization.hh → tests/unit/libutil/tests/characterization.hh

@@ -9,7 +9,7 @@
 namespace nix {
 
 /**
- * The path to the `unit-test-data` directory. See the contributing
+ * The path to the `tests/unit/data` directory. See the contributing
  * guide in the manual for further details.
  */
 static Path getUnitTestData() {

src/libutil/tests/git.cc → tests/unit/libutil/tests/git.cc

@@ -87,7 +87,7 @@ TEST_F(GitTest, blob_write) {
 /**
  * This data is for "shallow" tree tests. However, we use "real" hashes
  * so that we can check our test data in the corresponding functional
- * test (`git-hashing/unit-test-data`).
+ * test (`git-hashing/tests/unit/data`).
  */
 const static Tree tree = {
     {

src/libutil/tests/hash.cc → tests/unit/libutil/tests/hash.cc

@@ -4,7 +4,7 @@
 #include <gtest/gtest.h>
 #include <rapidcheck/gtest.h>
 
-#include <hash.hh>
+#include "hash.hh"
 
 #include "tests/hash.hh"