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 at
least regular `*.cc` files for tests do not.

.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
+/src/libexpr-tests/libnixexpr-tests
 
 # /src/libstore/
 *.gen.*
-/src/libstore/tests/libnixstore-tests
+/src/libstore-tests/libnixstore-tests
 
 # /src/libutil/
-/src/libutil/tests/libnixutil-tests
+/src/libutil-tests/libnixutil-tests
 
 /src/nix/nix
 

Makefile

@@ -23,9 +23,9 @@ makefiles = \
 
 ifeq ($(tests), yes)
 makefiles += \
-  src/libutil/tests/local.mk \
-  src/libstore/tests/local.mk \
-  src/libexpr/tests/local.mk \
+  src/libutil-tests/local.mk \
+  src/libstore-tests/local.mk \
+  src/libexpr-tests/local.mk \
   tests/local.mk \
   tests/ca/local.mk \
   tests/dyn-drv/local.mk \

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 \
+  src/libexpr-tests \
+  src/libexpr-tests/value \
+  src/libexpr-tests/tests \
+  src/libexpr-tests/tests/value \
   src/libexpr/value \
   src/libfetchers \
   src/libmain \
   src/libstore \
   src/libstore/build \
   src/libstore/builtins \
-  src/libstore/tests \
+  src/libstore-tests \
+  src/libstore-tests/tests \
   src/libutil \
-  src/libutil/tests \
+  src/libutil-tests \
+  src/libutil-tests/tests \
   src/nix \
   src/nix-env \
   src/nix-store

doc/manual/src/contributing/testing.md

@@ -2,10 +2,57 @@
 
 ## Unit-tests
 
-The unit-tests for each Nix library (`libexpr`, `libstore`, etc..) are defined
-under `src/{library_name}/tests` using the
-[googletest](https://google.github.io/googletest/) and
-[rapidcheck](https://github.com/emil-e/rapidcheck) frameworks.
+The unit-tests for defined using the [googletest] and [rapidcheck] frameworks.
+
+[googletest]: https://google.github.io/googletest/
+[rapidcheck]: https://github.com/emil-e/rapidcheck
+
+### Source and header layout
+
+> An example for `libnixexpr`, demonstrating much of what is described below
+>
+> ```
+> src
+> ├── libexpr
+> │   ├── value/context.hh
+> │   ├── value/context.cc
+> │   │
+> │   …
+> ├── libexpr-tests
+> │   ├── tests/
+> │   │   ├── value/context.hh
+> │   │   │
+> │   │   …
+> │   ├── value/context.cc
+> │   │
+> │   …
+> …
+> ```
+
+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, `value/context.{hh,cc}`, we write tests for it in `value/context.cc`, and (possibly) declare additional interfaces for testing purposes in `test/value/context.hh`.
+(Note only the extra header is nested inside the `test` directory. This is explained further below.)
+
+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` subdir too?
+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 `tests/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
 
 You can run the whole testsuite with `make check`, or the tests for a specific component with `make libfoo-tests_RUN`.
 Finer-grained filtering is also possible using the [--gtest_filter](https://google.github.io/googletest/advanced.html#running-a-subset-of-the-tests) command-line option, or the `GTEST_FILTER` environment variable.

src/libexpr-tests/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)/*.cc) \
+    $(wildcard $(d)/value/*.cc)
+
+libexpr-tests_CXXFLAGS += \
+    -I src/libexpr-tests \
+    -I src/libstore-tests \
+    -I src/libutil-tests \
+    -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 → src/libexpr-tests/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 → src/libstore-tests/local.mk

@@ -22,8 +22,14 @@ libstore-tests_INSTALL_DIR :=
 
 libstore-tests_SOURCES := $(wildcard $(d)/*.cc)
 
-libstore-tests_CXXFLAGS += -I src/libstore -I src/libutil
-
-libstore-tests_LIBS = libutil-tests libstore libutil
+libstore-tests_CXXFLAGS += \
+    -I src/libstore-tests \
+    -I src/libutil-tests \
+    -I src/libstore \
+    -I src/libutil
+
+libstore-tests_LIBS = \
+    libutil-tests \
+    libstore libutil
 
 libstore-tests_LDFLAGS := -lrapidcheck $(GTEST_LIBS)

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

@@ -137,7 +137,7 @@ TEST(machines, getMachinesWithIncorrectFormat) {
 }
 
 TEST(machines, getMachinesWithCorrectFileReference) {
-    auto path = absPath("src/libstore/tests/test-data/machines.valid");
+    auto path = absPath("src/libstore-tests/test-data/machines.valid");
     ASSERT_TRUE(pathExists(path));
 
     settings.builders = std::string("@") + path;
@@ -164,6 +164,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("src/libstore-tests/test-data/machines.bad_format");
     EXPECT_THROW(getMachines(), FormatError);
 }

src/libstore/tests/outputs-spec.cc → src/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 → src/libstore-tests/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/hash.cc → src/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"
 

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

@@ -22,7 +22,9 @@ libutil-tests_INSTALL_DIR :=
 
 libutil-tests_SOURCES := $(wildcard $(d)/*.cc)
 
-libutil-tests_CXXFLAGS += -I src/libutil
+libutil-tests_CXXFLAGS += \
+    -I src/libutil-tests \
+    -I src/libutil
 
 libutil-tests_LIBS = libutil