From 7fe308c2f840325199f49526b257bbe4ded5a909 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Jan 2023 08:51:00 -0500 Subject: [PATCH 1/3] Add `rapidcheck` dependency for testing Property tests are great! Co-authored-by: Cole Helbling --- configure.ac | 6 ++++++ doc/manual/src/contributing/hacking.md | 3 ++- flake.nix | 5 ++++- src/libstore/tests/local.mk | 2 +- 4 files changed, 13 insertions(+), 3 deletions(-) diff --git a/configure.ac b/configure.ac index 1b0d6fd27..0066bc389 100644 --- a/configure.ac +++ b/configure.ac @@ -274,6 +274,12 @@ fi PKG_CHECK_MODULES([GTEST], [gtest_main]) +# Look for rapidcheck. +# No pkg-config yet, https://github.com/emil-e/rapidcheck/issues/302 +AC_CHECK_HEADERS([rapidcheck/gtest.h], [], [], [#include ]) +AC_CHECK_LIB([rapidcheck], []) + + # Look for nlohmann/json. PKG_CHECK_MODULES([NLOHMANN_JSON], [nlohmann_json >= 3.9]) diff --git a/doc/manual/src/contributing/hacking.md b/doc/manual/src/contributing/hacking.md index aeb0d41b3..9dbafcc0a 100644 --- a/doc/manual/src/contributing/hacking.md +++ b/doc/manual/src/contributing/hacking.md @@ -92,7 +92,8 @@ $ nix develop 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/) framework. +[googletest](https://google.github.io/googletest/) and +[rapidcheck](https://github.com/emil-e/rapidcheck) frameworks. 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. diff --git a/flake.nix b/flake.nix index 573373c42..5796c54ea 100644 --- a/flake.nix +++ b/flake.nix @@ -82,7 +82,9 @@ }); configureFlags = - lib.optionals stdenv.isLinux [ + [ + "CXXFLAGS=-I${lib.getDev rapidcheck}/extras/gtest/include" + ] ++ lib.optionals stdenv.isLinux [ "--with-boost=${boost}/lib" "--with-sandbox-shell=${sh}/bin/busybox" ] @@ -116,6 +118,7 @@ boost lowdown-nix gtest + rapidcheck ] ++ lib.optionals stdenv.isLinux [libseccomp] ++ lib.optional (stdenv.isLinux || stdenv.isDarwin) libsodium diff --git a/src/libstore/tests/local.mk b/src/libstore/tests/local.mk index f74295d97..a2cf8a0cf 100644 --- a/src/libstore/tests/local.mk +++ b/src/libstore/tests/local.mk @@ -12,4 +12,4 @@ libstore-tests_CXXFLAGS += -I src/libstore -I src/libutil libstore-tests_LIBS = libstore libutil -libstore-tests_LDFLAGS := $(GTEST_LIBS) +libstore-tests_LDFLAGS := -lrapidcheck $(GTEST_LIBS) From 685395332d75713bc7aca0c6408fc1b9d2c14bc5 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Jan 2023 08:49:44 -0500 Subject: [PATCH 2/3] Better-scope `Store` forward declarations --- src/libstore/derivations.hh | 1 + src/libstore/path.hh | 1 - src/libstore/realisation.hh | 2 ++ 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index 7ee3ded6a..f42c13cdc 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -13,6 +13,7 @@ namespace nix { +class Store; /* Abstract syntax of derivations. */ diff --git a/src/libstore/path.hh b/src/libstore/path.hh index 0694b4c18..8e1cb5e55 100644 --- a/src/libstore/path.hh +++ b/src/libstore/path.hh @@ -5,7 +5,6 @@ namespace nix { -class Store; struct Hash; class StorePath diff --git a/src/libstore/realisation.hh b/src/libstore/realisation.hh index 911c61909..62561fce3 100644 --- a/src/libstore/realisation.hh +++ b/src/libstore/realisation.hh @@ -7,6 +7,8 @@ namespace nix { +class Store; + struct DrvOutput { // The hash modulo of the derivation Hash drvHash; From 018e2571aad8c68c80207f84b6b20695f20e5c40 Mon Sep 17 00:00:00 2001 From: John Ericson Date: Thu, 19 Jan 2023 00:26:06 -0500 Subject: [PATCH 3/3] Test store paths, with property tests The property test in fact found a bug: we were excluding numbers! --- src/libstore/outputs-spec.cc | 17 ++-- src/libstore/path-regex.hh | 7 ++ src/libstore/path.cc | 6 +- src/libstore/path.hh | 2 + src/libstore/tests/libstoretests.hh | 23 +++++ src/libstore/tests/outputs-spec.cc | 7 ++ src/libstore/tests/path.cc | 144 ++++++++++++++++++++++++++++ src/libutil/regex-combinators.hh | 30 ++++++ 8 files changed, 228 insertions(+), 8 deletions(-) create mode 100644 src/libstore/path-regex.hh create mode 100644 src/libstore/tests/libstoretests.hh create mode 100644 src/libstore/tests/path.cc create mode 100644 src/libutil/regex-combinators.hh diff --git a/src/libstore/outputs-spec.cc b/src/libstore/outputs-spec.cc index 096443cb2..e26c38138 100644 --- a/src/libstore/outputs-spec.cc +++ b/src/libstore/outputs-spec.cc @@ -1,8 +1,10 @@ -#include "util.hh" -#include "outputs-spec.hh" -#include "nlohmann/json.hpp" - #include +#include + +#include "util.hh" +#include "regex-combinators.hh" +#include "outputs-spec.hh" +#include "path-regex.hh" namespace nix { @@ -18,11 +20,14 @@ bool OutputsSpec::contains(const std::string & outputName) const }, raw()); } +static std::string outputSpecRegexStr = + regex::either( + regex::group(R"(\*)"), + regex::group(regex::list(nameRegexStr))); std::optional OutputsSpec::parseOpt(std::string_view s) { - // See checkName() for valid output name characters. - static std::regex regex(R"((\*)|([a-zA-Z\+\-\._\?=]+(,[a-zA-Z\+\-\._\?=]+)*))"); + static std::regex regex(std::string { outputSpecRegexStr }); std::smatch match; std::string s2 { s }; // until some improves std::regex diff --git a/src/libstore/path-regex.hh b/src/libstore/path-regex.hh new file mode 100644 index 000000000..6893c3876 --- /dev/null +++ b/src/libstore/path-regex.hh @@ -0,0 +1,7 @@ +#pragma once + +namespace nix { + +static constexpr std::string_view nameRegexStr = R"([0-9a-zA-Z\+\-\._\?=]+)"; + +} diff --git a/src/libstore/path.cc b/src/libstore/path.cc index 392db225e..46be54281 100644 --- a/src/libstore/path.cc +++ b/src/libstore/path.cc @@ -8,8 +8,10 @@ static void checkName(std::string_view path, std::string_view name) { if (name.empty()) throw BadStorePath("store path '%s' has an empty name", path); - if (name.size() > 211) - throw BadStorePath("store path '%s' has a name longer than 211 characters", path); + if (name.size() > StorePath::MaxPathLen) + throw BadStorePath("store path '%s' has a name longer than '%d characters", + StorePath::MaxPathLen, path); + // See nameRegexStr for the definition for (auto c : name) if (!((c >= '0' && c <= '9') || (c >= 'a' && c <= 'z') diff --git a/src/libstore/path.hh b/src/libstore/path.hh index 8e1cb5e55..6a8f027f9 100644 --- a/src/libstore/path.hh +++ b/src/libstore/path.hh @@ -16,6 +16,8 @@ public: /* Size of the hash part of store paths, in base-32 characters. */ constexpr static size_t HashLen = 32; // i.e. 160 bits + constexpr static size_t MaxPathLen = 211; + StorePath() = delete; StorePath(std::string_view baseName); diff --git a/src/libstore/tests/libstoretests.hh b/src/libstore/tests/libstoretests.hh new file mode 100644 index 000000000..05397659b --- /dev/null +++ b/src/libstore/tests/libstoretests.hh @@ -0,0 +1,23 @@ +#include +#include + +#include "store-api.hh" + +namespace nix { + +class LibStoreTest : public ::testing::Test { + public: + static void SetUpTestSuite() { + initLibStore(); + } + + protected: + LibStoreTest() + : store(openStore("dummy://")) + { } + + ref store; +}; + + +} /* namespace nix */ diff --git a/src/libstore/tests/outputs-spec.cc b/src/libstore/tests/outputs-spec.cc index 836ba7e82..06e4cabbd 100644 --- a/src/libstore/tests/outputs-spec.cc +++ b/src/libstore/tests/outputs-spec.cc @@ -47,6 +47,13 @@ TEST(OutputsSpec, names_underscore) { ASSERT_EQ(expected.to_string(), str); } +TEST(OutputsSpec, names_numberic) { + std::string_view str = "01"; + OutputsSpec expected = OutputsSpec::Names { "01" }; + ASSERT_EQ(OutputsSpec::parse(str), expected); + ASSERT_EQ(expected.to_string(), str); +} + TEST(OutputsSpec, names_out_bin) { OutputsSpec expected = OutputsSpec::Names { "out", "bin" }; ASSERT_EQ(OutputsSpec::parse("out,bin"), expected); diff --git a/src/libstore/tests/path.cc b/src/libstore/tests/path.cc new file mode 100644 index 000000000..8ea252c92 --- /dev/null +++ b/src/libstore/tests/path.cc @@ -0,0 +1,144 @@ +#include + +#include +#include +#include + +#include "path-regex.hh" +#include "store-api.hh" + +#include "libstoretests.hh" + +namespace nix { + +#define STORE_DIR "/nix/store/" +#define HASH_PART "g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q" + +class StorePathTest : public LibStoreTest +{ +}; + +static std::regex nameRegex { std::string { nameRegexStr } }; + +#define TEST_DONT_PARSE(NAME, STR) \ + TEST_F(StorePathTest, bad_ ## NAME) { \ + std::string_view str = \ + STORE_DIR HASH_PART "-" STR; \ + ASSERT_THROW( \ + store->parseStorePath(str), \ + BadStorePath); \ + std::string name { STR }; \ + EXPECT_FALSE(std::regex_match(name, nameRegex)); \ + } + +TEST_DONT_PARSE(empty, "") +TEST_DONT_PARSE(garbage, "&*()") +TEST_DONT_PARSE(double_star, "**") +TEST_DONT_PARSE(star_first, "*,foo") +TEST_DONT_PARSE(star_second, "foo,*") +TEST_DONT_PARSE(bang, "foo!o") + +#undef TEST_DONT_PARSE + +#define TEST_DO_PARSE(NAME, STR) \ + TEST_F(StorePathTest, good_ ## NAME) { \ + std::string_view str = \ + STORE_DIR HASH_PART "-" STR; \ + auto p = store->parseStorePath(str); \ + std::string name { p.name() }; \ + EXPECT_TRUE(std::regex_match(name, nameRegex)); \ + } + +// 0-9 a-z A-Z + - . _ ? = + +TEST_DO_PARSE(numbers, "02345") +TEST_DO_PARSE(lower_case, "foo") +TEST_DO_PARSE(upper_case, "FOO") +TEST_DO_PARSE(plus, "foo+bar") +TEST_DO_PARSE(dash, "foo-dev") +TEST_DO_PARSE(underscore, "foo_bar") +TEST_DO_PARSE(period, "foo.txt") +TEST_DO_PARSE(question_mark, "foo?why") +TEST_DO_PARSE(equals_sign, "foo=foo") + +#undef TEST_DO_PARSE + +// For rapidcheck +void showValue(const StorePath & p, std::ostream & os) { + os << p.to_string(); +} + +} + +namespace rc { +using namespace nix; + +template<> +struct Arbitrary { + static Gen arbitrary(); +}; + +Gen Arbitrary::arbitrary() +{ + auto len = *gen::inRange(1, StorePath::MaxPathLen); + + std::string pre { HASH_PART "-" }; + pre.reserve(pre.size() + len); + + for (size_t c = 0; c < len; ++c) { + switch (auto i = *gen::inRange(0, 10 + 2 * 26 + 6)) { + case 0 ... 9: + pre += '0' + i; + case 10 ... 35: + pre += 'A' + (i - 10); + break; + case 36 ... 61: + pre += 'a' + (i - 36); + break; + case 62: + pre += '+'; + break; + case 63: + pre += '-'; + break; + case 64: + pre += '.'; + break; + case 65: + pre += '_'; + break; + case 66: + pre += '?'; + break; + case 67: + pre += '='; + break; + default: + assert(false); + } + } + + return gen::just(StorePath { pre }); +} + +} // namespace rc + +namespace nix { + +RC_GTEST_FIXTURE_PROP( + StorePathTest, + prop_regex_accept, + (const StorePath & p)) +{ + RC_ASSERT(std::regex_match(std::string { p.name() }, nameRegex)); +} + +RC_GTEST_FIXTURE_PROP( + StorePathTest, + prop_round_rip, + (const StorePath & p)) +{ + RC_ASSERT(p == store->parseStorePath(store->printStorePath(p))); +} + +} diff --git a/src/libutil/regex-combinators.hh b/src/libutil/regex-combinators.hh new file mode 100644 index 000000000..0b997b25a --- /dev/null +++ b/src/libutil/regex-combinators.hh @@ -0,0 +1,30 @@ +#pragma once + +#include + +namespace nix::regex { + +// TODO use constexpr string building like +// https://github.com/akrzemi1/static_string/blob/master/include/ak_toolkit/static_string.hpp + +static inline std::string either(std::string_view a, std::string_view b) +{ + return std::string { a } + "|" + b; +} + +static inline std::string group(std::string_view a) +{ + return std::string { "(" } + a + ")"; +} + +static inline std::string many(std::string_view a) +{ + return std::string { "(?:" } + a + ")*"; +} + +static inline std::string list(std::string_view a) +{ + return std::string { a } + many(group("," + a)); +} + +}