diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f5dc8a6..e866aee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,6 +44,49 @@ jobs: - name: Test run: ctest --preset ${{ matrix.preset }} -j "$(nproc 2>/dev/null || sysctl -n hw.ncpu)" + # MSVC, against the conda-forge Arrow for win-64. The Visual Studio generator is + # multi-config, so the preset names the configuration at build and test time + # rather than through CMAKE_BUILD_TYPE; conda puts every library under + # Library/, which is what the preset's prefix path points at. cmd rather than + # bash so that CONDA_PREFIX reaches CMake as a Windows path. The build preset + # names its targets so that the build never enters the RUN_TESTS project + # enable_testing() defines, which runs ctest and so would test the tree from + # inside the build that is meant to produce it. Those names are CMake target + # names, not file names: MSBuild matches them against the generated project + # files, which are case-insensitive here, so a target called run_tests would + # select RUN_TESTS. + windows: + name: msvc on windows-latest + runs-on: windows-latest + defaults: + run: + shell: cmd /C call {0} + steps: + - uses: actions/checkout@v4 + + - uses: mamba-org/setup-micromamba@v2 + with: + environment-file: environment.yml + init-shell: cmd.exe + cache-environment: true + + - name: Configure + run: cmake --preset windows + + - name: Build + run: cmake --build --preset windows -j %NUMBER_OF_PROCESSORS% + + - name: Test + run: ctest --preset windows -j %NUMBER_OF_PROCESSORS% + + - name: Install the package + run: >- + pip install -e . --no-build-isolation + -Ccmake.define.CMAKE_PREFIX_PATH=%CONDA_PREFIX%/Library + + - name: Test the package + run: python -m pytest python/tests + python: name: python on ${{ matrix.os }} runs-on: ${{ matrix.os }} @@ -116,7 +159,7 @@ jobs: - name: clang-format run: >- clang-format --dry-run --Werror - src/main.cpp src/cpplink/* tests/*.cpp python/bindings/* + src/main.cpp src/cpplink/* tests/*.cpp tests/*.hpp python/bindings/* - name: cpplint run: cpplint --recursive src tests python/bindings diff --git a/CMakeLists.txt b/CMakeLists.txt index a9cec5c..6bb7cfe 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -99,16 +99,26 @@ if(BUILD_TESTING) include(GoogleTest) file(GLOB TEST_SOURCES CONFIGURE_DEPENDS tests/*.cpp) - add_executable(run_tests ${TEST_SOURCES}) - target_link_libraries(run_tests PRIVATE cpplink_core GTest::gtest_main) + # The target is cpplink_tests and the binary is run_tests: enable_testing() + # defines a target called RUN_TESTS, and the Visual Studio generator names a + # project file after its target, so a target called run_tests and that one + # are the same file on a case-insensitive filesystem. Whichever is written + # second wins, and building the test executable then runs ctest against a + # tree with no test executable in it. + add_executable(cpplink_tests ${TEST_SOURCES}) + set_target_properties(cpplink_tests PROPERTIES OUTPUT_NAME run_tests) + target_link_libraries(cpplink_tests PRIVATE cpplink_core GTest::gtest_main) + # Test-only headers are included as "tests/x.hpp", rooted at the project + # directory, the way the library's are rooted at src/. + target_include_directories(cpplink_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) # A test reading a checked-in fixture names it from the source tree, so the # suite passes from ctest's working directory as well as from the root. - target_compile_definitions(run_tests PRIVATE + target_compile_definitions(cpplink_tests PRIVATE CPPLINK_SOURCE_DIR="${CMAKE_CURRENT_SOURCE_DIR}") # Arrow pulls in a large shared library graph; discovery needs longer than the # 5 s default to load it. - gtest_discover_tests(run_tests DISCOVERY_TIMEOUT 120) + gtest_discover_tests(cpplink_tests DISCOVERY_TIMEOUT 120) endif() # clang-format target diff --git a/CMakePresets.json b/CMakePresets.json index d4ccdf8..d78d595 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -42,13 +42,27 @@ "CMAKE_BUILD_TYPE": "RelWithDebInfo", "CPPLINK_SANITIZE": "thread" } + }, + { + "name": "windows", + "displayName": "MSVC x64, with tests", + "inherits": "base", + "generator": "Visual Studio 18 2026", + "architecture": {"value": "x64", "strategy": "set"}, + "cacheVariables": {"CMAKE_PREFIX_PATH": "$env{CONDA_PREFIX}/Library"} } ], "buildPresets": [ {"name": "release", "configurePreset": "release"}, {"name": "debug", "configurePreset": "debug"}, {"name": "asan", "configurePreset": "asan"}, - {"name": "tsan", "configurePreset": "tsan"} + {"name": "tsan", "configurePreset": "tsan"}, + { + "name": "windows", + "configurePreset": "windows", + "configuration": "Release", + "targets": ["cpplink", "cpplink_tests"] + } ], "testPresets": [ { @@ -73,6 +87,12 @@ "inherits": "base", "configurePreset": "tsan", "environment": {"TSAN_OPTIONS": "halt_on_error=1:second_deadlock_stack=1"} + }, + { + "name": "windows", + "inherits": "base", + "configurePreset": "windows", + "configuration": "Release" } ] } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 35132e4..88e129e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -36,9 +36,10 @@ Two of them run the suite under a sanitizer, which is what checks the claim that ```sh cmake --preset asan && cmake --build --preset asan && ctest --preset asan # ASan + UBSan, about 30 s cmake --preset tsan && cmake --build --preset tsan && ctest --preset tsan # ThreadSanitizer, about 5 min +cmake --preset windows && cmake --build --preset windows && ctest --preset windows # MSVC, from an activated conda prompt ``` -Both are clean over the whole suite. +The two sanitizer presets are clean over the whole suite; `windows` is the same suite under MSVC, and CI runs it on `windows-latest`. CI ([.github/workflows/ci.yml](.github/workflows/ci.yml)) runs `release`, `asan` and `tsan` on Linux and macOS, then the Python suite and the format and lint checks below. ## The Python package diff --git a/docs/getting-started.md b/docs/getting-started.md index 5d7c5fc..dc85096 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -42,7 +42,8 @@ ctest --preset asan ``` `release` and `debug` are the plain builds with tests on, and `ctest --preset ` runs the suite with output on failure. -The GitHub Actions workflow runs `release`, `asan` and `tsan` on Linux and macOS, then the Python suite and the format and lint checks. +On Windows the `windows` preset builds with Visual Studio 2026 against the conda-forge Arrow, which lives under `%CONDA_PREFIX%\Library`, and the build and test presets of the same name select the `Release` configuration, since the Visual Studio generator holds every configuration in one tree. +The GitHub Actions workflow runs `release`, `asan` and `tsan` on Linux and macOS and `windows` on Windows, then the Python suite and the format and lint checks. ## The whole pipeline in seven commands diff --git a/python/bindings/stages.cpp b/python/bindings/stages.cpp index 76ce502..03c5a4d 100644 --- a/python/bindings/stages.cpp +++ b/python/bindings/stages.cpp @@ -332,7 +332,7 @@ MergeOutcome MergePredictions(const std::string& shards, const std::string& out, options.out_path = out; options.batch_rows = batch_rows; if (format.is_none()) { - const std::string suffix = std::filesystem::path(out).extension(); + const std::string suffix = std::filesystem::path(out).extension().string(); if (suffix == ".parquet" || suffix == ".pq") options.format = MergeFormat::kParquet; } else { diff --git a/src/cpplink/app.cpp b/src/cpplink/app.cpp index 23578de..0be8670 100644 --- a/src/cpplink/app.cpp +++ b/src/cpplink/app.cpp @@ -14,8 +14,6 @@ #include #include -#include -#include #include "cpplink/blocking.hpp" #include "cpplink/cluster.hpp" @@ -44,6 +42,25 @@ #include "cpplink/simplify.hpp" #include "cpplink/waterfall.hpp" +// The terminal query below is the one platform-specific call. windows.h comes after +// every project header so that its macros -- and it defines several hundred, from +// ERROR to far -- rewrite nothing the project declares. +#ifdef _WIN32 +#ifndef NOMINMAX +#define NOMINMAX // windows.h's min/max macros would break std::min below. +#endif +#ifndef WIN32_LEAN_AND_MEAN +#define WIN32_LEAN_AND_MEAN +#endif +#include + +#include +#include +#else +#include +#include +#endif + namespace cpplink { const char* const kVersion = "0.1.0"; @@ -910,10 +927,19 @@ int RunExplain(const std::vector& args, std::ostream& out, // string stream -- it is printed as lines. The stream is compared to the process's // own stderr because an ostream carries no descriptor to ask. unsigned TerminalColumns(const std::ostream& stream) { - if (&stream != &std::cerr || isatty(STDERR_FILENO) == 0) return 0; + if (&stream != &std::cerr) return 0; +#ifdef _WIN32 + if (_isatty(_fileno(stderr)) == 0) return 0; + CONSOLE_SCREEN_BUFFER_INFO info{}; + if (GetConsoleScreenBufferInfo(GetStdHandle(STD_ERROR_HANDLE), &info) == 0) return 80; + const int columns = info.srWindow.Right - info.srWindow.Left + 1; + return columns > 0 ? static_cast(columns) : 80; +#else + if (isatty(STDERR_FILENO) == 0) return 0; winsize size{}; if (ioctl(STDERR_FILENO, TIOCGWINSZ, &size) != 0 || size.ws_col == 0) return 80; return size.ws_col; +#endif } int RunExplainBlocking(const std::vector& args, std::ostream& out, @@ -1625,7 +1651,8 @@ int RunMergeEdges(const std::vector& args, std::ostream& out, // The extension is what a user means by the format; --format is for a name // that does not carry one. if (!format_given) { - const std::string suffix = std::filesystem::path(options.out_path).extension(); + const std::string suffix = + std::filesystem::path(options.out_path).extension().string(); if (suffix == ".parquet" || suffix == ".pq") { options.format = MergeFormat::kParquet; } diff --git a/src/cpplink/dictionary.hpp b/src/cpplink/dictionary.hpp index 0884701..610950e 100644 --- a/src/cpplink/dictionary.hpp +++ b/src/cpplink/dictionary.hpp @@ -23,6 +23,12 @@ inline constexpr uint32_t kNullId = 0xFFFFFFFFu; // by the index stay valid as the arena grows. class Dictionary { public: + Dictionary() = default; + Dictionary(const Dictionary&) = delete; + Dictionary& operator=(const Dictionary&) = delete; + Dictionary(Dictionary&&) = default; + Dictionary& operator=(Dictionary&&) = default; + uint32_t Intern(std::string_view value); std::string_view Value(uint32_t id) const { return entries_[id]; } uint32_t Size() const { return static_cast(entries_.size()); } diff --git a/src/cpplink/inspect.cpp b/src/cpplink/inspect.cpp index 789ed9f..64e4d31 100644 --- a/src/cpplink/inspect.cpp +++ b/src/cpplink/inspect.cpp @@ -4,6 +4,7 @@ #include "cpplink/inspect.hpp" #include +#include #include #include #include diff --git a/src/cpplink/schema.cpp b/src/cpplink/schema.cpp index b8ce078..0a0dada 100644 --- a/src/cpplink/schema.cpp +++ b/src/cpplink/schema.cpp @@ -3,6 +3,7 @@ #include "cpplink/schema.hpp" +#include #include #include #include diff --git a/src/cpplink/string_metrics.cpp b/src/cpplink/string_metrics.cpp index f7cc2f5..19b6737 100644 --- a/src/cpplink/string_metrics.cpp +++ b/src/cpplink/string_metrics.cpp @@ -10,6 +10,10 @@ #include #include +#ifdef _MSC_VER +#include +#endif + namespace cpplink { namespace { @@ -56,8 +60,30 @@ double ApplyWinkler(std::string_view a, std::string_view b, double jaro, // The one instruction, asked for by name. `std::bitset<64>::count()` does not // lower to it here -- it leaves a call to the generic bit-iterator count in the -// binary, which a profile of the scoring loop finds among the hot leaves. -int PopCount(uint64_t value) { return __builtin_popcountll(value); } +// binary, which a profile of the scoring loop finds among the hot leaves. MSVC's +// `__popcnt64` emits the instruction with no fallback, so an x64 build assumes +// POPCNT, which every processor Windows 11 runs on has; gcc and clang substitute +// a bit-trick sequence where the target lacks it. +int PopCount(uint64_t value) { +#if defined(_MSC_VER) && defined(_M_ARM64) + return static_cast(_CountOneBits64(value)); +#elif defined(_MSC_VER) + return static_cast(__popcnt64(value)); +#else + return __builtin_popcountll(value); +#endif +} + +// The index of the lowest set bit; `value` must not be zero. +int TrailingZeros(uint64_t value) { +#ifdef _MSC_VER + unsigned long index = 0; // NOLINT(runtime/int): the intrinsic's own type + _BitScanForward64(&index, value); + return static_cast(index); +#else + return __builtin_ctzll(value); +#endif +} // Myers' bit-vector edit distance (1999). The DP's column of vertical deltas is // carried in two words -- vp for +1, vn for -1 -- so a whole column costs a dozen @@ -187,7 +213,7 @@ double JaroWords(std::string_view a, std::string_view b, double screen) { uint64_t left = claimed; uint64_t right = taken; while (left != 0) { - if (a[__builtin_ctzll(left)] != b[__builtin_ctzll(right)]) ++transpositions; + if (a[TrailingZeros(left)] != b[TrailingZeros(right)]) ++transpositions; left &= left - 1; right &= right - 1; } diff --git a/tests/arrow_export_test.cpp b/tests/arrow_export_test.cpp index e53017b..61e2144 100644 --- a/tests/arrow_export_test.cpp +++ b/tests/arrow_export_test.cpp @@ -19,12 +19,13 @@ #include #include #include -#include #include "cpplink/batch_loader.hpp" #include "cpplink/parquet_io.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -223,7 +224,7 @@ TEST(ArrowExport, ABatchCanBeReusedAfterExport) { // same batches, through nothing but the C structs on either side. TEST(ArrowExport, ParquetRoundTrip) { const auto dir = std::filesystem::temp_directory_path() / - ("cpplink_arrow_export_" + std::to_string(::getpid())); + ("cpplink_arrow_export_" + cpplink_test::ProcessId()); std::filesystem::create_directories(dir); const std::string path = (dir / "rows.parquet").string(); @@ -274,5 +275,5 @@ TEST(ArrowExport, ParquetRoundTrip) { ArrowArrayStream missing; EXPECT_FALSE(cpplink::OpenParquetStream(path, {"gone"}, &missing, &error)); EXPECT_NE(error.find("\"gone\" is not in"), std::string::npos) << error; - std::filesystem::remove_all(dir); + cpplink_test::RemoveAll(dir); } diff --git a/tests/batch_loader_test.cpp b/tests/batch_loader_test.cpp index 309f37d..98da528 100644 --- a/tests/batch_loader_test.cpp +++ b/tests/batch_loader_test.cpp @@ -24,11 +24,12 @@ #include #include #include -#include #include "cpplink/parquet_loader.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -505,7 +506,7 @@ TEST(BatchLoader, MatchesTheParquetPathMemberForMember) { {true, true, false, true})}); const auto dir = std::filesystem::temp_directory_path() / - ("cpplink_batch_loader_" + std::to_string(::getpid())); + ("cpplink_batch_loader_" + cpplink_test::ProcessId()); std::filesystem::create_directories(dir); const std::string path = (dir / "table.parquet").string(); auto sink = arrow::io::FileOutputStream::Open(path); @@ -513,11 +514,14 @@ TEST(BatchLoader, MatchesTheParquetPathMemberForMember) { ASSERT_TRUE(parquet::arrow::WriteTable(*table, arrow::default_memory_pool(), *sink, /*chunk_size=*/3) .ok()); + // `WriteTable` does not close a stream it was handed, and a file with a + // handle still on it is one Windows will not let the removal below delete. + ASSERT_TRUE((*sink)->Close().ok()); cpplink::RecordStore from_file(schema); std::string error; ASSERT_TRUE(cpplink::LoadParquet(path, schema, &from_file, nullptr, &error)) << error; - std::filesystem::remove_all(dir); + cpplink_test::RemoveAll(dir); // The same table, decoded from its buffers in batches of two. cpplink::RecordStore from_buffers(schema); diff --git a/tests/boolean_test.cpp b/tests/boolean_test.cpp index 23f3702..0c0c6ec 100644 --- a/tests/boolean_test.cpp +++ b/tests/boolean_test.cpp @@ -22,7 +22,6 @@ #include #include #include -#include #include "cpplink/blocking.hpp" #include "cpplink/comparison.hpp" @@ -35,6 +34,8 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "cpplink/score.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -292,8 +293,8 @@ class BooleanLoadFixture : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_boolean_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_boolean_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); path_ = (dir_ / "flags.parquet").string(); } @@ -319,6 +320,8 @@ class BooleanLoadFixture : public ::testing::Test { *sink, /*chunk_size=*/3) .ok()); + // `WriteTable` does not close a stream it was handed. + ASSERT_TRUE((*sink)->Close().ok()); } std::filesystem::path dir_; diff --git a/tests/cluster_test.cpp b/tests/cluster_test.cpp index 396e004..52a5e17 100644 --- a/tests/cluster_test.cpp +++ b/tests/cluster_test.cpp @@ -15,13 +15,14 @@ #include #include #include -#include #include "cpplink/merge_edges.hpp" #include "cpplink/predict.hpp" #include "cpplink/record_store.hpp" #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -40,8 +41,8 @@ class ClusterFixture : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / - ("cpplink_cluster_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_cluster_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); cpplink::Schema schema; @@ -54,7 +55,7 @@ class ClusterFixture : public ::testing::Test { store_->Finalize(); } - void TearDown() override { std::filesystem::remove_all(dir_); } + void TearDown() override { cpplink_test::RemoveAll(dir_); } // Writes one shard in the exact form predict emits. std::string WriteShard(const std::string& name, diff --git a/tests/edge_table_test.cpp b/tests/edge_table_test.cpp index 9ab2bdc..caa29e9 100644 --- a/tests/edge_table_test.cpp +++ b/tests/edge_table_test.cpp @@ -20,7 +20,6 @@ #include #include #include -#include #include "cpplink/arrow_export.hpp" #include "cpplink/blocking.hpp" @@ -31,6 +30,8 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "cpplink/score.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -69,8 +70,8 @@ class EdgeTableFixture : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_edge_table_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_edge_table_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); std::string error; ASSERT_TRUE(cpplink::ParseSchema(kSchemaJson, &schema_, &error)) << error; diff --git a/tests/explain_batch_test.cpp b/tests/explain_batch_test.cpp index 20e1268..409927b 100644 --- a/tests/explain_batch_test.cpp +++ b/tests/explain_batch_test.cpp @@ -21,11 +21,12 @@ #include #include #include -#include #include "cpplink/app.hpp" #include "cpplink/model.hpp" #include "cpplink/sample_data.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -67,8 +68,8 @@ class ExplainBatch : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_explain_batch_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_explain_batch_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); data_ = (dir_ / "sample.parquet").string(); schema_ = (dir_ / "schema.json").string(); diff --git a/tests/explain_test.cpp b/tests/explain_test.cpp index 66f9d1d..f87a78a 100644 --- a/tests/explain_test.cpp +++ b/tests/explain_test.cpp @@ -3,6 +3,7 @@ #include "cpplink/explain.hpp" +#include #include #include #include diff --git a/tests/fit_test.cpp b/tests/fit_test.cpp index 84122b0..5ee0bdb 100644 --- a/tests/fit_test.cpp +++ b/tests/fit_test.cpp @@ -14,7 +14,6 @@ #include #include -#include #include "cpplink/app.hpp" #include "cpplink/blocking.hpp" @@ -24,6 +23,7 @@ #include "cpplink/record_store.hpp" #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" namespace { @@ -290,9 +290,8 @@ TEST_F(FitFixture, TheReportPrintsTheFit) { // --report writes the full report beside the compact one on the terminal. TEST(FitReport, ReportOptionWritesTheFullReport) { - const std::filesystem::path dir = - std::filesystem::temp_directory_path() / - ("cpplink_fit_report_" + std::to_string(::getpid())); + const std::filesystem::path dir = std::filesystem::temp_directory_path() / + ("cpplink_fit_report_" + cpplink_test::ProcessId()); std::filesystem::create_directories(dir); const std::string parquet = (dir / "sample.parquet").string(); cpplink::SampleOptions sample; diff --git a/tests/id_index_test.cpp b/tests/id_index_test.cpp index 379e6e8..da3d552 100644 --- a/tests/id_index_test.cpp +++ b/tests/id_index_test.cpp @@ -23,7 +23,6 @@ #include #include #include -#include #include "cpplink/app.hpp" #include "cpplink/cluster.hpp" @@ -33,6 +32,8 @@ #include "cpplink/recall.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -55,14 +56,27 @@ std::unique_ptr SharedIdStore() { TEST(DatasetNames, AreTheFileStemsMadeDistinctAndCsvSafe) { EXPECT_TRUE(cpplink::DatasetNamesFor({"only.parquet"}).empty()); const std::vector names = cpplink::DatasetNamesFor( - {"/data/a.parquet", "b/a.parquet", "x,y:z.parquet", "/other/b.parquet"}); + {"/data/a.parquet", "b/a.parquet", "x,y.parquet", "/other/b.parquet"}); ASSERT_EQ(names.size(), 4u); EXPECT_EQ(names[0], "a"); EXPECT_EQ(names[1], "a#1"); - EXPECT_EQ(names[2], "x_y_z"); + EXPECT_EQ(names[2], "x_y"); EXPECT_EQ(names[3], "b"); } +// The other character a name may not carry, checked where a file can carry it. +// Windows forbids a colon in a filename and its path parser reads one in a path +// as punctuation rather than as part of the stem, so there the rule guards +// against a name no file can have. +#ifndef _WIN32 +TEST(DatasetNames, AColonInAStemIsReplaced) { + const std::vector names = + cpplink::DatasetNamesFor({"x:y.parquet", "b.parquet"}); + ASSERT_EQ(names.size(), 2u); + EXPECT_EQ(names[0], "x_y") << "a name is written into `dataset:id`"; +} +#endif + TEST(DatasetNames, ASingleInputHasNoNameAndNoQualifier) { cpplink::Schema schema; schema.unique_id = "id"; @@ -181,8 +195,8 @@ class SharedIdFiles : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_shared_ids_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_shared_ids_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); store_ = SharedIdStore(); } @@ -465,8 +479,8 @@ class SharedIdCommands : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_shared_id_cli_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_shared_id_cli_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); left_ = (dir_ / "left.parquet").string(); right_ = (dir_ / "right.parquet").string(); @@ -510,6 +524,8 @@ class SharedIdCommands : public ::testing::Test { ASSERT_TRUE( parquet::arrow::WriteTable(*table, arrow::default_memory_pool(), *sink, 1024) .ok()); + // `WriteTable` does not close a stream it was handed. + ASSERT_TRUE((*sink)->Close().ok()); } std::filesystem::path dir_; diff --git a/tests/init_test.cpp b/tests/init_test.cpp index 8cea506..c27881a 100644 --- a/tests/init_test.cpp +++ b/tests/init_test.cpp @@ -14,13 +14,14 @@ #include #include #include -#include #include "cpplink/app.hpp" #include "cpplink/parquet_loader.hpp" #include "cpplink/record_store.hpp" #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -93,7 +94,7 @@ class DraftFixture : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_init_" + std::to_string(::getpid())); + ("cpplink_init_" + cpplink_test::ProcessId()); std::filesystem::create_directories(dir_); path_ = (dir_ / "sample.parquet").string(); cpplink::SampleOptions options; @@ -102,7 +103,7 @@ class DraftFixture : public ::testing::Test { std::string error; ASSERT_TRUE(cpplink::WriteSampleParquet(path_, options, &error)) << error; } - void TearDown() override { std::filesystem::remove_all(dir_); } + void TearDown() override { cpplink_test::RemoveAll(dir_); } const cpplink::DraftColumn* Column(const cpplink::DraftReport& report, const std::string& name) { @@ -289,6 +290,8 @@ TEST_F(DraftFixture, DraftsALinkFromTheFirstInputAndChecksTheOthers) { ASSERT_TRUE(parquet::arrow::WriteTable(*table, arrow::default_memory_pool(), *sink, 1024) .ok()); + // `WriteTable` does not close a stream it was handed. + ASSERT_TRUE((*sink)->Close().ok()); }; write(narrow, {arrow::field("id", arrow::utf8())}, {id_array}); std::vector columns; diff --git a/tests/levels_test.cpp b/tests/levels_test.cpp index 8afc647..137cebe 100644 --- a/tests/levels_test.cpp +++ b/tests/levels_test.cpp @@ -389,7 +389,7 @@ TEST_F(LevelsFixture, ReportsPrintAndParse) { std::ostringstream json; WriteLevelsJson(report, json); - EXPECT_NO_THROW(nlohmann::json::parse(json.str())); + EXPECT_NO_THROW((void)nlohmann::json::parse(json.str())); } TEST(LevelsCommand, NeedsSchemaAndData) { diff --git a/tests/loader_types_test.cpp b/tests/loader_types_test.cpp index 7f8a457..b94e310 100644 --- a/tests/loader_types_test.cpp +++ b/tests/loader_types_test.cpp @@ -20,11 +20,12 @@ #include #include #include -#include #include "cpplink/parquet_loader.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -32,8 +33,8 @@ class LoaderTypes : public ::testing::Test { protected: void SetUp() override { dir_ = std::filesystem::temp_directory_path() / - ("cpplink_loader_types_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_loader_types_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); path_ = (dir_ / "typed.parquet").string(); } @@ -52,6 +53,8 @@ class LoaderTypes : public ::testing::Test { *sink, /*chunk_size=*/2) .ok()); + // `WriteTable` does not close a stream it was handed. + ASSERT_TRUE((*sink)->Close().ok()); } template diff --git a/tests/merge_edges_test.cpp b/tests/merge_edges_test.cpp index bed9c80..8920d93 100644 --- a/tests/merge_edges_test.cpp +++ b/tests/merge_edges_test.cpp @@ -15,11 +15,12 @@ #include #include #include -#include #include "cpplink/predict.hpp" #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -42,8 +43,8 @@ class MergeFixture : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. root_ = std::filesystem::temp_directory_path() / - ("cpplink_merge_" + std::to_string(::getpid())); - std::filesystem::remove_all(root_); + ("cpplink_merge_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(root_); std::filesystem::create_directories(root_); cpplink::Schema schema; @@ -56,7 +57,7 @@ class MergeFixture : public ::testing::Test { store_->Finalize(); } - void TearDown() override { std::filesystem::remove_all(root_); } + void TearDown() override { cpplink_test::RemoveAll(root_); } std::filesystem::path MakeDir(const std::string& name) const { const std::filesystem::path dir = root_ / name; diff --git a/tests/minhash_test.cpp b/tests/minhash_test.cpp index a463004..f08fb5f 100644 --- a/tests/minhash_test.cpp +++ b/tests/minhash_test.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include diff --git a/tests/predict_test.cpp b/tests/predict_test.cpp index e74e690..be88e26 100644 --- a/tests/predict_test.cpp +++ b/tests/predict_test.cpp @@ -15,7 +15,6 @@ #include #include -#include #include "cpplink/blocking.hpp" #include "cpplink/comparison.hpp" @@ -24,6 +23,8 @@ #include "cpplink/record_store.hpp" #include "cpplink/schema.hpp" #include "cpplink/score.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -65,8 +66,8 @@ class PredictFixture : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / - ("cpplink_predict_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_predict_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); std::string error; diff --git a/tests/process_id.hpp b/tests/process_id.hpp new file mode 100644 index 0000000..c5f9e2d --- /dev/null +++ b/tests/process_id.hpp @@ -0,0 +1,27 @@ +// Copyright 2026 Mathieu Fourment +// SPDX-License-Identifier: MIT + +#pragma once + +#include + +#ifdef _WIN32 +#include +#else +#include +#endif + +namespace cpplink_test { + +// The process id as text, for naming a temporary directory that no other run of +// the test binary shares. Windows spells the call with a leading underscore and +// declares it in a different header, which is the whole reason this exists. +inline std::string ProcessId() { +#ifdef _WIN32 + return std::to_string(_getpid()); +#else + return std::to_string(::getpid()); +#endif +} + +} // namespace cpplink_test diff --git a/tests/rescore_test.cpp b/tests/rescore_test.cpp index e73bf7f..7f6efb9 100644 --- a/tests/rescore_test.cpp +++ b/tests/rescore_test.cpp @@ -16,7 +16,6 @@ #include #include -#include #include "cpplink/blocking.hpp" #include "cpplink/comparison.hpp" @@ -26,6 +25,8 @@ #include "cpplink/schema.hpp" #include "cpplink/score.hpp" #include "cpplink/spill.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -67,8 +68,8 @@ class RescoreFixture : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / - ("cpplink_rescore_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_rescore_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); std::string error; diff --git a/tests/roundtrip_test.cpp b/tests/roundtrip_test.cpp index ca5d133..bb69309 100644 --- a/tests/roundtrip_test.cpp +++ b/tests/roundtrip_test.cpp @@ -12,7 +12,6 @@ #include #include -#include #include "cpplink/app.hpp" #include "cpplink/blocking.hpp" @@ -25,6 +24,8 @@ #include "cpplink/sample_data.hpp" #include "cpplink/schema.hpp" #include "cpplink/string_metrics.hpp" +#include "tests/process_id.hpp" +#include "tests/temp_dir.hpp" namespace { @@ -73,8 +74,8 @@ class RoundTrip : public ::testing::Test { // a fixed name has concurrent cases of this fixture writing the same files // and deleting the directory under one another. dir_ = std::filesystem::temp_directory_path() / - ("cpplink_roundtrip_" + std::to_string(::getpid())); - std::filesystem::remove_all(dir_); + ("cpplink_roundtrip_" + cpplink_test::ProcessId()); + cpplink_test::RemoveAll(dir_); std::filesystem::create_directories(dir_); data_ = (dir_ / "sample.parquet").string(); link_ = (dir_ / "sample.b.parquet").string(); diff --git a/tests/temp_dir.hpp b/tests/temp_dir.hpp new file mode 100644 index 0000000..89319d2 --- /dev/null +++ b/tests/temp_dir.hpp @@ -0,0 +1,56 @@ +// Copyright 2026 Mathieu Fourment +// SPDX-License-Identifier: MIT + +#pragma once + +#include +#include +#include +#include +#include +#include + +namespace cpplink_test { + +// Removes a temporary directory a test owns, retrying while the platform says a +// file in it is still in use. Windows refuses the delete until the last handle +// on a file is gone, and the last handle is not always one the test can see: a +// reader's read-ahead can outlive the object that owns it, and a virus scanner +// opening a file the test has just written is enough on its own. Posix unlinks +// an open file without complaint, so the same lingering handle costs nothing +// there and this loop returns on its first attempt. +// +// A handle the process really leaked never clears, and that is a bug worth +// failing on, so giving up names the entry that is still held rather than the +// directory the caller passed -- which is all the platform's own message says. +inline void RemoveAll(const std::filesystem::path& dir) { + constexpr int kAttempts = 40; + constexpr auto kPause = std::chrono::milliseconds(50); + for (int attempt = 0; attempt < kAttempts; ++attempt) { + std::error_code ec; + std::filesystem::remove_all(dir, ec); + if (!ec) return; + std::this_thread::sleep_for(kPause); + } + // Still there after the whole budget: remove the entries one at a time, so + // the failure names the file rather than its directory. The paths are taken + // first and removed deepest first, because the walk cannot outlive the + // entries it is walking. + std::error_code ec; + std::vector entries; + for (const auto& entry : std::filesystem::recursive_directory_iterator(dir, ec)) { + entries.push_back(entry.path()); + } + for (auto it = entries.rbegin(); it != entries.rend(); ++it) { + std::error_code removed; + std::filesystem::remove(*it, removed); + if (removed) { + throw std::filesystem::filesystem_error( + "still held after " + std::to_string(kAttempts * kPause.count()) + " ms", + *it, removed); + } + } + std::filesystem::remove_all(dir); +} + +} // namespace cpplink_test