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.

Co-authored-by: Valentin Gagarin <[email protected]>

.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

@@ -27,9 +27,9 @@ endif
 ifeq ($(ENABLE_BUILD)_$(ENABLE_TESTS), yes_yes)
 UNIT_TEST_ENV = _NIX_TEST_UNIT_DATA=unit-test-data
 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
 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 \
+  src/libexpr-tests \
+  src/libexpr-tests/value \
+  src/libexpr-tests/test \
+  src/libexpr-tests/test/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/test \
   src/libutil \
-  src/libutil/tests \
+  src/libutil-tests \
+  src/libutil-tests/test \
   src/nix \
   src/nix-env \
   src/nix-store

doc/manual/src/contributing/testing.md

@@ -32,11 +32,15 @@ The unit tests are defined using the [googletest] and [rapidcheck] frameworks.
 > │   ├── value/context.cc
 > │   │
 > │   …
->     └── tests
-> │       ├── value/context.hh
-> │       ├── value/context.cc
-> │       │
-> │       …
+> │
+> ├── libexpr-tests
+> │   ├── test/
+> │   │   ├── value/context.hh
+> │   │   │
+> │   │   …
+> │   ├── value/context.cc
+> │   │
+> │   …
 > │
 > ├── unit-test-data
 > │   ├── libstore
@@ -46,7 +50,12 @@ The unit tests are defined using the [googletest] and [rapidcheck] frameworks.
 > …
 > ```
 
-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`.
@@ -57,6 +66,27 @@ The path to the `unit-test-data` directory is passed to the unit test executable
 > 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.
 
+### 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 "test/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 "test/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`.

src/libexpr/tests/derived-path.cc → src/libexpr-tests/derived-path.cc

@@ -2,8 +2,8 @@
 #include <gtest/gtest.h>
 #include <rapidcheck/gtest.h>
 
-#include "tests/derived-path.hh"
-#include "tests/libexpr.hh"
+#include "test/derived-path.hh"
+#include "test/libexpr.hh"
 
 namespace nix {
 

src/libexpr/tests/error_traces.cc → src/libexpr-tests/error_traces.cc

@@ -1,7 +1,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
-#include "tests/libexpr.hh"
+#include "test/libexpr.hh"
 
 namespace nix {
 

src/libexpr/tests/json.cc → src/libexpr-tests/json.cc

@@ -1,4 +1,4 @@
-#include "tests/libexpr.hh"
+#include "test/libexpr.hh"
 #include "value-to-json.hh"
 
 namespace nix {

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/primops.cc → src/libexpr-tests/primops.cc

@@ -1,7 +1,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
-#include "tests/libexpr.hh"
+#include "test/libexpr.hh"
 
 namespace nix {
     class CaptureLogger : public Logger

src/libexpr/tests/libexpr.hh → src/libexpr-tests/test/libexpr.hh

@@ -10,7 +10,7 @@
 #include "eval-inline.hh"
 #include "store-api.hh"
 
-#include "tests/libstore.hh"
+#include "test/libstore.hh"
 
 namespace nix {
     class LibExprTest : public LibStoreTest {

src/libexpr/tests/value/context.hh → src/libexpr-tests/test/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/libexpr/tests/value/print.cc → src/libexpr-tests/test/value/print.cc

@@ -1,4 +1,4 @@
-#include "tests/libexpr.hh"
+#include "test/libexpr.hh"
 
 #include "value.hh"
 

src/libexpr/tests/trivial.cc → src/libexpr-tests/trivial.cc

@@ -1,4 +1,4 @@
-#include "tests/libexpr.hh"
+#include "test/libexpr.hh"
 
 namespace nix {
     // Testing of trivial expressions

src/libexpr/tests/value/context.cc → src/libexpr-tests/value/context.cc

@@ -2,9 +2,9 @@
 #include <gtest/gtest.h>
 #include <rapidcheck/gtest.h>
 
-#include "tests/path.hh"
-#include "tests/libexpr.hh"
-#include "tests/value/context.hh"
+#include "test/path.hh"
+#include "test/libexpr.hh"
+#include "test/value/context.hh"
 
 namespace nix {
 

src/libstore/tests/common-protocol.cc → src/libstore-tests/common-protocol.cc

@@ -6,8 +6,8 @@
 #include "common-protocol.hh"
 #include "common-protocol-impl.hh"
 #include "build-result.hh"
-#include "tests/protocol.hh"
-#include "tests/characterization.hh"
+#include "test/protocol.hh"
+#include "test/characterization.hh"
 
 namespace nix {
 

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

@@ -4,7 +4,7 @@
 #include "experimental-features.hh"
 #include "derivations.hh"
 
-#include "tests/libstore.hh"
+#include "test/libstore.hh"
 
 namespace nix {
 

src/libstore/tests/derived-path.cc → src/libstore-tests/derived-path.cc

@@ -4,8 +4,8 @@
 #include <gtest/gtest.h>
 #include <rapidcheck/gtest.h>
 
-#include "tests/derived-path.hh"
-#include "tests/libstore.hh"
+#include "test/derived-path.hh"
+#include "test/libstore.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 "test/outputs-spec.hh"
 
 #include <nlohmann/json.hpp>
 #include <gtest/gtest.h>

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

@@ -7,9 +7,9 @@
 #include "path-regex.hh"
 #include "store-api.hh"
 
-#include "tests/hash.hh"
-#include "tests/libstore.hh"
-#include "tests/path.hh"
+#include "test/hash.hh"
+#include "test/libstore.hh"
+#include "test/path.hh"
 
 namespace nix {
 

src/libstore/tests/derived-path.hh → src/libstore-tests/test/derived-path.hh

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

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

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

src/libstore/tests/protocol.hh → src/libstore-tests/test/protocol.hh

@@ -1,8 +1,8 @@
 #include <nlohmann/json.hpp>
 #include <gtest/gtest.h>
 
-#include "tests/libstore.hh"
-#include "tests/characterization.hh"
+#include "test/libstore.hh"
+#include "test/characterization.hh"
 
 namespace nix {
 

src/libstore/tests/worker-protocol.cc → src/libstore-tests/worker-protocol.cc

@@ -7,8 +7,8 @@
 #include "worker-protocol-impl.hh"
 #include "derived-path.hh"
 #include "build-result.hh"
-#include "tests/protocol.hh"
-#include "tests/characterization.hh"
+#include "test/protocol.hh"
+#include "test/characterization.hh"
 
 namespace nix {
 

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

@@ -4,9 +4,9 @@
 #include <gtest/gtest.h>
 #include <rapidcheck/gtest.h>
 
-#include <hash.hh>
+#include "hash.hh"
 
-#include "tests/hash.hh"
+#include "test/hash.hh"
 
 namespace nix {
 

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